Re: [PATCH v2 0/3] rebase -i: mark commits that begin empty in todo editor

[Elijah Newren <newren@gmail.com>]

On Fri, Apr 10, 2020 at 2:14 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > This is a total tangent, but when I tried to rebase
> > jt/rebase-allow-duplicate that builds directly on top of v2.25.0 to
> > a newer base, after resolving conflicts, "commit -a" and "rebase
> > --continue", somewhere I seem to have mangled the authorship.  It
> > could entirely be a driver error, or it may be a bug in "rebase",
> > especially with recent backend change.  I am planning to come back
> > to it later to figure out if there is such a bug, but I'd need to
> > recover from the authorship screwup first, so it may take some
> > time.
>
> I think I know how it happens now.
>
> 52e8d1942c662 == jt/rebase-allow-duplicate
>
> Attempting to rebase this on top of the 'master', i.e.
>
>     $ git rebase --onto master 52e8d1942c662^ 52e8d1942c662
>
> results in a merge conflict, but it is easy to resolve in the
> working tree.  Then after "git add -u" to record the resolution
> in the index
>
>     $ git commit
>     $ git rebase --continue
>
> gives me the authorship credit.
>
> Back when the default rebase backend was 'apply', making the above
> "mistake" was not even possible; "git commit" would have given me an
> empty log buffer to edit, without pre-filling anything, to help me
> realize that I shouldn't commit.

"was not even possible" and "to help me realize that I shouldn't
commit" seem slightly at odds, but I think you're saying that the UI
was a bit better at helping you abort before you continued with such
an accident.  Perhaps it could be improved even more while we're here?

> With the sequencer backend, however, the above procedure happily
> records the commit under the author's name, it seems.
>
> I am not sure if that is a bug.  I am inclined to say so.

I am inclined to say so too, but it does get a bit more
complicated...with just a few cases I can think of off the top of my
head.

In the case of the merge backend, specifically with --interactive, it
does make sense to use "break" or "edit" commands and then allow the
user to run "git commit" in a stopped rebase.

It also could make sense in some cases to hit a conflict, allow the
user to run "git reset HEAD" and then start fixing and staging pieces
of the changes for a commit and then manually commit the pieces (which
then gain a different author and different commit message and
rearranged/subsetted/modified contents, with the expectation that
they'd probably do a Patched-based-on-work-by tag or equivalent).

But if we're in the middle of a rebase-apply, or in the middle of a
rebase-merge and resolving a conflict (i.e. not cases like the "break"
or "edit" or just ran "git reset HEAD" cases), or in the middle of a
cherry-pick and resolving a conflict, then I'm inclined to agree with
you that calling "git commit" represents an accident by the user that
should result in an error.  For my purposes here, "git add -u"
shouldn't mean we're no longer "busy resolving a conflict" (otherwise
git commit would have already stopped you); that state shouldn't go
away until "rebase --continue" or "cherry-pick --continue" or "git
reset HEAD" is executed.  But it gets slightly weird, though, since
"git reset HEAD" operates differently in merge and apply modes (with
apply still attempting to commit something afterwards and merge
realizing that a reset means that commit was tossed).  So it might
take a little more care and history digging to make sure that the
various cases are handled sanely.