Re: [PATCH v3 15/15] rebase: change the default backend from "am" to "merge"
- From
Elijah Newren <newren@gmail.com>
- Date
- Jan 11, 2020, 01:16 UTC
- Message-ID
- <CABPp-BEGwJ=0+6TMYXvMzpds0h6bz4gZA8Pi95SH9M4CKBtfOw@mail.gmail.com>
- In-Reply-To
- <20200110231436.GA24315@google.com>
Hi Jonathan,
On Fri, Jan 10, 2020 at 3:14 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 37 quoted lines
> > Hi, > > Elijah Newren via GitGitGadget wrote: > > > 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/ > > [2] https://lore.kernel.org/git/CABPp-BGiu2nVMQY_t-rnFR5GQUz_ipyEE8oDocKeO+h+t4Mn4A@mail.gmail.com/ > > [3] https://public-inbox.org/git/CABPp-BF=ev03WgODk6TMQmuNoatg2kiEe5DR__gJ0OTVqHSnfQ@mail.gmail.com/ > > [4] https://lore.kernel.org/git/CABPp-BGh7yW69QwxQb13K0HM38NKmQif3A6C6UULEKYnkEJ5vA@mail.gmail.com/ > > > > Signed-off-by: Elijah Newren <newren@gmail.com> > > --- > > 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.
Show 6 quoted lines
> 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.
Show 12 quoted lines
> 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.
Show 6 quoted lines
> 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