git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Michael Witten <mfwitten@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Johannes Schindelin <johannes.schindelin@gmx.de>,
	"Brian M. Carlson" <sandals@crustytoothpaste.net>,
	Pratik Karki <predatoramigo@gmail.com>,
	git@vger.kernel.org
Subject: `--rebase-merges' still failing badly
Date: Wed, 10 Oct 2018 18:51:17 -0000
Message-ID: <645452fda0ae411d86487b76aaac8151-mfwitten@gmail.com> (raw)
In-Reply-To: <xmqq8t36mk4t.fsf@gitster-ct.c.googlers.com>

On Wed, 10 Oct 2018 14:43:46 +0900, Junio wrote:

> We haven't seen  much complaints and breakages  reported against the
> two big "rewrite in C" topics  around "rebase"; perhaps it is a good
> time to merge  them to 'next' soonish  to cook them for  a few weeks
> before moving them to 'master'?

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. In short: "labels" are brittle; see below for tests.

Also, here are some quick *additional* thoughts:

    * Labels should be simply "r0", "r1", ... "rN".

          * The current, long label names are just cumbersome.
          * The embedded comments are already more than enough.
          * "r" is short for "revision" or "reset" or "remember", etc.
          * "r" is  located on a  QWERTY keyboard such that  it's very
            easy to type "rN", where "N" is a number.

    * Why is the command "label" and not "branch"? Every other related
      command looks  like a normal  git command: "reset"  and "merge".
      Make it "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

      Or, maybe git in general  should treat `-' as `ORIG_HEAD' (which
      would be similar to how `git checkout' understands `-'), thereby
      allowing a very quick idiomatic string of commands:

          pick 12345
          reset r0
          merge -

      In truth, I don't really know the semantics of `ORIG_HEAD', so
      maybe those should be nailed down and documented more clearly;
      I would like it to work as in the following:

          pick 12345
                     # label r1 (pretend)
          reset r0   # Store r1 in ORIG_HEAD
          pick 67890 # Do NOT touch ORIG_HEAD
          merge -    # Same as merge -C abcde r1

      Anyway, this  kind of unspoken  behavior would make  *writing* a
      new history by hand much more pleasant.

    * Why not just `--merges' instead of `--rebase-merges'? Or, better
      yet,  just make  it  the default  behavior;  the special  option
      should instead be:

          --flatten

      This option would simply tell `git rebase' to prepare an initial
      todo list without merges.

Thanks for this great feature.

I'm only complaining so much because it's such a useful feature, and I
want it  to be  even better, because  I'll  probably use it  A LOT; it
should have been available since the start as a natural consequence of
the way git works.

Sincerely,
Michael Witten

---------------

Unfortunately,   both  the   legacy   version  and   the  rewrite   of
`--rebase-merges'  display  a  bug  that  makes  this  feature  fairly
unusable in  practice; 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:

    :/\?.*[]

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 ` Michael Witten [this message]
2018-10-10 19:00   ` `--rebase-merges' still failing badly Michael Witten
2018-10-10 23:01   ` Junio C Hamano
2018-10-11  2:44     ` Michael Witten
2018-10-12  9:11   ` Johannes Schindelin
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=645452fda0ae411d86487b76aaac8151-mfwitten@gmail.com \
    --to=mfwitten@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=johannes.schindelin@gmx.de \
    --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