AGS project Branch & Commit Conventions - Proposal Draft

Started by Crimson Wizard, Thu 13/02/2014 13:57:26

Previous topic - Next topic

Crimson Wizard

//===========================================

Foreword

The branching model for AGS is defined considering the specifics of its development.
The development roughly breaks into following tasks:
1. Developing the Editor
2. Developing the Engine (interpreter) core functionality
3. Developing the Engine ports

The game features provided for creating and editing by the Editor should correspond to the feature support by the Engine. However, particular changes may be strictly related to only one of those programs.
The engine ports sometimes require their own improvements. Compatibility fixes providing support for running older games are usually made in sake of ports, but the changes occur in the Engine's core.

Branches.

Generally we base the branching model on a following reference:
http://nvie.com/posts/a-successful-git-branching-model/
It seems easy to follow and helps to keep most stable code in the primarily accessed branch, therefore the users who prefer making builds themselves will get the last fixed stable version without searching for proper branch.

On top of this model we add one exception for hotfixes and minor compatibility fixes: we allow them to be applied to master branch directly, and also we allow not to increase the release version after every single immediate bugfix and compatibility fix.
This makes it generally simplier to add ones; and it is still possible for bug reporters to mention the precise program state they used by refering to commit they were at when making the build (by commit hash).
This exception may be removed later should the development process become more complex or should such exception cause troubles.

Branch summary

In summary, the branching rules are following:
master - is a stable branch, which receives merges from "release" and "hotfix" branches when the code is good enough. There are tags, that mark version releases. Changes should not be made directly to "master", with previously mentioned exception of immediate bug fixes and very simple compatibility fixes.
Only "hotfix" and "release" branches could be merged in. This branch is meant for common end-users, who want be sure they are always using "official" stable version.

hotfix - these branches are used when the serious bugs are found in recently made release, that is when the new released code is already in "master". After making commits and testing them, the contents of a branch are merged to "master".

release - auxiliary branches for the final preparations before new version release. It is forked from "develop" branch when it is clear no more features are to be added to the nearest upcoming version. Only bugfixes are allowed. When it is done, it is merged to "master".

develop - main development branch. All new things are done here. When approaching new release, the "release" branch is spawned from "develop". From that moment all fixes to the closest release are done in related branch, and "develop" is used to start working on the next release.

feature - feature branches are temporary auxiliary branches. They are forked from any other branch (presumably from "master" or "develop") by a team or individual developers, to work separately on certain feature. It is strongly recommended to create "feature" branches in personal repositories, rather than in central one, unless it requires attention of multiple developers over a long period of time. The feature may target next upcoming release, or some distant release in future. When finished, it is merged to "develop".


Branch naming

The "master" and "develop" branches always keep their respective names. Other branches are named with regard to the title of version or feature they are dedicated for.
Hotfix branch should be named "hotfix-X.X.X.Z", where X.X.X is the last release version and Z is a new planned build number.
Release branch should be named "release-X.X.X", where X.X.X is the planned version number (without last build number, because it may eventually change).
Feature branch's name should carry the indication of the branch's purpose. It may start with "feature-" prefix for better visibility. It is recommended to give unique and expressive names. For example, "my_new_feature" or "my_work" are bad names, because they do not indicate the feature meaning. "new-script-commands" is not a very good name either, because there may be more than one branch dealing with new script commands.


Branch rules

All branches except "master" and "develop" are deleted some time after they were merged - to solidify their "finished" state (remember this won't delete commits, only branch's header).

The main "develop" branch is meant for working towards next major release. Sometimes it may be planned to make an intermediate minor release, or such need arises unexpectedly. This might be difficult to separate the development of minor and major version in time on one branch, so that major changes come only after minor ones and the minor release would not include them.
In such case it is recommended to create a separate branch named "develop-X.X.X" to work on minor update (where X.X.X is a planned version).
It is forbidden to have more than one major and one minor develop branches at a time. "Feature" branches should be used to work on and store code until the time when it may be merged into "develop".

After hotfix or release is merged into "master", it is recommended to merge them into nearest "develop" one (minor, then major) as soon as possible.
Similarily, when minor development branch is finished to be worked on (and new release branch spawns out of it), it should be merged into main "develop".
It is also recommended to merge "release" and minor development branches into main "develop" from time to time to keep things synced (especially if serious bugs were fixed in the former).

Feature branches carry little obligations. They may be postponed for undefined time, deleted, if appear to be a failure. They may be created solely to experiment on something.



Pull requests.

It is suggested to do most work on new features in personal repositories. Even if the developer has direct access to central repository, it is recommended to create personal branches when working on larger changes that require time to complete, review and test. This prevents from messing up commits from different developers, and also helps to distinguish commits related to certain feature or set of features.

Prior to other things, we recommend to setup a connection between your personal repository and central AGS repository. This page gives general tips on how to do this, and how to synchronize main branches between different repositories:
https://help.github.com/articles/fork-a-repo#step-3-configure-remotes
https://help.github.com/articles/fork-a-repo#pull-in-upstream-changes

Pull requests from personal repositories are used to merge developer's changes into central repository.
The most important rule for planned pull request is to work in a separate branch, even if the change is small, one branch per pull request, and one pull request for a feature or closely related set of small features.
This will allow you to keep your new feature code safe, not mess up irrelevant commits, and still be able to sync the main branches ("master", "release", "develop") in your personal repository with ones in central repository.
Remember that, if necessary, you may keep the feature branch unmerged for a long time, and merge only when it is most convenient to do so.
This is also important to remember that in most times there's no need to regularly update your feature branch with changes from "master" (or other main branches). Instead, if the main branch has changed too much, you may rebase your feature branch on top of updated main one. This helps to keep the feature branch and pull request cleaner, which also improves readability of changes history.

Pull requests should never contain irrelevant changes. Consider making two (or more) pull requests with different feature branches if all of those changes are required. If some changes are not needed at all, remove them prior to making pull request. Remember that Git allows to fix and copy branches and individual commits.
If you are making several minor changes that have similar meaning or related to each other consider making one pull request (these changes may be divided into several commits if wanted).

//===========================================
... to be continued with commit conventions.
//===========================================

Comments, suggestions, questions?

Wyz

I wholeheartedly agree with this; good show CW! (nod)

I don't have much to comment on I guess. Maybe something about forks; I guess there are two layers of developers working on AGS. Those working on the main project with milestones opted by the community and people working on their own fork having their own milestones. I guess some of these forks might contain useful features we want to pull into the mean project. Is that something that should be done by pull requests on the develop branch or should they first put this in a feature branch?

Anyhow, by the looks of these conventions the start-up of open source AGS is maturing, yay! :-D
Life is like an adventure without the pixel hunts.

Gurok

I feel like this is aimed at me. I've made such a mess of commits, pull requests and branching. I put this mostly down to Git being fairly unforgiving when you make a mistake, but I think I have to accept some responsibility too. I'm sorry CW! I'm getting better at Git, I swear!
[img]http://7d4iqnx.gif;rWRLUuw.gi

Crimson Wizard

#3
Quote from: Gurok on Fri 14/02/2014 00:57:50
I feel like this is aimed at me.
Not really. From what I know almost everyone contributing to AGS since it became opensource was new to Git (me too).
We needed this kind of convention for a long time, but I could not be arsed to write one.
Btw, nothing bad happened anyway.

Quote from: Gurok on Fri 14/02/2014 00:57:50
I put this mostly down to Git being fairly unforgiving when you make a mistake,
Actually it lets you to fix almost everything, although in severe cases that would require collaboration with other people to sync everyone's work.


Crimson Wizard

One thing I am still wondering about is the larger port updates. Thing is that sometimes it might be required to improve the port-related code without changing the code in the engine's core. We may think that port code as a slightly separate project that is not required to follow the general version. Sometimes a port may be updated after the AGS release is made.
Perhaps we may allow to merge port-related features right into the master?

Alternatively we may split ports code out and refer to them as to submodules. In such case they may have their own branches.


Another thing, I found this simple description about how to sync your personal repository with central one without much hassle:
https://help.github.com/articles/fork-a-repo#step-3-configure-remotes
https://help.github.com/articles/fork-a-repo#pull-in-upstream-changes

Crimson Wizard

#6
Recommendations on commits

Naming and commenting

This is generally suggested that commit comment should be broken into "title" (brief summary) and "message" (explanation as detailed as required). The title is seen in git logs when all commits are listed, and also it is used as a file name when creating patches.
There's also a recommended limit to the title length to be around 50 characters (probably has something to do with viewing logs in terminal). GitHub has a limit of 72 characters which it displays in commits history, and the rest is hidden under expandable commentary block.
Hence the recommendations:

