[sane-devel] Proposal to disallow direct pushes to the master branch

[Olaf Meeuwissen]

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