git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Michael Witten <mfwitten@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	"Brian M. Carlson" <sandals@crustytoothpaste.net>,
	Pratik Karki <predatoramigo@gmail.com>,
	git@vger.kernel.org
Subject: Re: `--rebase-merges' still failing badly
Date: Fri, 12 Oct 2018 11:11:51 +0200 (DST)
Message-ID: <nycvar.QRO.7.76.6.1810121054050.45@tvgsbejvaqbjf.bet> (raw)
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.

>     * 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.

> 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

> 
> 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
> 

  parent reply index

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-10  5:43 What's cooking in git.git (Oct 2018, #01; Wed, 10) Junio C Hamano
2018-10-10  7:59 ` Ævar Arnfjörð Bjarmason
2018-10-10 15:06   ` Jeff King
2018-10-10 12:57 ` builtin stash/rebase, was " Johannes Schindelin
2018-10-10 13:19   ` Junio C Hamano
2018-10-11  2:18   ` Junio C Hamano
2018-10-10 13:04 ` js/mingw-wants-vista-or-above, " Johannes Schindelin
2018-10-10 13:13   ` Junio C Hamano
2018-10-10 13:58 ` Phillip Wood
2018-10-11  1:54   ` Junio C Hamano
2018-10-11 22:40   ` Junio C Hamano
2018-10-11 22:59     ` [PATCH] diff.c: die on unknown color-moved ws mode Stefan Beller
2018-10-11 23:01       ` Stefan Beller
2018-10-12  1:22       ` Junio C Hamano
2018-10-11 23:06     ` What's cooking in git.git (Oct 2018, #01; Wed, 10) Stefan Beller
2018-10-12  0:51       ` Junio C Hamano
2018-10-12  9:59     ` Phillip Wood
2018-10-12 13:36       ` Junio C Hamano
2018-10-16 13:38         ` Phillip Wood
2018-10-16 17:13           ` Stefan Beller
2018-10-10 14:18 ` Thomas Gummerer
2018-10-11  1:40   ` Junio C Hamano
2018-10-10 18:51 ` `--rebase-merges' still failing badly Michael Witten
2018-10-10 19:00   ` Michael Witten
2018-10-10 23:01   ` Junio C Hamano
2018-10-11  2:44     ` Michael Witten
2018-10-12  9:11   ` Johannes Schindelin [this message]
2018-10-10 18:55 ` What's cooking in git.git (Oct 2018, #01; Wed, 10) Stefan Beller
2018-10-11  2:00   ` Junio C Hamano
2018-10-10 20:38 ` Tim Schumacher
2018-10-10 21:25 ` Johannes Sixt
2018-10-11  1:53   ` Junio C Hamano
2018-10-11 11:16 ` Derrick Stolee
2018-10-14 12:21 ` Duy Nguyen

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=nycvar.QRO.7.76.6.1810121054050.45@tvgsbejvaqbjf.bet \
    --to=johannes.schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=mfwitten@gmail.com \
    --cc=predatoramigo@gmail.com \
    --cc=sandals@crustytoothpaste.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox