Re: `--rebase-merges' still failing badly

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Oct 12, 2018, 09:11 UTC
Message-ID: <nycvar.QRO.7.76.6.1810121054050.45@tvgsbejvaqbjf.bet>
In-Reply-To: <645452fda0ae411d86487b76aaac8151-mfwitten@gmail.com>

Hi Michael,

On Wed, 10 Oct 2018, Michael Witten wrote:

> In my opinion, the `--rebase-merges' feature has been broken since the
> beginning, and the builtin version should  be fixed before it is moved
> ahead.

Everybody is entitled to an opinion. My opinion differs from yours, and I am a heavy user of `git rebase -kir`.

The `--rebase-merges` feature is not without problems, of course. I can name a couple of bugs, but I have a hunch that it is more efficient for me to just fix them.

> In short: "labels" are brittle; see below for tests.

Sure, let's improve them.

> Also, here are some quick *additional* thoughts:
> 
>     * Labels should be simply "r0", "r1", ... "rN".

That would not be an improvement.

The *interactive* version of `--rebase-merges` is what I use extensively to juggle Git for Windows' branch thicket. It would be really bad if I had to somehow map those label names in my head, rather than having the intuitively-understood labels.

I would understand if you suggested to try to come up with a better naming than `branch-point-<n>`. But `r<n>`? That's worse than the current state. By a lot.

>     * Why is the command "label" and not "branch"?

Because it is more versatile than just a branch. It is also branch points. As a matter of fact, the very first statement is about the `onto` label, which is not a branch.

Show 12 quoted lines

>     * In my experience, there's a lot of this boiler plate:
> 
>           pick 12345
>           label r1
>           reset r0
>           merge r1
> 
>       How about instead, use git's existing ideas:
> 
>           pick 12345
>           reset r0
>           merge ORIG_HEAD

Too magic. And you cannot change it easily. I had this very real example, a couple of times yesterday: A merge was in one of the "branches", and needed to be moved out of it:

	pick abc
	label branch-point
	merge -C 0123 topic
	pick def
	label bug-fix

	reset branch-point
	merge -C 4567 bug-fix

This `merge -C 0123 topic` needed to be moved before the branch point.

Another example where the explicit labeling comes in *real* handy is when I made a Pull Request in Git for Windows ready for contribution to core Git. These Pull Requests are normally based on `master`, because that is what the best PR flow is: you based your contributions as close to the tip as possible, to avoid merge conflicts (and to test as close to the real, after-merge thing). This would look like this:

	label branch-point
	pick 123
	pick 456
	label pr-0815

	reset branch-point
	merge -C abc pr-0815

Now, to prepare this for core Git, I have to graft this PR onto the `master` of *upstream*, in our case I would use the `onto` label for that, by inserting a `reset onto` just before `pick 123`.

So you see, the current, non-implicit, but very much explicit syntax, makes all of these tasks *quite* easy, and more importantly, straight-forward: I did not have to explain this to anyone who I needed to teach how this works.

Remember: the syntax of the todo list is not optimized to be short. It is
optimized to be *editable*. I need to have a very easy way to juggle
criss-cross-merging branch thickets. And the current syntax, while
chattier than you would like, does the job. Pretty well, even.

>     * Why not just `--merges' instead of `--rebase-merges'?

This ship has sailed. It is pointless to discuss this now.

Besides, I believe that in your quest to shorten things, you unfortunately shortened things too much: it is no longer clear what "merges" means in the context of `--merges`.

