Hi Jonathan, On Fri, Jan 10, 2020 at 3:14 PM Jonathan Nieder <jrnieder@xxxxxxxxx> wrote: > > 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@xxxxxxxxxxxxxxxxxxxxxxxxx/ > > [2] https://lore.kernel.org/git/CABPp-BGiu2nVMQY_t-rnFR5GQUz_ipyEE8oDocKeO+h+t4Mn4A@xxxxxxxxxxxxxx/ > > [3] https://public-inbox.org/git/CABPp-BF=ev03WgODk6TMQmuNoatg2kiEe5DR__gJ0OTVqHSnfQ@xxxxxxxxxxxxxx/ > > [4] https://lore.kernel.org/git/CABPp-BGh7yW69QwxQb13K0HM38NKmQif3A6C6UULEKYnkEJ5vA@xxxxxxxxxxxxxx/ > > > > Signed-off-by: Elijah Newren <newren@xxxxxxxxx> > > --- > > 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
