Hi Jonathan & Elijah
On 11/01/2020 01:16, Elijah Newren wrote:
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.
Perhaps that hook could learn not to warn if a branch is being rebased?
git could be more helpful there by having a porcelain option to status
that prints the branch name if we're rebasing (`git worktree --list`
shows the branch correctly when it's being rebased but does not (yet - I
have a patch to do it) mark the current worktree so isn't very helpful.)
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.
Looking through the history the am based rebase has never run the
post-commit hook as am has its own set of hooks and the scripted version
used commit-tree. The merge based rebase ran `git commit` which ran the
post commit hook. The interactive rebase ran the hook until and I broke
it in a356ee4659b ("sequencer: try to commit without forking 'git
commit'", 2017-11-24) and after I fixed it in 4627bc777e ("sequencer:
run post-commit hook", 2019-10-15). As it was broken for two years with
no one noticing it can't be that popular.
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.
It's creating a new commit so I lean towards thinking it should run the
post-commit hook. As an example I have a post-commit hook that prints a
warning if a commit is created on a branch that is being rewritten by
one of my scripts in another worktree. There are pre-commit and
pre-rebase hooks to try and prevent that, but the warning is there as a
last resort if those hooks are by-passed.
2. GIT_REFLOG_ACTION contains "rebase -i" even though the rebase is
not interactive.
If this is important to people I think it should be easy enough to set
GIT_REFLOG_ACTION to the appropriate string in builtin/rebase.c (so long
as it hasn't already been set by the user) rather than relying on
sequencer.c to do it.
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.
I'm confused by the test as well. As ra/rebase-i-more-options only
touched the sequencer then any bugs would only show up in this test
(which runs a non-interactive rebase) once en/rbease-backend switched to
that backend. It seems likely that ra/rebase-i-more-options is to blame.
Jonathan - do you happen to know if your users create empty commits at
all? and if so what do they expect rebase to do with them (and any that
become empty when they are rebased) - cf
https://lore.kernel.org/git/<CABPp-BEH=9qejeqysHYE+AJ+JPaBympZizq-bx_OjArYFa4xUQ@xxxxxxxxxxxxxx>
Best Wishes
Phillip
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
