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.

+/*
+ * The _message variables are commit names from which to take
+ * the commit message and/or authorship.
+ */

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.

+       if (whence == FROM_CHERRY_PICK && !renew_authorship) {
+               author_message = "CHERRY_PICK_HEAD";
+               author_message_buffer = read_commit_message(author_message);

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 [off-list ref]'
	...
	# 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 <redacted>
---
 builtin/commit.c |   59 ++++++++++++++++++++++++++++++++---------------------
 1 files changed, 36 insertions(+), 23 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 35eb024..f398910 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -80,6 +80,7 @@ static const char *template_file;
  * the commit message and/or authorship.
  */
 static const char *author_message, *author_message_buffer;
+static int author_message_overrides_environ;
 static char *edit_message, *use_message;
 static char *fixup_message, *squash_message;
 static int all, edit_flag, also, interactive, only, amend, signoff;

@@ -504,6 +505,38 @@ static int is_a_merge(const unsigned char *sha1)
 
 static const char sign_off_header[] = "Signed-off-by: ";
 
+static void reuse_author_info(char **name, char **email, char **date)
+{
+	const char *a, *lb, *rb, *eol;
+
+	if (author_message_overrides_environ)
+		*name = *email = *date = NULL;
+
+	a = strstr(author_message_buffer, "\nauthor ");
+	if (!a)
+		die("invalid commit: %s", author_message);
+
+	lb = strchrnul(a + strlen("\nauthor "), '<');
+	rb = strchrnul(lb, '>');
+	eol = strchrnul(rb, '\n');
+	if (!*lb || !*rb || !*eol)
+		die("invalid commit: %s", author_message);
+
+	if (!*name) {
+		if (lb == a + strlen("\nauthor "))
+			/* \nauthor <foo@example.com> */
+			*name = xcalloc(1, 1);
+		else
+			*name = xmemdupz(a + strlen("\nauthor "),
+					 (lb - strlen(" ") -
+					  (a + strlen("\nauthor "))));
+	}
+	if (!*email)
+		*email = xmemdupz(lb + strlen("<"), rb - (lb + strlen("<")));
+	if (!*date)
+		*date = xmemdupz(rb + strlen("> "), eol - (rb + strlen("> ")));
+}
+
 static void determine_author_info(struct strbuf *author_ident)
 {
 	char *name, *email, *date;

@@ -512,29 +545,8 @@ static void determine_author_info(struct strbuf *author_ident)
 	email = getenv("GIT_AUTHOR_EMAIL");
 	date = getenv("GIT_AUTHOR_DATE");
 
-	if (author_message) {
-		const char *a, *lb, *rb, *eol;
-
-		a = strstr(author_message_buffer, "\nauthor ");
-		if (!a)
-			die("invalid commit: %s", author_message);
-
-		lb = strchrnul(a + strlen("\nauthor "), '<');
-		rb = strchrnul(lb, '>');
-		eol = strchrnul(rb, '\n');
-		if (!*lb || !*rb || !*eol)
-			die("invalid commit: %s", author_message);
-
-		if (lb == a + strlen("\nauthor "))
-			/* \nauthor <foo@example.com> */
-			name = xcalloc(1, 1);
-		else
-			name = xmemdupz(a + strlen("\nauthor "),
-					(lb - strlen(" ") -
-					 (a + strlen("\nauthor "))));
-		email = xmemdupz(lb + strlen("<"), rb - (lb + strlen("<")));
-		date = xmemdupz(rb + strlen("> "), eol - (rb + strlen("> ")));
-	}
+	if (author_message)
+		reuse_author_info(&name, &email, &date);
 
 	if (force_author) {
 		const char *lb = strstr(force_author, " <");

@@ -1034,6 +1046,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
 			author_message = use_message;
 			author_message_buffer = use_message_buffer;
 		}
+		author_message_overrides_environ = 1;
 	}
 	if (whence == FROM_CHERRY_PICK && !renew_authorship) {
 		author_message = "CHERRY_PICK_HEAD";

-- 
1.7.4.1