Hi,
On Sat, 11 Jan 2020, Phillip Wood wrote:
> On 11/01/2020 01:16, Elijah Newren wrote:
> >
> > On Fri, Jan 10, 2020 at 3:14 PM Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> > >
> > > 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.)
>
> > 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?
> > 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.
> > > 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...)
> > 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>
The double `@` looks very funny. I would be interested in seeing an MCVE.
> > > 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
