Hi Povilas, # I've been mulling this over for a bit, hence the belated reply. # Please note that there is actually an [issue][0] on this topic as well. # # [0]: https://gitlab.com/sane-project/ops/issues/5 # # @llagendijk> Thanks for reminding me.
Povilas Kanapickas writes: > Hi, > > Currently there are two ways to submit code to the sane-project/backends > repository: directly pushing to master and creating a merge request on > GitLab. I'd like to propose we only allow the use of the latter option. For clarity, this proposal only applies to sane-project/backends members with "push-to-master" privileges. As of writing that is all the folks with Developer, Maintainer or Owner privileges (17 out of 18 members). > Direct pushes have the risk of accidentally breaking the build or tests. So do merges via GitLab merge requests: in the case that one (or more) (merge) commit(s) land on master after the start of your branch. There was a relatively recent [GitLab blog post][1] on this and how you might prevent that kind of breakage (requires a GitLab Premium subscription). [1]: https://about.gitlab.com/blog/2019/09/11/how-to-avoid-broken-master-with-pipelines-for-merge-requests/ > Our CI setup tests builds on 5 environments, I doubt that everyone would > test all configurations that are automatically covered by the CI for > each code submission. Personally, going through merge requests was > really worth the 20 seconds that it takes to open one, as numerous > issues have been found on GitLab CI. # I've been getting two notifications for every single one of those :-) # First one for creation of the request, second one for the merge. As for locally testing all configurations that are covered by CI, the whole point of CI is that you don't have to. CI is supposed to give quick feedback in case of trouble so you can fix it promptly while you're still fresh on the changes. # Our CI pipelines usually take around 15 minutes to complete at the # moment. > Additionally, merge requests on GitLab always use master branch as the > first parent. This is important, because it allows various tooling to > use the `--first-parent` flag to iterate through merges to the master > branch ignoring the commits. As a result it's possible to e.g. bisect > some issue picking only the merges on the master branch which makes the > process much more efficient, as intermediate commits on side branches > don't even build a lot of the time. As it stands now, it would only take > a single merge of the current master branch to an old enough commit to > prevent such tooling working altogether. There have already been several > occasions of such reverse merges happening, fortunately they used not > too old commits as the first parent. I wasn't familiar with the --first-parent flag so did some homework. I think I now understand your use-case/desire for disallowing direct pushes better. Without the use of that flag, a bisect on the changes after 1.0.28 would have to consider 672 commits whereas as with use of the flag only 144 (as of 2b1ce918aef19061bf5f0cc3cba625314bb6f1c3), roughly halving the number of commits that need to be checked. It also cuts out a large number of commits that may or may not build to begin with as they were interim commits on a "topic" branch, reducing the chance of git bisect identifying a commit that has nothing to do with what you're investigating. # I should add that `git bisect` does not support this flag directly so # you would have to deal with that in some way yourself. In addition, # there are other ways to speed up bisection, such as specifying the # files you're interested in and configuring your build with a BACKENDS # environment variable set to the ones of interest. However, this --first-parent argument is only applicable to multi-commit topic branches, merged with the --no-ff option. On GitLab that's how merges are configured (by default, IIRC) but note that git's default is for --ff merges. For single commit branches, the advantage is limited to CI feedback. And then it only really makes a difference in case the CI pipeline fails and that single commit branch becomes a multi-commit one when you push a fix. That is, for a merge commit sequence like below the advantage of forcing all commits to master via a GitLab merge is close to zero. master -o---o---o---o---o-- \ / \ / \ / \ / branches o o o o Even if any of these introduced a bug (that CI didn't catch), I think discussion would start in an issue (or on the mailing list). I don't really see it start in the GitLab merge request. That said, I do realize that there is merit in going the merge request way if you're concerned about the possibility of breaking the build on master. > What do you think about this? So while I certainly agree that using GitLab merge requests is a good idea (and should be encouraged), I am not completely convinced that it should be made the *only* way to push code changes. I still think that a *single* commit for which you'd normally want to skip CI is OK to push directly to master. I say "normally" because changes to any of the *.desc files should trigger CI so that the list of supported devices by the master branch gets updated on the website in a timely fashion. # BTW, you can add [ci skip] or [skip ci] marker to your commit message # to prevent triggering a build. However, any sequence of related commits had best go via a GitLab merge request (or local branch with a --no-ff commit to master). Just pushing that sequence out onto master (as would happen when you develop directly on master or push a local fast-forwarded merge!) is something that is probably better avoided for the reasons discussed above. Hope this helps, -- Olaf Meeuwissen, LPIC-2 FSF Associate Member since 2004-01-27 GnuPG key: F84A2DD9/B3C0 2F47 EA19 64F4 9F13 F43E B8A4 A88A F84A 2DD9 Support Free Software https://my.fsf.org/donate Join the Free Software Foundation https://my.fsf.org/join
