Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>,
	Phillip Wood <phillip.wood123@gmail.com>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Bryan Turner <bturner@atlassian.com>,
	Sami Boukortt <sami@boukortt.com>
Subject: Re: [PATCH v2 0/3] rebase -i: mark commits that begin empty in todo editor
Date: Fri, 10 Apr 2020 15:11:57 -0700
Message-ID: <CABPp-BECuio43GScBsPx3er60nHmrYnLMzOtibdodJW+KGFU5Q@mail.gmail.com> (raw)
In-Reply-To: <xmqqh7xrhwlu.fsf@gitster.c.googlers.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.

  reply index

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-09 23:26 [PATCH] " Elijah Newren via GitGitGadget
2020-04-10  0:50 ` Junio C Hamano
2020-04-10  2:06   ` Bryan Turner
2020-04-10  4:57     ` Junio C Hamano
2020-04-10 17:51 ` [PATCH v2 0/3] " Elijah Newren via GitGitGadget
2020-04-10 17:51   ` [PATCH v2 1/3] " Elijah Newren via GitGitGadget
2020-04-10 17:51   ` [PATCH v2 2/3] rebase: reinstate --no-keep-empty Elijah Newren via GitGitGadget
2020-04-10 20:37     ` Junio C Hamano
2020-04-10 21:41       ` Elijah Newren
2020-04-10 17:51   ` [PATCH v2 3/3] rebase: fix an incompatible-options error message Elijah Newren via GitGitGadget
2020-04-10 20:42   ` [PATCH v2 0/3] rebase -i: mark commits that begin empty in todo editor Junio C Hamano
2020-04-10 21:04     ` Junio C Hamano
2020-04-10 21:14       ` Junio C Hamano
2020-04-10 22:11         ` Elijah Newren [this message]
2020-04-10 21:29     ` Junio C Hamano
2020-04-10 22:13       ` Elijah Newren
2020-04-10 22:30         ` Junio C Hamano
2020-04-11  0:07           ` Elijah Newren
2020-04-11 21:14             ` Junio C Hamano
2020-04-11  2:44   ` [PATCH v3 0/4] " Elijah Newren via GitGitGadget
2020-04-11  2:44     ` [PATCH v3 1/4] " Elijah Newren via GitGitGadget
2020-04-15 20:52       ` Junio C Hamano
2020-04-11  2:44     ` [PATCH v3 2/4] rebase: reinstate --no-keep-empty Elijah Newren via GitGitGadget
2020-04-11  2:44     ` [PATCH v3 3/4] rebase: fix an incompatible-options error message Elijah Newren via GitGitGadget
2020-04-11  2:44     ` [PATCH v3 4/4] rebase --merge: optionally skip upstreamed commits Jonathan Tan via GitGitGadget
2020-04-11 18:12       ` Jonathan Tan
2020-04-14  9:11     ` [PATCH v3 0/4] rebase -i: mark commits that begin empty in todo editor Phillip Wood

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CABPp-BECuio43GScBsPx3er60nHmrYnLMzOtibdodJW+KGFU5Q@mail.gmail.com \
    --to=newren@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=bturner@atlassian.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=phillip.wood123@gmail.com \
    --cc=sami@boukortt.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git