-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Policy around git branching & merging process #6
Comments
FWIW - some thoughts from me. A mini workshop on this is probably worthwhile for anyone working regularly on the code. I still find git hard and have only just got in to a work flow I feel happy with on individual projects, let alone collaborative ones. On specific questions:
I think yes - mainly as I think it will make it easier to encourage the behaviours you desire from the beginning.
As above, but accept that a lot of hand-holding may be needed to begin with.
I think you should encourage this as it definitely makes things a lot more easier for the project as a whole if not the individual. Again, lots of on-boarding and hand-holding to begin with but think it's worthwhile in the long run. |
Som thoughts:
Yes - I think we should assume that people will want to try our packages from the get go, and cloning/installing from GH will get the master branch, which therefore needs to be production grade.
This is a little more challenging. I - and I think other people - tend to work on a single feature, say an Rcpp implementation of
I would say the squash or rebase options make more sense. I typically rebase when I'm supporting a lead developer, taking their work as canonical and mine as having to conform to their (upstream) choices. I'd say squash and merge is probably better if we do atomic commits (as I do), and you don't want the commit history clogged with quite small changes, if that makes sense.
I don't mind this.
This would be ideal. |
As a follow up: I’m a little wary of the advice here https://github.blog/2022-06-30-write-better-commits-build-better-projects/ |
Thanks both for your comments! 👍 @pratikunterwegs, I understand your concerns and I appreciate using one PR per feature & reorganising commits takes time, which means we might produce slightly less code. However:
|
I do think you bring up a great point by saying we should not "present our development process in a way it didn't actually happen" though. I don't not think it's what we would be doing by reordering / cleaning up commits. It would be an entire part of our development process, useful to our stated goals, and not a sneaky attempt at making things look better than they actually do. I do agree however that we should make clear to potential contributors that this is the result of a conscious effort and that we do not immediately come up with perfect commits, nor should they be expected to. |
I don't mind either of these policies - I'm yet to see a working example in our context as we don't have a lot of people working on different repos just yet, so it might become clear over time. |
Thanks all for this conversation - very instructive and interesting! :) Probably stating the obvious, but all the above strategies come with a price: overhead for the dev, risks of conflicts, etc. On the points raised so far, my 2-cents
Yes. The only drawback I can see there is automation using GH actions to push to main requires privileges but there are workarounds.
I think yes, otherwise it feels like setting up traps for our dev team.
Definitely sounds like a feature we'll want to use. Any idea when this becomes stable?
It sounds like squashing when merging into main would be a simple way to clean up history. I like seeing the details of the commits when reviewing code, but once it's made it to main, I find more streamlined history easier to read.
I would tend to echo @pratikunterwegs and think rewriting the chronology of code creation may be misleading, though I can see the appeal. For now I would think this might be more overhead for the devs than is needed, but again, this is a personal take. I just have not seen situations where not doing this led to headaches (but admittedly I have not contributed much to large, heavily collaborative projects). If we go down the route of squash-merge, it seems this would only useful before merging, i.e. for code reviews, right? git versus githubAs part of our guidelines, we probably want to be explicit about what events are expected to take place on the github website/client, or locally with git. For instance, one could merge a branch on main locally and push to the remote, which would bypass other processes and close associated PRs. Similarly, one could squash commits before pushing to remote for a PR, which is probably not what we want? |
Thanks for bringing this up @thibautjombart. I would say anything that moves from |
What specific workflows do you have in mind? Things as the
No idea. I'll try to investigate.
Yes, if we stick to PRs focused on a single task/feature, then squash & merge is roughly equivalent to reorganising commits (just all commit are reorganised into one). I would be okay with this. |
Yup. My current workaround is used here and uses a PAT to push to main. Indeed triggering the job to run on PRs would remove the need to push to main. There are two use-cases where this might remain useful:
|
If the workflow fails in the PR, we will probably want to restart it from there anyways so it should not be an issue in practice.
I think that's a fair concern and I have definitely done this in the past but since this project involves many collaborators, it might be good to try and move away from this. |
All fair points. I think protecting main has mostly advantages, and the need for GH actions to push to main is probably going to be very rare, if it happens at all. Let's go with it then. |
I'm seeing 'Q4 2022 – Oct-Dec' for the merge queues / merge groups on the GitHub public roadmap but it's not clear to me if this is the date for the public beta release (it's currently in limited public beta) or the official release. |
Maybe the public beta release would be worth a try in any case? We could use it on a couple of active projects and see how it goes. |
Unrelated to above: what should we do with merged feature branches? Delete or keep? I would say delete. |
Yes, my vote is for "delete" as well. I always set it up to auto-delete in the projects I maintain and would recommend we do the same here. |
+1 to delete |
Mergin / rebasing locallyWe probably also want to discuss our policy on updating branches locally. E.g. if 'dev' is out of date with main, do we want to merge or rebase onto main? I usually do the former, but feel rebasing may be the better option to get clean linear history. Reviewing PRsDrawing lessons from this PR, it felt like merging could have happened faster (but curious to hear your thoughts). It underwent multiple rounds of reviews, and while the suggested changes were in general useful (e.g. improving tests), they were not directly linked to the initial purpose of the PR (creating the base structure of the package). I have the impression the following might help in the future:
|
A very important point: If we use a certain method to merge PRs, we have to use the same method to update our local branches. And vice versa. So, the choice we make here to merge PRs will also impact the daily workflow of our devs with their local branches (it should not be an issue for punctual contributors but only for more regular contributors/devs). |
I think going the |
The goal of this discussion is to settle on a set of common practices around branches & merges in git/GitHub.
New changes in branches
All changes should be done in branches and submitted by pull requests. Even in simple cases, where a review doesn't seem strictly necessary, using pull requests helps the rest of the team to notice the change and to stay up-to-date on the codebase.
- Should we enforce this by protecting the
main
branch?At the same time, each pull request should focus on a single feature. Pull requests modifying multiple aspects of the code or fixing multiple unrelated bugs are usually more difficult and thus longer to review, which doesn't align well with our agile development method.
Pull requests should target the
main
branch. We do not believe it is necessary to have an intermediatedevelop
branch. Because we follow an agile strategy, the stable version on the package corresponds to the latest CRAN release and the development version is the GitHub version.However, we discourage using branches targeting other branches as this can quickly escalate in difficult to manage conflicts. If you need to add or propose changes to a non-
main
branch, you should either use the commit suggestions feature from GitHub. In more complex cases, you should agree with the person who opened the pull request to push directly to their branches.Merge mechanism
GitHub currently has 3 pull request merge mechanisms:
The merge commit option is the only option that doesn't rewrite history. As such, it is the preferred option when multiple people commit directly to the same branch without prior concertation.
However, merge commits present other drawbacks, such as creating a heavily non-linear history, which is extremely difficult to read without a client displaying the various branches over time. Notably, it is very difficult to browse & understand a git history with merge commits in GitHub web interface.
For related reasons, merge commits can create flat out unintelligible diffs in pull requests in some cases.
The preference of squash vs simple rebase depends on the quality of the commits in the branches to merge. If the commits are grouped logically, with clear commit messages, they can be rebased & merged directly. If not, then a squash & merge should be preferred.
- Should we remove the options we choose to not use from the list of options offered by GitHub?
- Should we encourage contributors to clean & reorganise their git commit history before merging their branches? This blog post is a detailed explanation of why this can be beneficial but it is important to note that this is not also straightforward as reorganising commits can easily create conflicts if not done with care.
It is also important to note that our choice here also impacts how we should resolve diverging histories locally because the various methods are partially incompatible. For example, it is not possible to rebase & merge a branch that already contains a merge commit. Therefore, if we wish to use "Rebase & merge", we must ensure that collaborators do not create merge commits in their branches.
Avoiding merge commit when pulling
One common source of unintended merge commits is running
git pull
to fetch remote commits when you already have unpushed local commits. A simple way to solve this is to rungit pull --rebase
instead of justgit pull
. This setting can also be adjusted globally with the following command:git config --global pull.rebase true
Future option: merge queues
GitHub has released a 4th merge mechanism (currently in private beta): merge queues.
This is meant to address one major issue that agile projects encounter. If your project has multiple concurrent pull requests, as soon as you merge one of them, checks in the others become out of date because they ran against an outdated version of the
main
branch. Merge queues solve this issue by creating a queue with all the pull requests you want to merge and running checks against a temporary branch containing all the changes in the queue.This is likely the mechanism we will want to use in the future but it is currently in beta and detailed documentation is lacking.
The text was updated successfully, but these errors were encountered: