git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines

From
PWPhillip Wood <phillip.wood@talktalk.net>
Date
Nov 4, 2018, 16:45 UTC
Message-ID
<27bcf7a6-8590-fa21-8381-697e1b030182@talktalk.net>
In-Reply-To
<20181104072253.12357-1-pclouds@gmail.com>
On 04/11/2018 07:22, Nguyễn Thái Ngọc Duy wrote:
Show 12 quoted lines
> When a commit is reverted (or cherry-picked with -x) we add an English
> sentence recording that commit id in the new commit message. Make
> these real trailer lines instead so that they are more friendly to
> parsers (especially "git interpret-trailers").
> 
> A reverted commit will have a new trailer
> 
>      Revert: <commit-id>
> 
> Similarly a cherry-picked commit with -x will have
> 
>      Cherry-Pick: <commit-id>

I think this is a good idea though I wonder if it will break someones script that is looking for the messages generated by -x at the moment. I've got a couple of comments below.

Show 55 quoted lines
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>   I think standardizing how we record commit ids in the commit message
>   is a good idea. Though to be honest this started because of my irk of
>   an English string "cherry picked from..." that cannot be translated.
>   It might as well be a computer language that happens to look like
>   English.
> 
>   Documentation/git-cherry-pick.txt |  5 ++---
>   sequencer.c                       | 20 ++++++--------------
>   t/t3510-cherry-pick-sequence.sh   | 12 ++++++------
>   t/t3511-cherry-pick-x.sh          | 26 +++++++++++++-------------
>   4 files changed, 27 insertions(+), 36 deletions(-)
> 
> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
> index d35d771fc8..8ffbaed1a0 100644
> --- a/Documentation/git-cherry-pick.txt
> +++ b/Documentation/git-cherry-pick.txt
> @@ -58,9 +58,8 @@ OPTIONS
>   	message prior to committing.
>   
>   -x::
> -	When recording the commit, append a line that says
> -	"(cherry picked from commit ...)" to the original commit
> -	message in order to indicate which commit this change was
> +	When recording the commit, append "Cherry-Pick:" trailer line
> +	recording the commit name which commit this change was
>   	cherry-picked from.  This is done only for cherry
>   	picks without conflicts.  Do not use this option if
>   	you are cherry-picking from your private branch because
> diff --git a/sequencer.c b/sequencer.c
> index 9e1ab3a2a7..f7318f2862 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -36,7 +36,6 @@
>   #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>   
>   const char sign_off_header[] = "Signed-off-by: ";
> -static const char cherry_picked_prefix[] = "(cherry picked from commit ";
>   
>   GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG")
>   
> @@ -1758,16 +1757,10 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
>   		base_label = msg.label;
>   		next = parent;
>   		next_label = msg.parent_label;
> -		strbuf_addstr(&msgbuf, "Revert \"");
> -		strbuf_addstr(&msgbuf, msg.subject);
> -		strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit ");
> -		strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));
> -
> -		if (commit->parents && commit->parents->next) {
> -			strbuf_addstr(&msgbuf, ", reversing\nchanges made to ");
> -			strbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid));
> -		}

As revert currently records the parent given on the command line when reverting a merge commit it would probably be a good idea to add that either as a separate trailer or to the Revert: trailer and possibly also generate it for cherry picks.

> -		strbuf_addstr(&msgbuf, ".\n");
> +		strbuf_addf(&msgbuf, "Revert \"%s\"\n\n", msg.subject);

If the message already contains trailers then should we just append the Revert trailer those rather than inserting "\n\n"?

