Git Development Guidelines

[jzzheng22]

Developers should try to follow the same processes when contributing to ISSIE to ensure changes are easy to follow and keep track of.

This would include:

• Use of PRs
• How to merge PRs (merge commit, squashing, or rebasing)
• Commit messages

Pull Requests

My personal preference is for PRs to be squashed. Note squash instead of rebase: this allows for PRs to be reverted more easily than a rebase, since the PR is contained in a single commit rather than many commits.
Pros:

• Cleaner, linear history: easier to follow changes
• Does not 'pollute' the commit history with merge commits: less cognitive strain when looking at commit history

Cons:

• Rebase is more complicated than merge: those not familiar with Git may find this as a deterrent to contributing. This is particularly a problem with those who will most likely be contributing to ISSIE (university students), as proper Git practices are (currently) not taught (at least at Imperial EE).
  • I would say that learning how to rebase is a very valuable Git skill.
• Rebasing is a destructive operation: you might lose information.
• Repeatedly resolving merge conflicts for each individual commit gets very tedious very quickly.

When rebasing, I prefer to do an interactive rebase (git rebase -i master), so I can see exactly how the git history will be changed.
Developers should use git push --force-with-lease when pushing their work upstream after a rebase: this should help prevent loss of work.

Note: I've found that using GitHub Desktop for rebases is a bit annoying. I always use terminal so this isn't a problem for me, but someone who is only comfortable using GitHub Desktop may struggle with rebases.

Commit Messages

Commit messages should be informative and concise. Someone reading the commit message should ideally be able to understand what that commit actually changes (e.g. 'fix bug' is not a good commit message).

Assuming that PRs are squashed: the PR should be named such that it is obvious what that PR resolves, since that name will be the commit message once merged.

[jzzheng22]

CC @apantelopoulos @tomcl @adidesh20

[tomcl]

Good detailed background on this topic

How to use rebasing properly - Golden Rule

My view - rebasing is a good idea and safe but is must be done locally on a devs private branch

Following this workflow:

Local Cleanup

One of the best ways to incorporate rebasing into your workflow is to clean up local, in-progress features. By periodically performing an interactive rebase, you can make sure each commit in your feature is focused and meaningful. This lets you write your code without worrying about breaking it up into isolated commits—you can fix it up after the fact.

When calling git rebase, you have two options for the new base: The feature’s parent branch (e.g., main), or an earlier commit in your feature. We saw an example of the first option in the Interactive Rebasing section. The latter option is nice when you only need to fix up the last few commits. For example, the following command begins an interactive rebase of only the last 3 commits.

git checkout feature git rebase -i HEAD~3

By specifying HEAD~3 as the new base, you’re not actually moving the branch—you’re just interactively re-writing the 3 commits that follow it. Note that this will not incorporate upstream changes into the feature branch.

[tomcl]

(1) @jzzheng22 does the above local rebase workflow do what you want? Is it what you now do?
(2) force-push should only be used with great caution when you have not followed correct workflow

One of the only times you should be force-pushing is when you’ve performed a local cleanup after you’ve pushed a private feature branch to a remote repository (e.g., for backup purposes). This is like saying, “Oops, I didn’t really want to push that original version of the feature branch. Take the current one instead.” Again, it’s important that nobody is working off of the commits from the original version of the feature branch.

[tomcl]

One thing I'm not sure about is how to deal with upstream changes.

I like to merge them in ASAP because sorting out conflicts is easier than if all at once - but maybe that is not true?

However if this is done, what does that do to rebasing? Not sure I understand this. I think that you cannot merge in upstream commits and then rebase because the upstream commits then get rewritten which is no good for other devs?

Not really sure. No decision on what we do till this is resolved

[jzzheng22]

What I do is periodically rebase my local branch on top of master:

1. On my (local) master branch: git pull upstream master
2. Checkout my dev branch: git checkout my-dev-branch
3. On my dev branch: git rebase -i master.
4. Rebase: squash/edit/fixup any commits to 'clean up' my history, and solve any merge conflicts (if any)
5. After rebase: git push --force-with-lease to publish my local changes to my branch on GitHub. This force push is necessary because after the rebase you have rewritten your git history. Of course, if you hadn't published your branch before, then the force push is unnecessary: you can just push normally, since you aren't rewriting anything.

This link explains git push --force-with-lease better than I can: https://stackoverflow.com/questions/52823692/git-push-force-with-lease-vs-force. Essentially, this is a safer version of git push --force.

To get any changes from upstream, you would not use git merge: you would rebase your branch on top of master instead.

Admittedly, this is more work than a simple git merge, but I think overall worth it.

[tomcl]

How to deal with upstream commits

