Skip to content
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

Discussion: Which branching strategy to use? #1236

Open
AesaraB opened this issue Feb 4, 2024 · 16 comments
Open

Discussion: Which branching strategy to use? #1236

AesaraB opened this issue Feb 4, 2024 · 16 comments
Labels
cat.Docs.Dev Issue relates to developer docs cat.Misc Refine into further categories at a later date type.CodeQuality Issue relates to code quality type.Enhancement Issue requests feature.
Milestone

Comments

@AesaraB
Copy link
Contributor

AesaraB commented Feb 4, 2024

Point of concern

For the longest time this repository has used the primary branch for the most bleeding edge changes, and tags for releases. I'm sure feature branches existed too.

We've recently transitioned to Git flow, but concern struck when a contributor attempted to merge into the primary branch (and presumably developed off the primary branch) instead of the development branch.

Immediately this was no problem, the branches were identical, and even if a couple files were ahead, the modified files may not be the same as, nor changed heavily enough to prevent an easy fix. However, this is an incredibly short-term way of thinking. This current branching strategy could easily create the most avoidable rebases imaginable.

Here is what we came up with.

Proposal for a branching strategy, three branches:

- Primary branch

  • Develop off of this one
  • Needs to stay stable at all times.
  • Branch Protection is enabled on this one.
    • Needs one or two reviewers to sign off before merging.
  • Tag and Release from the Primary Branch.

- Feature branches

  • Need to be stable before merging into Primary Branch
  • Rebase when Primary Branch Updates.
  • If there are breaking changes to a feature branch, recreate the branch and append a number to the end.

- Release branches

  • Main purpose is for backporting fixes or features to a specific version for compatibility sake.

- Pull Requests

  • Bugfix/hotfix can be pulled into the Primary Branch unless otherwise stated.
  • Feature Pull Requests should merged as a Feature Branch
    • This is so other contributors can work on it without having to clone another repo.
    • Especially if there are changes that need changes from Libmypaint, or mypaint-brushes repos.
      • This keeps testing way easier.

Advantage

  • Like GitLab flow (with release branches) you can work on bleeding edge features in the primary branch while limiting the scope for the release, without cluttering up your list of branches.

Disadvantage:

  • You can only work on one release at a time

Keep in mind this will need to be reflected for libmypaint as well.

gitGraph
    commit id:"initial state"
    branch release
    checkout release
    cherry-pick id: "initial state" tag: "v1.2.3-beta"
    checkout main
    branch feature-z
    checkout feature-z
    commit id:"z: added..."
    checkout main
    commit id:"merged stable feature x"
    checkout release
    cherry-pick id: "merged stable feature x"
    checkout main
    commit id:"merged experimental feature y"
    checkout feature-z
    commit id:"z: fixed..."
    commit id:"z: cleaned up..."
    checkout main
    merge feature-z id:"merged feature z"
    checkout release
    checkout main
    commit id: "updated translations"
    checkout release
    cherry-pick id: "updated translations" tag: "v1.2.3-rc-1"
    checkout main
    commit id: "hotfix..."
    checkout release
    cherry-pick id: "hotfix..." tag:"v1.2.3"
@AesaraB AesaraB added type.Enhancement Issue requests feature. type.CodeQuality Issue relates to code quality cat.Docs.Dev Issue relates to developer docs cat.Misc Refine into further categories at a later date labels Feb 4, 2024
@jtojnar
Copy link
Contributor

jtojnar commented Feb 4, 2024

Yeah, I only realized you used Git flow after the PR was merged and it did not appear on master. This is the comment in question, where I rant argue against Git flow a bit.

Since MyPaint releases patch versions, I would recommend what GitKraken calls “GitLab flow”. Similar process used e.g. by GNOME project. It does not have the main×develop distinction, which I think is unnecessary for us and needlessly complicated.

@odysseywestra
Copy link
Member

@jtojnar Honestly after thinking about it, yeah Gitlab flow is what I was looking for in developing MyPaint. I wouldn't do versioning branching unless we had a major version change and needed to keep the previous major version for backporting sake as we do now. @AesaraB I think this structure would work better for us. It'll still allow us to have feature/fix/hotfix branching and have a continuous main branch to work with and tag and release from. Plus I can still pull in Feature PRs to their own branches. Also if there are changes to both mypaint and libmypaint, testing is much easier to track if we keep the feature-branch name the same on both repos.

All-in-All thank you for bringing this up cause I think this will simplify the branching structure for all the repos too.

@AesaraB
Copy link
Contributor Author

AesaraB commented Feb 5, 2024

So let me get this straight

  • Develop in the primary branch
  • Work towards larger features in feature branches
  • Releases in tag form
  • Backported updates to releases as branches

Am I missing anything?

This to me sounds closer to a modified GitHub flow (modified i.e. tagging releases, having branches for backporting) than GitLab flow, as in the latter:

  1. Parts of the primary branch are merged into release branches, or
  2. The primary branch is a production branch.