Best Wishes
Phillip
> +
> +		strbuf_addf(&msgbuf, "Revert: %s\n",
> +			    oid_to_hex(&commit->object.oid));
Show 168 quoted lines
>   	} else {
>   		const char *p;
>   
> @@ -1784,9 +1777,8 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
>   			strbuf_complete_line(&msgbuf);
>   			if (!has_conforming_footer(&msgbuf, NULL, 0))
>   				strbuf_addch(&msgbuf, '\n');
> -			strbuf_addstr(&msgbuf, cherry_picked_prefix);
> -			strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));
> -			strbuf_addstr(&msgbuf, ")\n");
> +			strbuf_addf(&msgbuf, "Cherry-Pick: %s\n",
> +				    oid_to_hex(&commit->object.oid));
>   		}
>   		if (!is_fixup(command))
>   			author = get_author(msg.message);
> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
> index c84eeefdc9..89b6e7fc1e 100755
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -390,10 +390,10 @@ test_expect_success '--continue respects opts' '
>   	git cat-file commit HEAD~1 >picked_msg &&
>   	git cat-file commit HEAD~2 >unrelatedpick_msg &&
>   	git cat-file commit HEAD~3 >initial_msg &&
> -	! grep "cherry picked from" initial_msg &&
> -	grep "cherry picked from" unrelatedpick_msg &&
> -	grep "cherry picked from" picked_msg &&
> -	grep "cherry picked from" anotherpick_msg
> +	! grep "Cherry-Pick:" initial_msg &&
> +	grep "Cherry-Pick: " unrelatedpick_msg &&
> +	grep "Cherry-Pick: " picked_msg &&
> +	grep "Cherry-Pick: " anotherpick_msg
>   '
>   
>   test_expect_success '--continue of single-pick respects -x' '
> @@ -404,7 +404,7 @@ test_expect_success '--continue of single-pick respects -x' '
>   	git cherry-pick --continue &&
>   	test_path_is_missing .git/sequencer &&
>   	git cat-file commit HEAD >msg &&
> -	grep "cherry picked from" msg
> +	grep "Cherry-Pick:" msg
>   '
>   
>   test_expect_success '--continue respects -x in first commit in multi-pick' '
> @@ -416,7 +416,7 @@ test_expect_success '--continue respects -x in first commit in multi-pick' '
>   	test_path_is_missing .git/sequencer &&
>   	git cat-file commit HEAD^ >msg &&
>   	picked=$(git rev-parse --verify picked) &&
> -	grep "cherry picked from.*$picked" msg
> +	grep "Cherry-Pick: $picked" msg
>   '
>   
>   test_expect_failure '--signoff is automatically propagated to resolved conflict' '
> diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
> index 9888bf34b9..db11dd2430 100755
> --- a/t/t3511-cherry-pick-x.sh
> +++ b/t/t3511-cherry-pick-x.sh
> @@ -32,7 +32,7 @@ mesg_with_footer_sob="$mesg_with_footer
>   Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
>   
>   mesg_with_cherry_footer="$mesg_with_footer_sob
> -(cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709)
> +Cherry-Pick: da39a3ee5e6b4b0d3255bfef95601890afd80709
>   Tested-by: C.U. Thor <cuthor@example.com>"
>   
>   mesg_unclean="$mesg_one_line
> @@ -81,7 +81,7 @@ test_expect_success 'cherry-pick -x inserts blank line after one line subject' '
>   	cat <<-EOF >expect &&
>   		$mesg_one_line
>   
> -		(cherry picked from commit $sha1)
> +		Cherry-Pick: $sha1
>   	EOF
>   	git log -1 --pretty=format:%B >actual &&
>   	test_cmp expect actual
> @@ -129,7 +129,7 @@ test_expect_success 'cherry-pick -x inserts blank line when conforming footer no
>   	cat <<-EOF >expect &&
>   		$mesg_no_footer
>   
> -		(cherry picked from commit $sha1)
> +		Cherry-Pick: $sha1
>   	EOF
>   	git log -1 --pretty=format:%B >actual &&
>   	test_cmp expect actual
> @@ -154,7 +154,7 @@ test_expect_success 'cherry-pick -x -s inserts blank line when conforming footer
>   	cat <<-EOF >expect &&
>   		$mesg_no_footer
>   
> -		(cherry picked from commit $sha1)
> +		Cherry-Pick: $sha1
>   		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
>   	EOF
>   	git log -1 --pretty=format:%B >actual &&
> @@ -178,7 +178,7 @@ test_expect_success 'cherry-pick -x -s adds sob when last sob doesnt match commi
>   	git cherry-pick -x -s mesg-with-footer &&
>   	cat <<-EOF >expect &&
>   		$mesg_with_footer
> -		(cherry picked from commit $sha1)
> +		Cherry-Pick: $sha1
>   		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
>   	EOF
>   	git log -1 --pretty=format:%B >actual &&
> @@ -201,7 +201,7 @@ test_expect_success 'cherry-pick -x -s adds sob even when trailing sob exists fo
>   	git cherry-pick -x -s mesg-with-footer-sob &&
>   	cat <<-EOF >expect &&
>   		$mesg_with_footer_sob
> -		(cherry picked from commit $sha1)
> +		Cherry-Pick: $sha1
>   		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
>   	EOF
>   	git log -1 --pretty=format:%B >actual &&
> @@ -215,7 +215,7 @@ test_expect_success 'cherry-pick -x handles commits with no NL at end of message
>   	git cherry-pick -x $sha1 &&
>   	git log -1 --pretty=format:%B >actual &&
>   
> -	printf "\n(cherry picked from commit %s)\n" $sha1 >>msg &&
> +	printf "\nCherry-Pick: %s\n" $sha1 >>msg &&
>   	test_cmp msg actual
>   '
>   
> @@ -226,7 +226,7 @@ test_expect_success 'cherry-pick -x handles commits with no footer and no NL at
>   	git cherry-pick -x $sha1 &&
>   	git log -1 --pretty=format:%B >actual &&
>   
> -	printf "\n\n(cherry picked from commit %s)\n" $sha1 >>msg &&
> +	printf "\n\nCherry-Pick: %s\n" $sha1 >>msg &&
>   	test_cmp msg actual
>   '
>   
> @@ -252,19 +252,19 @@ test_expect_success 'cherry-pick -s handles commits with no footer and no NL at
>   	test_cmp msg actual
>   '
>   
> -test_expect_success 'cherry-pick -x treats "(cherry picked from..." line as part of footer' '
> +test_expect_success 'cherry-pick -x treats "Cherry-Pick:" line as part of footer' '
>   	pristine_detach initial &&
>   	sha1=$(git rev-parse mesg-with-cherry-footer^0) &&
>   	git cherry-pick -x mesg-with-cherry-footer &&
>   	cat <<-EOF >expect &&
>   		$mesg_with_cherry_footer
> -		(cherry picked from commit $sha1)
> +		Cherry-Pick: $sha1
>   	EOF
>   	git log -1 --pretty=format:%B >actual &&
>   	test_cmp expect actual
>   '
>   
> -test_expect_success 'cherry-pick -s treats "(cherry picked from..." line as part of footer' '
> +test_expect_success 'cherry-pick -s treats "Cherry-Pick:" line as part of footer' '
>   	pristine_detach initial &&
>   	git cherry-pick -s mesg-with-cherry-footer &&
>   	cat <<-EOF >expect &&
> @@ -275,13 +275,13 @@ test_expect_success 'cherry-pick -s treats "(cherry picked from..." line as part
>   	test_cmp expect actual
>   '
>   
> -test_expect_success 'cherry-pick -x -s treats "(cherry picked from..." line as part of footer' '
> +test_expect_success 'cherry-pick -x -s treats "Cherry-Pick:..." line as part of footer' '
>   	pristine_detach initial &&
>   	sha1=$(git rev-parse mesg-with-cherry-footer^0) &&
>   	git cherry-pick -x -s mesg-with-cherry-footer &&
>   	cat <<-EOF >expect &&
>   		$mesg_with_cherry_footer
> -		(cherry picked from commit $sha1)
> +		Cherry-Pick: $sha1
>   		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
>   	EOF
>   	git log -1 --pretty=format:%B >actual &&
>
Previous: Nguyễn Thái Ngọc DuyNext: Duy Nguyen
Message 2 of 25 in “sequencer.c: record revert/cherry-pick commit with trailer lines”
  1. sequencer.c: record revert/cherry-pick commit with trailer linesNguyễn Thái Ngọc Duy, Nov 4, 2018
  2. Phillip WoodNov 4, 2018
  3. Duy NguyenNov 4, 2018
  4. Phillip WoodNov 4, 2018
  5. Johannes SchindelinNov 5, 2018
  6. sequencer.c: record revert/cherry-pick commit with trailer linesNguyễn Thái Ngọc Duy, Nov 4, 2018
  7. brian m. carlsonNov 4, 2018
  8. Duy NguyenNov 5, 2018
  9. Support --append-trailer in cherry-pick and revertNguyễn Thái Ngọc Duy, Nov 6, 2018
  10. Ævar Arnfjörð BjarmasonNov 6, 2018
  11. Jeff KingNov 6, 2018
  12. Jeff KingNov 6, 2018
  13. Junio C HamanoNov 7, 2018
  14. Duy NguyenNov 7, 2018
  15. Jeff KingNov 7, 2018
  16. Junio C HamanoNov 8, 2018
  17. Jeff KingNov 8, 2018
  18. Junio C HamanoNov 8, 2018
  19. Junio C HamanoNov 7, 2018
  20. Junio C HamanoNov 5, 2018
  21. Duy NguyenNov 5, 2018
  22. Junio C HamanoNov 6, 2018
  23. Ævar Arnfjörð BjarmasonNov 6, 2018
  24. Duy NguyenNov 6, 2018
  25. Ævar Arnfjörð BjarmasonNov 6, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.