Re: [PATCH] sequencer: honor GIT_REFLOG_ACTION

From: Phillip Wood <phillip.wood123@gmail.com>
To: Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Cc: jrnieder@gmail.com, elbrus@debian.org,
	ijackson@chiark.greenend.org.uk, phillip.wood@dunelm.org.uk,
	alban.gruin@gmail.com, Johannes.Schindelin@gmx.de,
	Elijah Newren <newren@gmail.com>
Subject: Re: [PATCH] sequencer: honor GIT_REFLOG_ACTION
Date: Thu, 2 Apr 2020 10:25:29 +0100	[thread overview]
Message-ID: <b187cb5f-a6c8-2908-e3fd-e1210e6970e0@gmail.com> (raw)
In-Reply-To: <pull.746.git.git.1585773096145.gitgitgadget@gmail.com>

Hi Elijah

Thanks for fixing this

On 01/04/2020 21:31, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> There is a lot of code to honor GIT_REFLOG_ACTION throughout git,
> including some in sequencer.c; unfortunately, reflog_message() and its
> callers ignored it.  Instruct reflog_message() to check the existing
> environment variable, and use it when present as an override to
> action_name().
> 
> Also restructure pick_commits() to only temporarily modify
> GIT_REFLOG_ACTION for a short duration and then restore the old value,
> so that when we do this setting within a loop we do not keep adding "
> (pick)" substrings and end up with a reflog message of the form
>      rebase (pick) (pick) (pick) (finish): returning to refs/heads/master
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>      sequencer: honor GIT_REFLOG_ACTION
>      
>      I'm not the best with getenv/setenv. The xstrdup() wrapping is
>      apparently necessary on mac and bsd. The xstrdup seems like it leaves us
>      with a memory leak, but since setenv(3) says to not alter or free it, I
>      think it's right. Anyone have any alternative suggestions?
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-746%2Fnewren%2Fhonor-reflog-action-in-sequencer-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-746/newren/honor-reflog-action-in-sequencer-v1
> Pull-Request: https://github.com/git/git/pull/746
> 
>   sequencer.c               |  9 +++++++--
>   t/t3406-rebase-message.sh | 16 ++++++++--------
>   2 files changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index e528225e787..5837fdaabbe 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3708,10 +3708,11 @@ static const char *reflog_message(struct replay_opts *opts,
>   {
>   	va_list ap;
>   	static struct strbuf buf = STRBUF_INIT;
> +	char *reflog_action = getenv("GIT_REFLOG_ACTION");

Minor nit - you're using a string here rather that the pre-processor 
constant that is used below

>   	va_start(ap, fmt);
>   	strbuf_reset(&buf);
> -	strbuf_addstr(&buf, action_name(opts));
> +	strbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));
>   	if (sub_action)
>   		strbuf_addf(&buf, " (%s)", sub_action);
>   	if (fmt) {
> @@ -3799,8 +3800,10 @@ static int pick_commits(struct repository *r,
>   			struct replay_opts *opts)
>   {
>   	int res = 0, reschedule = 0;
> +	char *prev_reflog_action;
>   
>   	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
> +	prev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));

I'm confused as to why saving the environment variable immediately after 
setting it works but the test shows it does - why doesn't this clobber 
the value of GIT_REFLOG_ACTION set by the user?

Best Wishes

Phillip

>   	if (opts->allow_ff)
>   		assert(!(opts->signoff || opts->no_commit ||
>   				opts->record_origin || opts->edit));
> @@ -3845,12 +3848,14 @@ static int pick_commits(struct repository *r,
>   		}
>   		if (item->command <= TODO_SQUASH) {
>   			if (is_rebase_i(opts))
> -				setenv("GIT_REFLOG_ACTION", reflog_message(opts,
> +				setenv(GIT_REFLOG_ACTION, reflog_message(opts,
>   					command_to_string(item->command), NULL),
>   					1);
>   			res = do_pick_commit(r, item->command, item->commit,
>   					     opts, is_final_fixup(todo_list),
>   					     &check_todo);
> +			if (is_rebase_i(opts))
> +				setenv(GIT_REFLOG_ACTION, prev_reflog_action, 1);
>   			if (is_rebase_i(opts) && res < 0) {
>   				/* Reschedule */
>   				advise(_(rescheduled_advice),
> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> index 61b76f33019..927a4f4a4e4 100755
> --- a/t/t3406-rebase-message.sh
> +++ b/t/t3406-rebase-message.sh
> @@ -89,22 +89,22 @@ test_expect_success 'GIT_REFLOG_ACTION' '
>   	git checkout -b reflog-topic start &&
>   	test_commit reflog-to-rebase &&
>   
> -	git rebase --apply reflog-onto &&
> +	git rebase reflog-onto &&
>   	git log -g --format=%gs -3 >actual &&
>   	cat >expect <<-\EOF &&
> -	rebase finished: returning to refs/heads/reflog-topic
> -	rebase: reflog-to-rebase
> -	rebase: checkout reflog-onto
> +	rebase (finish): returning to refs/heads/reflog-topic
> +	rebase (pick): reflog-to-rebase
> +	rebase (start): checkout reflog-onto
>   	EOF
>   	test_cmp expect actual &&
>   
>   	git checkout -b reflog-prefix reflog-to-rebase &&
> -	GIT_REFLOG_ACTION=change-the-reflog git rebase --apply reflog-onto &&
> +	GIT_REFLOG_ACTION=change-the-reflog git rebase reflog-onto &&
>   	git log -g --format=%gs -3 >actual &&
>   	cat >expect <<-\EOF &&
> -	rebase finished: returning to refs/heads/reflog-prefix
> -	change-the-reflog: reflog-to-rebase
> -	change-the-reflog: checkout reflog-onto
> +	change-the-reflog (finish): returning to refs/heads/reflog-prefix
> +	change-the-reflog (pick): reflog-to-rebase
> +	change-the-reflog (start): checkout reflog-onto
>   	EOF
>   	test_cmp expect actual
>   '
> 
> base-commit: 274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925
> 