git commit hook to prevent changes on left of merge

[p5pRT]

Migrated from rt.perl.org#116965 (status was 'open')

Searchable as RT116965$

[p5pRT]

From @rjbs

In the perl.git repo, we usually want merge commits to introduce changes only
on the right. That is, this is okay​:

  * merge foo branch
  | \
  | * work on perl foo
  | * more work on perl foo
  | /
  * previous unrelated work

...but this is not​:

  * merge bar branch
  | \
  | * work on perl bar
  | * more work on perl bar
  * | work on blead
  | /
  * previous unrelated work

The second example should be rebased on blead, and then either merged or not
merged.

Changes on the left of the merge should only occur quite rarely, for example
when merging a release branch back to blead, when blead has advanced during the
release process.

We should prevent merges like this unless --force is given. The warning should
explain what is wrong, and how the user should proceed.

--
rjbs

[p5pRT]

From @jkeenan

On Wed Feb 27 06​:20​:36 2013, rjbs wrote​:

In the perl.git repo, we usually want merge commits to introduce
changes only
on the right. That is, this is okay​:

* merge foo branch
| \
| * work on perl foo
| * more work on perl foo
| /
* previous unrelated work

...but this is not​:

* merge bar branch
| \
| * work on perl bar
| * more work on perl bar
* | work on blead
| /
* previous unrelated work

The second example should be rebased on blead, and then either merged
or not
merged.

Changes on the left of the merge should only occur quite rarely, for
example
when merging a release branch back to blead, when blead has advanced
during the
release process.

We should prevent merges like this unless --force is given. The
warning should
explain what is wrong, and how the user should proceed.

RJBS​: This ticket hasn't received any responses since you filed it two-and-a-half years ago.

That suggests to me that the suggested functionality isn't really needed. Indeed, when I used 'git graph' to scroll through our release history, the only commits which seemed to look like your "not okay" example were precisely the sort of commits which you said were the exceptions to the rule (release-related).

Is this ticket closable?

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

[p5pRT]

The RT System itself - Status changed from 'new' to 'open'

[p5pRT]

From @ap

* James E Keenan via RT <perlbug-followup@​perl.org> [2015-08-02 03​:15]​:

Indeed, when I used 'git graph' to scroll through our release history,
the only commits which seemed to look like your "not okay" example
were precisely the sort of commits which you said were the exceptions
to the rule (release-related).

Absence of evidence is not evidence of absence. Roughly a year after the
ticket, there was an accident that this hook might have prevented, whose
record has been removed from history.

That was, however, the only incident.

[p5pRT]

From @rjbs

* James E Keenan via RT <perlbug-followup@​perl.org> [2015-08-01T21​:13​:35]

RJBS​: This ticket hasn't received any responses since you filed it
two-and-a-half years ago.

Disappointing, isn't it? Well, I tweeted about our desire for a hook here, so
I'm sure we'll have some code any minute now. Right?

https://twitter.com/rjbs/status/692841313981444096

Anyway, I remain in favor of keeping this open, even a it garners no reaction.
I am accepting that I'm not likely to write this hook any time soon, though, as
I had once thought I would.

--
rjbs

[p5pRT]

From granny-cpan@ofb.net

Saw the tweet.

If you don't want a merge commit​:

  git checkout master
  git merge --ff-only feature-branch-name

But I assume you want a merge commit (as per your ascii art)

Annoyingly, '--ff-only' and '--no-ff' can't both be specified. As we can see by the documentation for merge.ff option.

   merge\.ff
      By default\, Git does not create an extra merge commit when merging a commit that is a descendant of the current commit\. Instead\,
     the tip of the current branch is fast\-forwarded\. When set to false\, this variable tells Git to create an extra merge commit in
      such a case \(equivalent to giving the \-\-no\-ff option from the command line\)\. When set to only\, only such fast\-forward merges are
      allowed \(equivalent to giving the \-\-ff\-only option from the command line\)\.

There isn't a direct hook for per-merge. You could try and use prepare-commit-msg, but that doesn't know the branches involved in the merge. MERGE_HEAD doesn't show up until after perpare-commit-msg is done running.

I've written a little helper ff-merge-check.sh that will exit non-zero if a ff merge would fail. You could manually run this before the merge. Would this help?

  git checkout master
  ff-merge-check.sh $feature_branch && git merge --no-ff $feature_branch

https://gist.github.com/spazm/02b62a48d2d2771d8311

[p5pRT]

From @robn

Experimenting with this right now.

There's no easy support for looking down a particular side of a merge commit, but we can do it in stages​: for each merge commit, get merge base and list of parents. For each parent, find commits between the merge base and the parent. Then it's just counting - in a regular merge, the first (left) branch must have zero commits.

Another approach would be to filter the output of `git log --graph --oneline [--stuff]` and look for graph segments with '*' to the left of '|'. It's sort of easier, but feels fragile. I'd need to play a lot more before I felt confident with it.

Question​: does this need to handle merge commits with more than two parents (eg octopus)? If we generalise it as there should be nothing at all on the leftmost branch, regardless of how many parents are in the merge, then I think it would work for any merge strategy. But if you never want it, maybe it's easier just to reject merges with more than two parents.

I'll try to post some actual code soon.

[p5pRT]

From @robn

First attempt attached. Parses the output of --graph, which actually seems good enough the more I think about it.

There's a massive problem with this - --force doesn't work.

The --force switch to git push is resolved on the client. By default, git runs the equivalent of "git rev-list $oldrev ^$newrev" to determine if $newrev is ahead of $oldrev (that command returns commits reachable from $oldrev that aren't in $newrev). --force disables that test.

The problem is, --force is not received by server hooks. All you get is the branch name and the previous and new refs. There's nothing to tell you whether you want to run this check. The only ways you have is to leave hints in other places in the repo.

I've seen the commit message used for this; you could put a special tag string in the merge commit comment to tell it to ignore the check. Maybe it's not so bad if it's truly an unusual situation? Of course, that does mean this shorthand method using --graph won't work, since the hook will now need to handle each merge in the push separately.

So that's about as far as I can do without dictating workflow policy, which I know absolutely nothing about with respect to the Perl project. But if anyone wants to give some guidance, do holler :)

[p5pRT]

From @robn

update

[p5pRT]

From dennis@kaarsemaker.net

RJBS requests​:

We should prevent merges like this

This is possible as an update hook. Quite easy even, make sure that for
each merge a push introduces, commit^1 is a parent of commit^2. As a
commit hook it's probably even easier (git merge-base --is-ancestor
HEAD MERGE_HEAD)

unless --force is given. 

But this is impossible in an update hook I believe. Update hooks do not
see that --force was given. For commit hooks --no-verify can be used,
but commit hooks are easy to forget in fresh clones, so I'd prefer
something that runs when pushing. 

--
Dennis Kaarsemaker
www.kaarsemaker.net

[p5pRT]

From @rjbs

* Dennis Kaarsemaker <dennis@​kaarsemaker.net> [2016-01-29T15​:35​:33]

unless --force is given. 

But this is impossible in an update hook I believe. Update hooks do not
see that --force was given. For commit hooks --no-verify can be used,
but commit hooks are easy to forget in fresh clones, so I'd prefer
something that runs when pushing. 

Yeah, commit hooks are no good here for exactly the reason you suggest.

I think that --force is something we can live without. Right?

That is​: why would we need to be able to have this kind of merge? I suspect
never. If it turns out we do, a committers can remove or temporarily disable
the hook.

So, may as well move foward. Rob N. supplied a likely-looking hook. spazm's
work also looks plausible, although I think it would need some tweaking to not
worry about HEAD.

Anybody else?

--
rjbs

[p5pRT]

From @tonycoz

On Sun, Jan 31, 2016 at 10​:38​:01PM -0500, Ricardo Signes wrote​:

* Dennis Kaarsemaker <dennis@​kaarsemaker.net> [2016-01-29T15​:35​:33]

unless --force is given. 

But this is impossible in an update hook I believe. Update hooks do not
see that --force was given. For commit hooks --no-verify can be used,
but commit hooks are easy to forget in fresh clones, so I'd prefer
something that runs when pushing. 

Yeah, commit hooks are no good here for exactly the reason you suggest.

I think that --force is something we can live without. Right?

That is​: why would we need to be able to have this kind of merge? I suspect
never. If it turns out we do, a committers can remove or temporarily disable
the hook.

Merges with commits on both sides are pretty common for merging
release branches.

See around v5.23.5, v5.21.7, v5.19.6 and v5.19.4 for example.

Tony

[p5pRT]

From @jimc

On Sun, Jan 31, 2016 at 8​:38 PM, Ricardo Signes <perl.p5p@​rjbs.manxome.org>
wrote​:

* Dennis Kaarsemaker <dennis@​kaarsemaker.net> [2016-01-29T15​:35​:33]

unless --force is given.

But this is impossible in an update hook I believe. Update hooks do not
see that --force was given. For commit hooks --no-verify can be used,
but commit hooks are easy to forget in fresh clones, so I'd prefer
something that runs when pushing.

Yeah, commit hooks are no good here for exactly the reason you suggest.

I think that --force is something we can live without. Right?

That is​: why would we need to be able to have this kind of merge? I
suspect
never. If it turns out we do, a committers can remove or temporarily
disable
the hook.

So, may as well move foward. Rob N. supplied a likely-looking hook.
spazm's
work also looks plausible, although I think it would need some tweaking to
not
worry about HEAD.

Anybody else?

I believe blead is currently lacking a

  make regen && commit -m fixup-after-3879c54d uconfig.h

whatever commit-hook is devised,
catching need for make regen would seem a good use-case.

is it time for make regen to be a dependency of all ?
a commit hook seems slightly less intrusive.

--

rjbs