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

Hi,

On Sat, 11 Jan 2020, Phillip Wood wrote:

On 11/01/2020 01:16, Elijah Newren wrote:

quoted

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

quoted

Elijah Newren via GitGitGadget wrote:

  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.)

quoted

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.

Maybe a crazy idea, but maybe not: how about running the `post-commit`
hook _only_ if `--merge` was specified explicitly, and in that case (and
guarded behind a check verifying that the `post-commit` hook _actually_
exists _and_ is executable) warn the user that this hook won't be run in
future versions?

To make things better for users who actually want to run that hook during
rebases, we could introduce a config option, say,
`rebase.runPostCommitHook` that is a tri-state (`true`, `false`,
`onlyForDashDashMerge`, at first defaulting to the last, eventually to
`false`).

Crazy? Or helpful?

quoted

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.

I guess you're right, it is quite surprising that the `post-commit` hook
is _not_ run for `--am` rebases even though commits are created.

quoted

quoted

  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.

I agree (but won't have time to implement it, so maybe I should shut up
already...)

quoted

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.

quoted

  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/[ref]

The double `@` looks very funny. I would be interested in seeing an MCVE.

quoted

quoted

  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.

Yes, exiting with status 0 would be a major bug, and I think it might even
be a bug that was introduced by me when I re-implemented the core loop of
the interactive rebase in C.

But to me it sounds as if 4. is not so about the exit code but about
aborting immediately. I do not recall seeing --am rebases to abort,
though, but to exit with error (and I saw the same behavior in interactive
rebases).

We will need to see a reduced concrete example (preferably as a new test
case) of the described behavior.

Ciao,
Dscho