Re: [PATCH] Teach commit about CHERRY_PICK_HEAD

Jay Soffian wrote:

After scratching my head a little, this seems to mean

	After conflict resolution, your last cherry-pick does not
	introduce any changes. To commit it as an empty commit, use:\n
	\n
	    git commit --allow-empty\n
	\n
	Alternatively, you can cancel the cherry-pick by removing\n
	the file %s\n

It suspect it might be more intuitive to say

	Alternatively, you can cancel the cherry-pick by running\n
	"git reset".\n

which works because the index is known to be clean at that point.

Makes sense, thanks. I'll think more about an intuitive and consistent name for these and hopefully send a patch for it later as a separate topic.

I mentioned in a side thread that script authors might be happier if their carefully prepared GIT_AUTHOR_NAME and GIT_AUTHOR_EMAIL do not get overridden in this case, but I was wrong[*]. Might make sense to add a test for that:

	GIT_AUTHOR_NAME="Someone else" &&
	GIT_AUTHOR_EMAIL=someone@else.example.com &&
	export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL &&
	git cherry-pick something &&

	git diff-tree -s --format="%an <%ae>" >actual &&
	test_cmp expect actual

(This is just a side note; the patch is good.)

In the $GIT_CHERRY_PICK_HELP set case, won't the CHERRY_PICK_HEAD behavior be harmless? rebase --interactive --continue wants to set GIT_AUTHOR_NAME and GIT_AUTHOR_EMAIL but that is only in order to copy the author identity from the original commit.

The advice 'use "git reset HEAD -- <file>..." to unstage' makes perfect sense to me in the cherry-pick case, no? The operator is replaying an existing commit, and tweaking the result amounts to pretending she made the change herself and tweaking that.

Likewise.

I think these comments are not in a place that will help a person understand the code, but I don't have the energy to go back and forth on it.

Everything not mentioned above except the --no-commit case mentioned by Junio looks good to me, though I haven't tested.

Thanks for your thoughtfulness. Jonathan

[*]
-- 8< --
Subject: [BAD IDEA] commit: let GIT_AUTHOR_NAME/EMAIL take effect when commiting a cherry-pick

commit -c/-C/--amend to take the commit message and author from another message has always overridden the GIT_AUTHOR_NAME and GIT_AUTHOR_EMAIL variables. Letting the command line override the environment in this way is generally convenient and more intuitive if a person has forgotten what is in the environment:

	GIT_AUTHOR_NAME=me
	GIT_AUTHOR_EMAIL=email@example.com
	export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL

	...
	git commit;	# using identity from environment
	...
	git commit;	# another commit as me
	...
	# commit by someone else
	git commit --author='Someone Else <other@example.com>'
	...
	# commit by someone else
	git commit -c someone-elses-commit

The new CHERRY_PICK_HEAD feature inherits the same semantics. Uncareful scripts that want to commit with a different author name and email when taking over after a failed cherry-pick use authorship from the cherry-picked commit instead of the environment:

	! git cherry-pick complicated;	# failed cherry-pick
	... resolve the conflict ...
	git commit

While that could theoretically break some scripts, it's worth it for consistency, the intuitiveness-when-user-forgets-environment issue mentioned above, and because cherry-pick itself, which internally does something like

	git cherry-pick --no-commit $1;	# simple cherry-pick
	git commit

has always ignored the GIT_AUTHOR_NAME/EMAIL environment settings.

The patch below makes commit with CHERRY_PICK_HEAD respect GIT_AUTHOR_NAME/EMAIL anyway, to demonstrate how broken that is. It breaks tests t3404 and t3506.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/commit.c |   59 ++++++++++++++++++++++++++++++++---------------------
 1 files changed, 36 insertions(+), 23 deletions(-)