Re: [PATCH v3 15/15] rebase: change the default backend from "am" to "merge"

Hi Jonathan,

On Fri, Jan 10, 2020 at 3:14 PM Jonathan Nieder [off-list ref] wrote:

Hi,

Elijah Newren via GitGitGadget wrote:

quoted

The am-backend drops information and thus limits what we can do:

  * lack of full tree information from the original commits means we
    cannot do directory rename detection and warn users that they might
    want to move some of their new files that they placed in old
    directories to prevent their becoming orphaned.[1]
  * reduction in context from only having a few lines beyond those
    changed means that when context lines are non-unique we can apply
    patches incorrectly.[2]
  * lack of access to original commits means that conflict marker
    annotation has less information available.

Also, the merge/interactive backend have far more abilities, appear to
currently have a slight performance advantage[3] and have room for more
optimizations than the am backend[4] (and work is underway to take
advantage of some of those possibilities).

[1] https://lore.kernel.org/git/xmqqh8jeh1id.fsf@gitster-ct.c.googlers.com/ (local)
[2] https://lore.kernel.org/git/CABPp-BGiu2nVMQY_t-rnFR5GQUz_ipyEE8oDocKeO+h+t4Mn4A@mail.gmail.com/ (local)
[3] https://public-inbox.org/git/CABPp-BF=ev03WgODk6TMQmuNoatg2kiEe5DR__gJ0OTVqHSnfQ@mail.gmail.com/
[4] https://lore.kernel.org/git/CABPp-BGh7yW69QwxQb13K0HM38NKmQif3A6C6UULEKYnkEJ5vA@mail.gmail.com/ (local)

Signed-off-by: Elijah Newren <redacted>
---
 Documentation/git-rebase.txt           |  2 +-
 builtin/rebase.c                       |  4 ++--
 t/t5520-pull.sh                        | 10 ++++++----
 t/t9106-git-svn-commit-diff-clobber.sh |  3 ++-
 4 files changed, 11 insertions(+), 8 deletions(-)

Thanks for writing this.  We finally rolled this out to our internal
population at $DAYJOB and ran into a couple of issues:

Cool, thanks for testing it out.

 1. "git rebase --am" does not invoke the post-commit hook, but "git
    rebase --merge" does.  Is this behavior change intended?

    Noticed because jiri[1] installs a post-commit hook that warns
    about commits on detached HEAD, so this change makes rebases more
    noisy in repositories that were set up using jiri.

I've never used a post-commit hook or seen one in the wild.  Certainly
wasn't intentional, but it's not clear to me if it's wrong or right
either.  I don't see why it would make sense to distinguish between
any of git rebase --am/--merge/--interactive, but it isn't too
surprising that by historical accident the two rebase backends which
happened to call git-commit behind the scenes would call a post-commit
hook and the other rebase backend that didn't call git-commit
wouldn't.

But the big question here, is what is correct behavior?  Should rebase
call the post-commit hook, or should it skip it?  I haven't any clue
what the answer to that is.

 2. GIT_REFLOG_ACTION contains "rebase -i" even though the rebase is
    not interactive.

Yep, as does --keep, --exec, --rebase-merges, etc.  There are lots of
rebases which use the interactive machinery even if they aren't
explicitly interactive.  I've never seen the "-i" in the reflog
message defined, but clearly it has always been used whenever the
interactive machinery was in play regardless of whether the rebase was
interactive.  In that regard, I figured that --merge fit in rather
nicely.  (And I noted the fact that reflog messages were different
between the backends among the "BEHAVIORAL DIFFERENCES" section of
git-rebase.txt).  But if others think we should just drop the -i (much
as we did for the bash prompt), I'd be happy with that too.  If we go
that route, I think I'd rather drop the -i in the reflog for all
rebases, not just the
using-the-interactive-machinery-but-not-explicitly-interactive ones.

 3. In circumstances I haven't pinned down yet, we get the error
    message "invalid date format: @@2592000 +0000":

        $ git rebase --committer-date-is-author-date --onto branch_K branch_L~1 branch_L
        $ git checkout --theirs file
        $ git add file
        $ git rebase --continue
        fatal: invalid date format: @@2592000 +0000
        error: could not commit staged changes.

    This isn't reproducible without --committer-date-is-author-date.
    More context (the test where it happens) is in [2].

Interesting.  Do you happen to know if this started happening with
ra/rebase-i-more-options, or did it just become an issue with
en/rebase-backend?  I looked around at the link you provided and feel
a bit confused; I'm not sure which test does this or how I'd
reproduce.

 4. I suspect the exit status in the "you need to resolve conflicts"
    case has changed.  With rebase --am, [3] would automatically
    invoke rebase --abort when conflicts are present, but with rebase
    --merge it does not.

Known?

Nope, but I would certainly hope that "you need to resolve conflicts"
would result in a non-zero exit status.  If it doesn't, that sounds
like a bug in the interactive backend that we need to fix.  I'll dig
in.



Thanks for the reports!
Elijah