> Unfortunately,   both  the   legacy   version  and   the  rewrite   of
> `--rebase-merges'  display  a  bug  that  makes  this  feature  fairly
> unusable in  practice;

You will be surprised just how much I would embrace bug fixes, once you provide any.

Show 6 quoted lines

> it tries  to create  a "label" (i.e.,  a branch name) from a commit log
> summary  line, and the result is often invalid (or just  plain
> irritating to work  with). In particular, it  fails on typical
> characters, including at least these:
> 
>     :/\?.*[]

And of course those are not the only ones. The trick is to reduce runs of disallowed characters to dashes, as is already done with spaces.

Ciao, Johannes

Show 136 quoted lines

> 
> To see this, first define some POSIX shell functions:
> 
>     test()
>     {
>         (
>             set -e
>             summary=$1
>             d=/tmp/repo ##### WARNING. CHANGE IF NECESSARY.
>             rm -rf "$d"; mkdir -p "$d"; cd "$d"
>             git init -q
>             echo a > a; git add a; git commit -q -m a
>             git branch base
>             echo b > b; git add b; git commit -q -m b
>             git reset -q --hard HEAD^
>             git merge -q --no-ff -m "$summary" ORIG_HEAD
>             git log --graph --oneline
>             git rebase --rebase-merges base
>         ); status=$?
>         echo
>         return "$status"
>     }
> 
>     Test()
>     {
>         if test "$@" 1>/dev/null 2>&1; then
>             echo '    good'; return 0
>         else
>             echo '    fail'; return 1
>         fi
>     }
> 
> Then, try various commit summaries (see below for results):
> 
>     test c
>     test 'combine these into a merge: a and b'
>     Test ab:
>     Test a:b
>     Test :
>     Test a/b
>     Test 'Now supports /regex/'
>     Test ab/
>     Test /ab
>     Test /
>     Test 'a\b'
>     Test '\'
>     Test 'Maybe this works?'
>     Test '?'
>     Test 'This does not work.'
>     Test 'This works. Strange!'
>     Test .git
>     Test .
>     Test 'Cast each pointer to *void'
>     Test '*'
>     Test 'return a[1] not a[0]'
>     Test '[ does not work'
>     Test '['
>     Test '] does work'
>     Test ']'
> 
> Here are the results of pasting the above commands into my terminal:
> 
>     $ test c
>     warning: templates not found in ../install/share/git-core/templates
>     *   1992d07 (HEAD -> master) c
>     |\
>     | * 34555b5 b
>     |/
>     * 338db9b (base) a
>     Successfully rebased and updated refs/heads/master.
> 
>     $ test 'combine these into a merge: a and b'
>     warning: templates not found in ../install/share/git-core/templates
>     *   4202c49 (HEAD -> master) combine these into a merge: a and b
>     |\
>     | * 34555b5 b
>     |/
>     * 338db9b (base) a
>     error: refusing to update ref with bad name 'refs/rewritten/combine-these-into-a-merge:-a-and-b'
>     hint: Could not execute the todo command
>     hint:
>     hint:     label combine-these-into-a-merge:-a-and-b
>     hint:
>     hint: It has been rescheduled; To edit the command before continuing, please
>     hint: edit the todo list first:
>     hint:
>     hint:     git rebase --edit-todo
>     hint:     git rebase --continue
> 
>     $ Test ab:
>         fail
>     $ Test a:b
>         fail
>     $ Test :
>         fail
>     $ Test a/b
>         good
>     $ Test 'Now supports /regex/'
>         fail
>     $ Test ab/
>         fail
>     $ Test /ab
>         fail
>     $ Test /
>         fail
>     $ Test 'a\b'
>         fail
>     $ Test '\'
>         fail
>     $ Test 'Maybe this works?'
>         fail
>     $ Test '?'
>         fail
>     $ Test 'This does not work.'
>         fail
>     $ Test 'This works. Strange!'
>         good
>     $ Test .git
>         fail
>     $ Test .
>         fail
>     $ Test 'Cast each pointer to *void'
>         fail
>     $ Test '*'
>         fail
>     $ Test 'return a[1] not a[0]'
>         fail
>     $ Test '[ does not work'
>         fail
>     $ Test '['
>         fail
>     $ Test '] does work'
>         good
>     $ Test ']'
>         good
>