@AesaraB
Copy link
Contributor Author

AesaraB commented Feb 5, 2024

GitLab flow could be interesting, say if we were past feature lock in a development cycle but there were still such kinds of contributions. However, this sounds like more work for the rest of a version development cycle.

@jtojnar
Copy link
Contributor

jtojnar commented Feb 5, 2024

This to me sounds closer to a modified GitHub flow (modified i.e. tagging releases, having branches for backporting) than GitLab flow, as in the latter:

  1. Parts of the primary branch are merged into release branches, or
  2. The primary branch is a production branch.

The GItLab flow is a bit confusing as it actually takes one of two forms:

  • environment branches: there is continuous stabilization with changes flowing from the development to production (possibly with more stages), useful for unversioned software like SaaS web apps.
  • release branches: one branch for each version from which we want to create point versions, there tends to be a clear cut-off point marked by a tag, often the flow of changes is limited (e.g. with SemVer).

I would be arguing for the latter.

As I understand it, the distinction between GitHub and GitLab is merely in presence of multiple (pre-)production branches with the latter.

So let me get this straight

  • Develop in the primary branch

I think that, in all of the discussed flows (Git, GitHub and GitLab), you develop in feature branches based on the development branch. This allows the team to review the change set, run CI, evolve the changes without affecting the development branch. Once the change set is deemed sufficiently reviewed and tested, it is merged into the development branch.

In GitHub and GitLab flows, the development branch is the primary branch. Feature branches can be stored in the upstream repo or in forks (necessary for external contributors on most Git forges).

  • Work towards larger features in feature branches

Not just larger ones – any change would ideally go through a feature branch so that at least CI has chance to run.

In this model, backports are variant of the feature branches, branched off and targeting one of the release branches. They will usually contain cherry-picked commits from the primary branch.

There is are also bugfix branches that are like backports but with original commits instead of cherry-picks. Those should be needed only rarely, for example, when fixing a bug in a code that no longer exists in the primary branch and was never fixed there.

  • Releases in tag form

Yes. The tags will be added to the primary branch for new major release, or to one of the release branches for minor/patch releases.

The release branch can be created immediately when a new major version is tagged (useful if we want to allow third party contributors to backport changes from development branch), or on demand once someone wants to do backporting (to avoid creating branches that would be unused).

  • Backported updates to releases as branches

Usually, we would limit backports to critical bug fixes (e.g. SemVer). If we backported everywhere, we could just use the primary branch directly and save the hassle.

@jtojnar
Copy link
Contributor

jtojnar commented Feb 5, 2024

I found a nice and detailed article that almost exactly describes what I have in mind: https://www.bitsnbites.eu/a-stable-mainline-branching-model-for-git/

It also references this great post https://graydon.livejournal.com/186550.html about the importance of testing before merges. Graydon created bors to enforce it, and nowadays it is actually supported by GitHub natively in the form of merge queues.

@AesaraB
Copy link
Contributor Author

AesaraB commented Feb 24, 2024

Proposal for a branching strategy, three branches:

  • Primary branch
    • Develop off of this one
    • Needs to stay stable at all times.
    • Branch Protection is enabled on this one.
      • Needs one or two reviewers to sign off before merging.
  • Feature branches
    • Stage commits for the primary branch in these
    • Keep short and focus if possible.
    • Rebase when Primary Branch Updates.
    • Outside Pull Requests are merged into these.
    • If there are breaking changes to a feature branch, recreate the branch and append a number to the end.
      • Delete the old feature branch when all needed contributors have synced their local repos.
  • Release branches
    • Branch off for Major Versions like 2.x.x for example.
    • Cherry Pick specific commits from the primary branch to here.
    • Create a tag when ready for distribution till the next major version change.
      • Feature freeze previous major version and only add bugfix/hotfix commits.
      • Set a limit on how long to support major versions.

Overall rule: no fast-forward commits. We need to keep commits and merges trackable.

Advantage

  • Like GitLab flow (with release branches) you can work on bleeding edge features in the primary branch while limiting the scope for the release, without cluttering up your list of branches.

Disadvantage:

  • You can only work on one release at a time

Keep in mind this will need to be reflected for libmypaint as well.

@jtojnar
Copy link
Contributor

jtojnar commented Feb 24, 2024

You can only work on one release at a time

If really needed, a feature branch can always target the “oldstable” branch. For example, GIMP still fully maintains 2.10 and they allow merge requests to the gimp-2-10 branch, if it the change is not relevant to development version.

But I would really want to avoid maintaining multiple versions at the same time beyond trivial backports, since that is tremendous amount of work.

@AesaraB AesaraB added this to the v2.1.0 milestone Feb 24, 2024
@odysseywestra
Copy link
Member

odysseywestra commented Feb 24, 2024

Okay, I added some additional info on top of @AesaraB wrote.

Probably should add branch protection rules too for release branches.

@AesaraB
Copy link
Contributor Author

AesaraB commented Feb 24, 2024