1. Commit commentary should consist of brief summary and expanded description, if required.
2. Summary is the first line of no more than 72 characters in length. Summary should give enough information to specify:
- the sort of commit; if that's a bug consider using word "fix" (or any of its form), e.g.:
QuoteFix terrible bug in the engine.
- which program(s) / part of program these changes are applied to, e.g.:
QuoteRoom editor: add copy/paste region tool.
3. If you want to add the description, put one empty line between summary and the rest of comment body.
4. Description may be of any length (I think), but keep it rational. Usually it should contain more detailed description of a new feature, or problem that caused the bug, if the summary wasn't enough, as well as optional explanation of situation, references to similar solutions, comments on why do you have to use a temporary workaround (if you actually do), etc, etc.

Remember that you may reference other commits from same repository/history by putting commit hash (the long weirdly looking hexadecimal number) into description. Those hashes may be used to find exact commits in history. GitHub, as well as many GUI-based git clients allow to click these hashes as if they were hyperlinks and thus navigate to commit.
Example from my own recent commit:
Quote
AGS.Native: fixed import from 2.72 projects

Was broken by: 1c8164d3b11d28a68f9cddcdab521e8be445ecf3


Making commits

It is strongly suggested to not put all the changes you make into one commit without consideration. When reviewing the history of changes it is very important to understand what has changed and why. Also, sometimes when looking for a source of bug we may walk down the history of commits, looking for the one which introduced the error: in this case it really helps when every commit has only task- and thematically related changes.
Sometimes it might be wanted or necessary to fork a new branch so that it contained only certain features, but not others. In such case having all features in one commit will require extra work to split things.

Hence the recommendations:
1. Irrelevant changes should be made as separate commits.
If you make irrelevant changes (not connected by task, theme and/or logically), even though they are applied to different places in code, please, make one commit per change.
2. Each task should be made a separate commit.
Even though a series of tasks may be relevant and connected to each other, it really helps to have them organized into different commits.
For example, when creating whole new class it is recommended to commit the new class as one commit, and putting the class into use in the existing code as a separate commit.
3. If you apply two or more overwriting changes to same place in code, then break these changes into multiple commits.
For example, if you want to both fix a bug in existing code and improve the look of the code, then do so in different commits, e.g. first fix the bug without changing the rest, then refactor/clean the code (or vice-versa).
Why: imagine you made a mistake somewhere when doing this. If you put both changes in one commit, you will wonder - whether you broke the program when cleaning the code or when fixing the bug. If you have them separately, you may easily narrow down the search.

In general: think of the changes you do as of the sequence of tasks. What are dependencies between these tasks? Which can't be completed without others made first? Create commits in the same order.
Remember that you can always fix the history of commits if you mess it up by reorganizing commits on a new auxilary branch, resetting history by "mixed reset" command (could be dangerous if you do hard-reset instead, so mind what you do, better make backups if you don't feel comfortable with Git yet), etc. Git documentation/help/tutorials are all over internet; if you have trouble, create dummy repositories on your hard drive and experiment, that really helps.
More info:
https://help.github.com/articles/what-are-other-good-resources-for-learning-git-and-github
http://git-scm.com/book/


I know I often have an awkward writing style (especially in English, which is not my native language), so tell if something I wrote here is hard to understand or sounds ambiguous. Also, if you have any objections or doubts, please state them (better now than later).

Crimson Wizard

Nick Sonneveld has made a suggestion to change the branch convention we are using to the following:

1. Only one development branch - "master".
2. Most development (features) is done in separate private branches, which are then merged into master after obligatory testing. This keeps master relatively stable at most times.
3. Development period is divided into two stages. During the first stage we accept major changes. When this development period is coming to an end, we stop merging pull requests (although we always allow creation of ones) and fix issues, this is the second stage.
4. When the second stage is over, version is released. We create a "tag", and possibly a "release-xxx" branch at the last commit, to indicate the position of stable state in the history. (We are doing this now anyway)
5. If there are more bugs found in the previous versions, and new version is far from completion to suggest updating to it, then we fix these bugs in the corresponding "release-xxx" branch we created earlier and release a patched version.

The reasoning behind this is to reduce number of branches: only 1 branch as opposed to at least 2 constant branches we supposedly have now (master and develop), and since development is done in "master", it will be easier for the users of the code to understand what is going on, and where to make pull requests.
Second reason is supposedly to encourage faster/smaller releases and stimulate better testing.


Personally, one adjustment that I would do is to create release-xxx branches at the start of the beta (issue fixing) stage, instead of final release, to allow futher development while waiting for the results of the beta test.

Other than that, I could not see possible problems that such method would create, that aren't an issue in the method we currently use.

SMF spam blocked by CleanTalk