On Fri, Apr 10, 2020 at 2:14 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Junio C Hamano <gitster@xxxxxxxxx> 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.