@odysseywestra I'm going to argue against a release branch for each major version because it feels like a "worst of both worlds" situation. Tags are more than adequate for demarcating major version changes, and if a backport is necessitated, we can turn a tag into a branch for that purpose.

@jtojnar
Copy link
Contributor

jtojnar commented Feb 24, 2024

  • Feature branches
    • […]
    • Outside Pull Requests are merged into these.

That would require creating a feature branch for each outside pull request, making outside contributions cumbersome.

And I do not think that is even necessary – IMO, third party forks could be considered extension of the main repo so we can just treat branches in other repos as if they were feature branches in the main repo. Forges like GitHub already allow committers to push changes to the merge request branches from forks so for this purpose, the distinction between repos does not exist.

  • If there are breaking changes to a feature branch, recreate the branch and append a number to the end.

    • Delete the old feature branch when all needed contributors have synced their local repos.

I think this is only relevant to to long-lived feature branches, off which other developers base their own feature branches. And those should generally be avoided when possible – as you say “Keep short and focus if possible.” – if only to avoid these heavy workflow considerations.

Overall rule: no fast-forward commits. We need to keep commits and merges trackable.

Non-ff merges are again useful in larger branches to group multiple commits together but not as much when feature branches are rebased onto primary branch before merging. And should not even be necessary when merging a single-commit PRs. Of course, it might be worth to consider enforcing non-ff-merges even when they cause more noise than utility, just so that the workflow is easier to remember.

Tracking backports can be achieved with using git cherry-pick with -x flag.

@AesaraB
Copy link
Contributor Author

AesaraB commented Feb 24, 2024

I think this is only relevant to to long-lived feature branches

Prime example would be #1244.

Changing the build system and directory structure has a cascading effect on much of the tooling ecosystem. So, I would argue it's better to bundle together all these PRs that improve one part of building MyPaint, only to break an existing system, until stuff stops breaking.

It's the "keep the primary branch stable" idea clashing with the "keep feature branches simple" idea.

@jtojnar
Copy link
Contributor

jtojnar commented Feb 24, 2024

Not necessarily. You could still design the individual PRs you are merging into #1244 in a way so they can be merged into master without any breakage.

But it is definitely a trade-off in that sometimes this might require a bit more work up front. In this case, moving build-aux would require also changing the CI and the existing build system, even when it would eventually be removed. But if you carefully order the merges (e.g. removing deprecated stuff like AppVeyor first), you will be able to reduce the effort a bit.

The benefit is that the individual PRs will be smaller and easier to review, which will allow them to be merged earlier, and the code base will be able to evolve faster.

Another pattern that helps with this are “feature flags”. Does not even have to be actual flags – for example, once we are reasonably satisfied with the Meson build PR, we can merge it, documenting it as experimental, while keeping the setup.py. Then we will be able to evolve them in parallel, and only removing setup.py once we are reasonably certain there are no regressions.

As I understand it, continuous deployment workflows like GitLab flow prefer this kind of trade-off. But of course, we cannot pick them blindly and need to consider if the trade is worth it in our case.

@AesaraB
Copy link
Contributor Author

AesaraB commented Feb 24, 2024

The benefit is that the individual PRs will be smaller and easier to review

Are you conflating individual PRs with merging the feature branch to the primary branch? In which case individual PRs are still discrete units reviewed independently, but a large feature branch won't play well with a very active primary branch.

I'm not sure whether or not I feel fortunate that we don't have such a primary branch, but the trade-off between a more up-to-date primary branch vs less overhead in large feature branches becomes easier to make.

Quick side note: I know you were only using this as an example, but setuptools cannot coexist with the new directory scheme without creating a new class in setup.py, which is time better spent making sure meson actually works.

@jtojnar
Copy link
Contributor

jtojnar commented Feb 24, 2024

The benefit is that the individual PRs will be smaller and easier to review

Are you conflating individual PRs with merging the feature branch to the primary branch? In which case individual PRs are still discrete units reviewed independently, but a large feature branch won't play well with a very active primary branch.

Yes, as I understand the GitHub and GitLab workflows, each feature branch corresponds to a single PR/MR (pull/merge request is a request for a review and eventual incorporation into the target branch), and these kinds of long standing branches are frowned upon.

I know you were only using this as an example, but setuptools cannot coexist with the new directory scheme without creating a new class in setup.py, which is time better spent making sure meson actually works.

Ah, I sort of hoped it would be easier. Yeah, in that case it might be not worth it.

But we could still probably find compatible changes like the AppVeyor removal and merge those to master.

@odysseywestra
Copy link
Member

odysseywestra commented Feb 26, 2024

Okay I took your feedback and added a revised branch strategy to the first post. I'll make changes as I see feedback until we are satisfy so @AesaraB can add the documentation to the new website.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat.Docs.Dev Issue relates to developer docs cat.Misc Refine into further categories at a later date type.CodeQuality Issue relates to code quality type.Enhancement Issue requests feature.
Development

No branches or pull requests

3 participants