I'm still not certain about this.

The issue about merging upstream commits is when your development on a feature goes for a long time before you have it fully working and therefore ok to put on master.

In that case (I think) merging upstream commits into your feature branch helps because you are developing on the most recent version,
therefore there is less likely to be a conflict.

Variants of force, and safety

force-with-lease vs force-with-includes

See the answer to this question for an insightful description

https://stackoverflow.com/questions/65837109/when-should-i-use-git-push-force-if-includes

Summary: neither force-with-lease nor force-with-includes is 100% safe unless used on purely local repo commits. It seems possible that force-with-includes is a bit safer than force-with-lease, which itself is safer than force.

[jzzheng22]

Might be a misunderstanding here - you are free to keep rebasing your branch on top of master to get the latest upstream commits while you are still developing your own features - this is what I've been doing on my own local branch. I have all of the latest commits from master (save from the one from the PR just merged).

And yes.. git push --force-with-lease --force-if-includes does seem to be safer. I hadn't seen this option before!

[tomcl]

OK. Thanks Jason. I've thought about this and now I think I understand things fully:

1. Rebasing a private dev branch (in local repo) on top of master changes commits in local repo to incorporate incoming master commits before the local changes by changing each of the local dev branch commits so they diff from a changed parent commit. Before rebase a master FETCH must always be done to make local master uptodate.
2. This works when no local updates from master have been made (so the branch does not contain merge commits).
3. If the commits to be rebased have been pushed to upstream (as would usually be the case) a force commit will be required.
4. Force commit is OK as long as dev branch is truly private - no-one else must make commits based on the local commits.
5. We get some protection from disaster (if anyone disobeys 3) by using cleverer versions of force that fail in typical cases where others have built onto private commits (the descendant commits, if any exist, would be broken by the rebase). Note that no-one should disobey (3) in which case protection does not matter.
6. A set of private commits, before rebasing, can be re-ordered, squashed, etc to make them reflect the logical dev steps.

For a dev workflow therefore we need something like:

• Private (no-one else is allowed to merge commits from them into their work) feature branches have some fixed name: I suggest <initials>-dev.
• All development is done on these dev branches
• Instead of merging master commits into dev branches regularly, devs should rebase dev branches on master as soon as:
  • New commits on master
  • They have reached a state where dev can be checkpointed (note that the feature in development does not need to be working)
• As soon as a new feature passes all tests (in other words the intended enhancement may not be complete, but what is complete works fully) the local branch can be rebased (one final time)
• It is good practice to re-order and squash local commits before rebasing so that commit names make sense and reflect work.
  • Do this in github desktop (drag-and-drop) or with git rebase -i.
  • Why is github desktop rebase difficult to use? Worth understanding this.
• Legacy dev. Anyone with current dev branch that contains merges can rebase back to most the recent master merge?? Does rebase do this automatically?

[jzzheng22]

Above all seems fine, with a few comments:

• Dev branches don't need to be fixed name - personally I would name a branch according to the feature that I am implementing at the time. Perhaps prefixed with my initials.
• Should make it clear during write-up of guidelines that once work is merged onto master, that dev branch needs to be deleted. You could create a new branch with the same name after deletion if desired.
• I think the GitHub Desktop issue was because there were multiple people working on that branch at the time (unlikely to happen with ISSIE - this case was a group coursework hence people working on the same thing at the same time). If someone else did a rebase, and then you used Desktop to rebase later on, there were issues with pulling the most recent changes I think. I haven't faced the issue myself, but I know of others who did.
• Unsure about best thing to do with legacy dev. Not sure how the merge commits would work here. I think a rebase automatically drops any merge commits, but I'm not sure how that will affect the code. Could be worth trying and seeing what the effects are, and if there are problems, just use merge commits for those ones, then rebase for future dev.

[tomcl]

• OK, let us keep legacy dev simple and merge it.
• Agreed branches can reflect feature name but need initials as well. Branches NOT private other than master can replace initials by dev perhaps?
• Suggest you do write-up under separate Issie wiki page - Github usage Guidelines

[jzzheng22]

One more thing: ideally the size of a PR should be quite small so they are easy to review. However, with FYP work (such as for myself and @adidesh20), I'm not sure what the best practice here is. I already have a lot of changes just for an MVP, and most of these changes are required in order to not break master.

[tomcl]

PRs to master need to be working. That may not be possible quickly in some cases. I suggest it is OK to have big PRs - they should have well chosen commits that segment design.

[jzzheng22]

One more thing before I do a full write-up: @tomcl I suggest you set up branch protection rules that disallow people from pushing directly to master. This will ensure everything goes via a code review and PR before it enters master.