[PATCH 0/2] rebase: stop setting GIT_REFLOG_ACTION

[PATCH 0/2] rebase: stop setting GIT_REFLOG_ACTION

[Phillip Wood via GitGitGadget]

This is a follow up to pw/rebase-reflog-fixes that moves away from using
GIT_REFLOG_ACTION internally. This conflicts with patches 12 & 14 in [1]. As
this series replaces the code being changed in those patches I think the
best solution would be to just drop them.

[1]
https://lore.kernel.org/git/cover-00.17-00000000000-20221103T164632Z-avarab@gmail.com/

Phillip Wood (2):
  sequencer: stop exporting GIT_REFLOG_ACTION
  rebase: stop exporting GIT_REFLOG_ACTION

 builtin/rebase.c | 27 +++++++++++++++------------
 sequencer.c      | 45 +++++++++++++++++++++++++--------------------
 sequencer.h      |  6 ++++++
 3 files changed, 46 insertions(+), 32 deletions(-)


base-commit: 3b08839926fcc7cc48cf4c759737c1a71af430c1
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1405%2Fphillipwood%2Fmore-rebase-reflog-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1405/phillipwood/more-rebase-reflog-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1405
-- 
gitgitgadget

[PATCH 1/2] sequencer: stop exporting GIT_REFLOG_ACTION

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Each time it picks a commit the sequencer copies the GIT_REFLOG_ACITON
environment variable so it can temporarily change it and then restore
the previous value. This results in code that is hard to follow and also
leaks memory because (i) we fail to free the copy when we've finished
with it and (ii) each call to setenv() leaks the previous value. Instead
pass the reflog action around in a variable and use it to set
GIT_REFLOG_ACTION in the child environment when running "git commit".

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 45 +++++++++++++++++++++++++--------------------
 sequencer.h |  6 ++++++
 2 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index e658df7e8ff..e23f6f0b718 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -375,6 +375,7 @@ int sequencer_remove_state(struct replay_opts *opts)
 	}
 
 	free(opts->gpg_sign);
+	free(opts->reflog_action);
 	free(opts->default_strategy);
 	free(opts->strategy);
 	for (i = 0; i < opts->xopts_nr; i++)
@@ -1050,6 +1051,8 @@ static int run_git_commit(const char *defmsg,
 			     gpg_opt, gpg_opt);
 	}
 
+	strvec_pushf(&cmd.env, GIT_REFLOG_ACTION "=%s", opts->reflog_message);
+
 	if (opts->committer_date_is_author_date)
 		strvec_pushf(&cmd.env, "GIT_COMMITTER_DATE=%s",
 			     opts->ignore_date ?
@@ -1589,8 +1592,8 @@ static int try_to_commit(struct repository *r,
 		goto out;
 	}
 
-	if (update_head_with_reflog(current_head, oid,
-				    getenv("GIT_REFLOG_ACTION"), msg, &err)) {
+	if (update_head_with_reflog(current_head, oid, opts->reflog_message,
+				    msg, &err)) {
 		res = error("%s", err.buf);
 		goto out;
 	}
@@ -3674,17 +3677,28 @@ static int do_label(struct repository *r, const char *name, int len)
 	return ret;
 }
 
+static const char *sequencer_reflog_action(struct replay_opts *opts)
+{
+	if (!opts->reflog_action) {
+		opts->reflog_action = getenv(GIT_REFLOG_ACTION);
+		opts->reflog_action =
+			xstrdup(opts->reflog_action ? opts->reflog_action
+						    : action_name(opts));
+	}
+
+	return opts->reflog_action;
+}
+
 __attribute__((format (printf, 3, 4)))
 static const char *reflog_message(struct replay_opts *opts,
 	const char *sub_action, const char *fmt, ...)
 {
 	va_list ap;
 	static struct strbuf buf = STRBUF_INIT;
-	char *reflog_action = getenv(GIT_REFLOG_ACTION);
 
 	va_start(ap, fmt);
 	strbuf_reset(&buf);
-	strbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));
+	strbuf_addstr(&buf, sequencer_reflog_action(opts));
 	if (sub_action)
 		strbuf_addf(&buf, " (%s)", sub_action);
 	if (fmt) {
@@ -4497,7 +4511,7 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,
 				RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
 		.head_msg = reflog_message(opts, "start", "checkout %s",
 					   onto_name),
-		.default_reflog_action = "rebase"
+		.default_reflog_action = sequencer_reflog_action(opts)
 	};
 	if (reset_head(r, &ropts)) {
 		apply_autostash(rebase_path_autostash());
@@ -4566,11 +4580,8 @@ static int pick_commits(struct repository *r,
 			struct replay_opts *opts)
 {
 	int res = 0, reschedule = 0;
-	char *prev_reflog_action;
 
-	/* Note that 0 for 3rd parameter of setenv means set only if not set */
-	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
-	prev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));
+	opts->reflog_message = sequencer_reflog_action(opts);
 	if (opts->allow_ff)
 		assert(!(opts->signoff || opts->no_commit ||
 			 opts->record_origin || should_edit(opts) ||
@@ -4618,14 +4629,12 @@ static int pick_commits(struct repository *r,
 		}
 		if (item->command <= TODO_SQUASH) {
 			if (is_rebase_i(opts))
-				setenv(GIT_REFLOG_ACTION, reflog_message(opts,
-					command_to_string(item->command), NULL),
-					1);
+				opts->reflog_message = reflog_message(opts,
+				      command_to_string(item->command), NULL);
+
 			res = do_pick_commit(r, item, 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),
@@ -5050,8 +5059,6 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
 	if (read_populate_opts(opts))
 		return -1;
 	if (is_rebase_i(opts)) {
-		char *previous_reflog_action;
-
 		if ((res = read_populate_todo(r, &todo_list, opts)))
 			goto release_todo_list;
 
@@ -5062,13 +5069,11 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
 			unlink(rebase_path_dropped());
 		}
 
-		previous_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));
-		setenv(GIT_REFLOG_ACTION, reflog_message(opts, "continue", NULL), 1);
+		opts->reflog_message = reflog_message(opts, "continue", NULL);
 		if (commit_staged_changes(r, opts, &todo_list)) {
 			res = -1;
 			goto release_todo_list;
 		}
-		setenv(GIT_REFLOG_ACTION, previous_reflog_action, 1);
 	} else if (!file_exists(get_todo_path(opts)))
 		return continue_single_pick(r, opts);
 	else if ((res = read_populate_todo(r, &todo_list, opts)))
@@ -5116,7 +5121,7 @@ static int single_pick(struct repository *r,
 			TODO_PICK : TODO_REVERT;
 	item.commit = cmit;
 
-	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
+	opts->reflog_message = sequencer_reflog_action(opts);
 	return do_pick_commit(r, &item, opts, 0, &check_todo);
 }
 
diff --git a/sequencer.h b/sequencer.h
index 563fe599334..888c18aad71 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -63,6 +63,9 @@ struct replay_opts {
 	char **xopts;
 	size_t xopts_nr, xopts_alloc;
 
+	/* Reflog */
+	char *reflog_action;
+
 	/* Used by fixup/squash */
 	struct strbuf current_fixups;
 	int current_fixup_count;
@@ -73,6 +76,9 @@ struct replay_opts {
 
 	/* Only used by REPLAY_NONE */
 	struct rev_info *revs;
+
+	/* Private use */
+	const char *reflog_message;
 };
 #define REPLAY_OPTS_INIT { .edit = -1, .action = -1, .current_fixups = STRBUF_INIT }
 
-- 
gitgitgadget

[PATCH 2/2] rebase: stop exporting GIT_REFLOG_ACTION

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Now that struct replay_opts has a reflog_action member we no longer
need to export GIT_REFLOG_ACTION when starting a rebase. If the user
has set GIT_REFLOG_ACTION then we use it when initializing
reflog_action.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5d855fd8f51..4d6839a5785 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -30,8 +30,6 @@
 #include "reset.h"
 #include "hook.h"
 
-#define DEFAULT_REFLOG_ACTION "rebase"
-
 static char const * const builtin_rebase_usage[] = {
 	N_("git rebase [-i] [options] [--exec <cmd>] "
 		"[--onto <newbase> | --keep-base] [<upstream> [<branch>]]"),
@@ -106,6 +104,7 @@ struct rebase_options {
 	} flags;
 	struct strvec git_am_opts;
 	enum action action;
+	char *reflog_action;
 	int signoff;
 	int allow_rerere_autoupdate;
 	int keep_empty;
@@ -159,6 +158,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
 					opts->committer_date_is_author_date;
 	replay.ignore_date = opts->ignore_date;
 	replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
+	replay.reflog_action = xstrdup(opts->reflog_action);
 	if (opts->strategy)
 		replay.strategy = xstrdup_or_null(opts->strategy);
 	else if (!replay.strategy && replay.default_strategy) {
@@ -585,10 +585,10 @@ static int move_to_original_branch(struct rebase_options *opts)
 		BUG("move_to_original_branch without onto");
 
 	strbuf_addf(&branch_reflog, "%s (finish): %s onto %s",
-		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
+		    opts->reflog_action,
 		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
 	strbuf_addf(&head_reflog, "%s (finish): returning to %s",
-		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), opts->head_name);
+		    opts->reflog_action, opts->head_name);
 	ropts.branch = opts->head_name;
 	ropts.flags = RESET_HEAD_REFS_ONLY;
 	ropts.branch_msg = branch_reflog.buf;
@@ -618,7 +618,7 @@ static int run_am(struct rebase_options *opts)
 	am.git_cmd = 1;
 	strvec_push(&am.args, "am");
 	strvec_pushf(&am.env, GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)",
-		     getenv(GIT_REFLOG_ACTION_ENVIRONMENT));
+		     opts->reflog_action);
 	if (opts->action == ACTION_CONTINUE) {
 		strvec_push(&am.args, "--resolved");
 		strvec_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
@@ -685,7 +685,7 @@ static int run_am(struct rebase_options *opts)
 
 		ropts.oid = &opts->orig_head->object.oid;
 		ropts.branch = opts->head_name;
-		ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
+		ropts.default_reflog_action = opts->reflog_action;
 		reset_head(the_repository, &ropts);
 		error(_("\ngit encountered an error while preparing the "
 			"patches to replay\n"
@@ -834,8 +834,7 @@ static int checkout_up_to_date(struct rebase_options *options)
 	int ret = 0;
 
 	strbuf_addf(&buf, "%s: checkout %s",
-		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
-		    options->switch_to);
+		    options->reflog_action, options->switch_to);
 	ropts.oid = &options->orig_head->object.oid;
 	ropts.branch = options->head_name;
 	ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
@@ -1243,7 +1242,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 
 	if (options.action != ACTION_NONE && !in_progress)
 		die(_("No rebase in progress?"));
-	setenv(GIT_REFLOG_ACTION_ENVIRONMENT, "rebase", 0);
 
 	if (options.action == ACTION_EDIT_TODO && !is_merge(&options))
 		die(_("The --edit-todo action can only be used during "
@@ -1258,6 +1256,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			trace2_cmd_mode(action_names[options.action]);
 	}
 
+	options.reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
+	options.reflog_action =
+		xstrdup(options.reflog_action ? options.reflog_action : "rebase");
+
 	switch (options.action) {
 	case ACTION_CONTINUE: {
 		struct object_id head;
@@ -1310,7 +1312,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			exit(1);
 
 		strbuf_addf(&head_msg, "%s (abort): returning to %s",
-			    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
+			    options.reflog_action,
 			    options.head_name ? options.head_name
 					      : oid_to_hex(&options.orig_head->object.oid));
 		ropts.oid = &options.orig_head->object.oid;
@@ -1786,13 +1788,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			 "it...\n"));
 
 	strbuf_addf(&msg, "%s (start): checkout %s",
-		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
+		    options.reflog_action, options.onto_name);
 	ropts.oid = &options.onto->object.oid;
 	ropts.orig_head = &options.orig_head->object.oid,
 	ropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |
 			RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
 	ropts.head_msg = msg.buf;
-	ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
+	ropts.default_reflog_action = options.reflog_action;
 	if (reset_head(the_repository, &ropts))
 		die(_("Could not detach HEAD"));
 	strbuf_release(&msg);
@@ -1824,6 +1826,7 @@ run_rebase:
 cleanup:
 	strbuf_release(&buf);
 	strbuf_release(&revisions);
+	free(options.reflog_action);
 	free(options.head_name);
 	free(options.gpg_sign_opt);
 	free(options.cmd);
-- 
gitgitgadget

Re: [PATCH 0/2] rebase: stop setting GIT_REFLOG_ACTION

[Taylor Blau]

On Fri, Nov 04, 2022 at 03:19:00PM +0000, Phillip Wood via GitGitGadget wrote:
> This is a follow up to pw/rebase-reflog-fixes that moves away from using
> GIT_REFLOG_ACTION internally. This conflicts with patches 12 & 14 in [1]. As
> this series replaces the code being changed in those patches I think the
> best solution would be to just drop them.

Thanks, I appreciate the updated round.

The conflict you noted in [1] is a perfect example of why I dislike
queuing sweeping leak cleanups like in that series. Those two patches
need to get dropped in order to queue this series. OK, except what
happens if a different part of [1] marks a test as leak-free when that
is no longer the case because of something in this series?

I haven't queued this topic yet, so perhaps all of this is moot with
respect to these particular two series. But in general, such a problem
is not hard to imagine.

It is greatly appreciated to err on the side of smaller, more targeted
series instead of sweeping changes when they can be avoided.

Thanks,
Taylor

Re: [PATCH 0/2] rebase: stop setting GIT_REFLOG_ACTION

[Taylor Blau]

[dropping broken email from the To: list]

On Fri, Nov 04, 2022 at 05:49:03PM -0400, Taylor Blau wrote:
> On Fri, Nov 04, 2022 at 03:19:00PM +0000, Phillip Wood via GitGitGadget wrote:
> > This is a follow up to pw/rebase-reflog-fixes that moves away from using
> > GIT_REFLOG_ACTION internally. This conflicts with patches 12 & 14 in [1]. As
> > this series replaces the code being changed in those patches I think the
> > best solution would be to just drop them.
>
> Thanks, I appreciate the updated round.
>
> The conflict you noted in [1] is a perfect example of why I dislike
> queuing sweeping leak cleanups like in that series. Those two patches
> need to get dropped in order to queue this series. OK, except what
> happens if a different part of [1] marks a test as leak-free when that
> is no longer the case because of something in this series?
>
> I haven't queued this topic yet, so perhaps all of this is moot with
> respect to these particular two series. But in general, such a problem
> is not hard to imagine.
>
> It is greatly appreciated to err on the side of smaller, more targeted
> series instead of sweeping changes when they can be avoided.
>
> Thanks,
> Taylor

Re: [PATCH 1/2] sequencer: stop exporting GIT_REFLOG_ACTION

[Taylor Blau]

On Fri, Nov 04, 2022 at 03:19:01PM +0000, Phillip Wood via GitGitGadget wrote:
> ---
>  sequencer.c | 45 +++++++++++++++++++++++++--------------------
>  sequencer.h |  6 ++++++
>  2 files changed, 31 insertions(+), 20 deletions(-)

The end result is much cleaner, thanks. Are there any spots left in the
code that still expect to read GIT_REFLOG_ACTION from their environment?

I did a quick look around and based on my understanding of the sequencer
code this should all be OK. But some analysis in the patch message is
appreciated.

Thanks,
Taylor

Re: [PATCH 0/2] rebase: stop setting GIT_REFLOG_ACTION

[Ævar Arnfjörð Bjarmason]

On Fri, Nov 04 2022, Taylor Blau wrote:

> On Fri, Nov 04, 2022 at 03:19:00PM +0000, Phillip Wood via GitGitGadget wrote:
>> This is a follow up to pw/rebase-reflog-fixes that moves away from using
>> GIT_REFLOG_ACTION internally. This conflicts with patches 12 & 14 in [1]. As
>> this series replaces the code being changed in those patches I think the
>> best solution would be to just drop them.
>
> Thanks, I appreciate the updated round.
>
> The conflict you noted in [1] is a perfect example of why I dislike
> queuing sweeping leak cleanups like in that series. Those two patches
> need to get dropped in order to queue this series. OK, except what
> happens if a different part of [1] marks a test as leak-free when that
> is no longer the case because of something in this series?

I'm about to rebase my v2 on this topic, which I think is the best way
forward, so this is about to become a moot point.

But I think this is a good example of why it's better to solve the merge
conflict rather than dropping patches from one topic:

In this case the merge conflict is trivial to solve: Keep the side of
this topic over mine, and after remove the one function call the
compiler was alerting about.

> I haven't queued this topic yet, so perhaps all of this is moot with
> respect to these particular two series. But in general, such a problem
> is not hard to imagine.

Yes, by ejecting the two patches from my topic it doesn't pass anymore
with:

	git rebase -i -x 'GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true make SANITIZE=leak test'

Which is not really a problem, it's an obscure test mode that only I'm
using, and before that topic we failed on "master" anyway. Just say'n,
in general... :)

Re: [PATCH 1/2] sequencer: stop exporting GIT_REFLOG_ACTION

[Ævar Arnfjörð Bjarmason]

On Fri, Nov 04 2022, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>

> +static const char *sequencer_reflog_action(struct replay_opts *opts)
> +{
> +	if (!opts->reflog_action) {
> +		opts->reflog_action = getenv(GIT_REFLOG_ACTION);
> +		opts->reflog_action =
> +			xstrdup(opts->reflog_action ? opts->reflog_action
> +						    : action_name(opts));
> +	}
> +
> +	return opts->reflog_action;
> +}

We always return an xstrdup'd here, so this should be a "char *" return
value, not a "const char *"., but

>  __attribute__((format (printf, 3, 4)))
>  static const char *reflog_message(struct replay_opts *opts,
>  	const char *sub_action, const char *fmt, ...)
>  {
>  	va_list ap;
>  	static struct strbuf buf = STRBUF_INIT;
> -	char *reflog_action = getenv(GIT_REFLOG_ACTION);
>  
>  	va_start(ap, fmt);
>  	strbuf_reset(&buf);

Here we just reset the strbuf...

> -	strbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));
> +	strbuf_addstr(&buf, sequencer_reflog_action(opts));

Here we leak the freshly xstrdup'd value, mostly untested, but shouldn't
this instead be:
	
	diff --git a/sequencer.c b/sequencer.c
	index e23f6f0b718..58a97e04c67 100644
	--- a/sequencer.c
	+++ b/sequencer.c
	@@ -3695,10 +3695,11 @@ static const char *reflog_message(struct replay_opts *opts,
	 {
	 	va_list ap;
	 	static struct strbuf buf = STRBUF_INIT;
	+	char *msg = sequencer_reflog_action(opts);
	 
	 	va_start(ap, fmt);
	 	strbuf_reset(&buf);
	-	strbuf_addstr(&buf, sequencer_reflog_action(opts));
	+	strbuf_attach(&buf, msg, strlen(msg), strlen(msg) + 1);
	 	if (sub_action)
	 		strbuf_addf(&buf, " (%s)", sub_action);
	 	if (fmt) {

Of course that requires dropping the "const", per the above...


>  	if (sub_action)
>  		strbuf_addf(&buf, " (%s)", sub_action);
>  	if (fmt) {
> @@ -4497,7 +4511,7 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,
>  				RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
>  		.head_msg = reflog_message(opts, "start", "checkout %s",
>  					   onto_name),
> -		.default_reflog_action = "rebase"
> +		.default_reflog_action = sequencer_reflog_action(opts)

Here we'd before hand a fixed string to reset_head(), but now it's
xstrdup()'d, but the corresponding free() on that side is missing.

But aren't we always just returing "rebase" here still?

> [...]
> @@ -5116,7 +5121,7 @@ static int single_pick(struct repository *r,
>  			TODO_PICK : TODO_REVERT;
>  	item.commit = cmit;
>  
> -	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
> +	opts->reflog_message = sequencer_reflog_action(opts);
>  	return do_pick_commit(r, &item, opts, 0, &check_todo);

Here you're adding a new memory leak, which you can see if you run
e.g. the 1st test of ./t1013-read-tree-submodule.sh before & after this
change.

Re: [PATCH 1/2] sequencer: stop exporting GIT_REFLOG_ACTION

[Phillip Wood]

Hi Ævar

On 07/11/2022 16:12, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Nov 04 2022, Phillip Wood via GitGitGadget wrote:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
>> +static const char *sequencer_reflog_action(struct replay_opts *opts)
>> +{
>> +	if (!opts->reflog_action) {
>> +		opts->reflog_action = getenv(GIT_REFLOG_ACTION);
>> +		opts->reflog_action =
>> +			xstrdup(opts->reflog_action ? opts->reflog_action
>> +						    : action_name(opts));
>> +	}
>> +
>> +	return opts->reflog_action;
>> +}
> 
> We always return an xstrdup'd here, so this should be a "char *" return
> value, not a "const char *"., but

We always return the same string, it is only allocated on the first 
call. opt->reflog_action is freed at the end of the rebase in 
sequencer_remove_state().

>>   __attribute__((format (printf, 3, 4)))
>>   static const char *reflog_message(struct replay_opts *opts,
>>   	const char *sub_action, const char *fmt, ...)
>>   {
>>   	va_list ap;
>>   	static struct strbuf buf = STRBUF_INIT;
>> -	char *reflog_action = getenv(GIT_REFLOG_ACTION);
>>   
>>   	va_start(ap, fmt);
>>   	strbuf_reset(&buf);
> 
> Here we just reset the strbuf...
> 
>> -	strbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));
>> +	strbuf_addstr(&buf, sequencer_reflog_action(opts));
> 
> Here we leak the freshly xstrdup'd value, mostly untested, but shouldn't
> this instead be:
> 	
> 	diff --git a/sequencer.c b/sequencer.c
> 	index e23f6f0b718..58a97e04c67 100644
> 	--- a/sequencer.c
> 	+++ b/sequencer.c
> 	@@ -3695,10 +3695,11 @@ static const char *reflog_message(struct replay_opts *opts,
> 	 {
> 	 	va_list ap;
> 	 	static struct strbuf buf = STRBUF_INIT;
> 	+	char *msg = sequencer_reflog_action(opts);
> 	
> 	 	va_start(ap, fmt);
> 	 	strbuf_reset(&buf);
> 	-	strbuf_addstr(&buf, sequencer_reflog_action(opts));
> 	+	strbuf_attach(&buf, msg, strlen(msg), strlen(msg) + 1);
> 	 	if (sub_action)
> 	 		strbuf_addf(&buf, " (%s)", sub_action);
> 	 	if (fmt) {
> 
> Of course that requires dropping the "const", per the above...
> 
> 
>>   	if (sub_action)
>>   		strbuf_addf(&buf, " (%s)", sub_action);
>>   	if (fmt) {
>> @@ -4497,7 +4511,7 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,
>>   				RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
>>   		.head_msg = reflog_message(opts, "start", "checkout %s",
>>   					   onto_name),
>> -		.default_reflog_action = "rebase"
>> +		.default_reflog_action = sequencer_reflog_action(opts)
> 
> Here we'd before hand a fixed string to reset_head(), but now it's
> xstrdup()'d, but the corresponding free() on that side is missing.
> 
> But aren't we always just returing "rebase" here still?

Yes but I think it makes sense to call squencer_reflog_action() in case 
this changes in the future. Note I don't think this is leaking anything.

>> [...]
>> @@ -5116,7 +5121,7 @@ static int single_pick(struct repository *r,
>>   			TODO_PICK : TODO_REVERT;
>>   	item.commit = cmit;
>>   
>> -	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
>> +	opts->reflog_message = sequencer_reflog_action(opts);
>>   	return do_pick_commit(r, &item, opts, 0, &check_todo);
> 
> Here you're adding a new memory leak, which you can see if you run
> e.g. the 1st test of ./t1013-read-tree-submodule.sh before & after this
> change.

I'm not sure how, opts->reflog_message will be a copy of 
opts->reflog_action which is freed at the end of the rebase. I'll have a 
proper look tomorrow to see if I'm missing something.

Best Wishes

Phillip

Re: [PATCH 0/2] rebase: stop setting GIT_REFLOG_ACTION

[Taylor Blau]

On Mon, Nov 07, 2022 at 04:51:38PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
> On Fri, Nov 04 2022, Taylor Blau wrote:
>
> > On Fri, Nov 04, 2022 at 03:19:00PM +0000, Phillip Wood via GitGitGadget wrote:
> >> This is a follow up to pw/rebase-reflog-fixes that moves away from using
> >> GIT_REFLOG_ACTION internally. This conflicts with patches 12 & 14 in [1]. As
> >> this series replaces the code being changed in those patches I think the
> >> best solution would be to just drop them.
> >
> > Thanks, I appreciate the updated round.
> >
> > The conflict you noted in [1] is a perfect example of why I dislike
> > queuing sweeping leak cleanups like in that series. Those two patches
> > need to get dropped in order to queue this series. OK, except what
> > happens if a different part of [1] marks a test as leak-free when that
> > is no longer the case because of something in this series?
>
> I'm about to rebase my v2 on this topic, which I think is the best way
> forward, so this is about to become a moot point.
>
> But I think this is a good example of why it's better to solve the merge
> conflict rather than dropping patches from one topic:
>
> In this case the merge conflict is trivial to solve: Keep the side of
> this topic over mine, and after remove the one function call the
> compiler was alerting about.

I agree that it is better to solve the merge conflict. But doing so is
time consuming, especially in a series which is unfamiliar to me. I
appreciate you sending a debased version.

Thanks,
Taylor

Re: [PATCH 1/2] sequencer: stop exporting GIT_REFLOG_ACTION

[Phillip Wood]

Hi Ævar

On 07/11/2022 19:35, Phillip Wood wrote:
>>> @@ -5116,7 +5121,7 @@ static int single_pick(struct repository *r,
>>>               TODO_PICK : TODO_REVERT;
>>>       item.commit = cmit;
>>> -    setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
>>> +    opts->reflog_message = sequencer_reflog_action(opts);
>>>       return do_pick_commit(r, &item, opts, 0, &check_todo);
>>
>> Here you're adding a new memory leak, which you can see if you run
>> e.g. the 1st test of ./t1013-read-tree-submodule.sh before & after this
>> change.

What's a read-tree test using rebase for? I find the submodule tests 
completely incomprehensible. It is calling 
test_submodule_switch_recursing_with_args() which does not call rebase 
directly but who knows what is going on in all the helper functions. 
Have you got a simple example of a test which shows a new leak?

> I'm not sure how, opts->reflog_message will be a copy of 
> opts->reflog_action which is freed at the end of the rebase. I'll have a 
> proper look tomorrow to see if I'm missing something.

So it is possible this is showing up because I think we only free the 
heap allocated members of opts in sequencer_remove_state() and that is 
not called when we stop for a conflict resolution, a break command, a 
failed exec or a rescheduled pick/reset etc. The way to fix that would 
be to refactor sequencer_remove_state() to create replay_opts_release() 
and call that from builtin/{revert,rebase}.c

As that is unrelated to removing the setenv() calls which is the focus 
of this series I will not be doing that in this series.

> Best Wishes
> 
> Phillip

Re: [PATCH 1/2] sequencer: stop exporting GIT_REFLOG_ACTION

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 08 2022, Phillip Wood wrote:

> Hi Ævar
>
> On 07/11/2022 19:35, Phillip Wood wrote:
>>>> @@ -5116,7 +5121,7 @@ static int single_pick(struct repository *r,
>>>>               TODO_PICK : TODO_REVERT;
>>>>       item.commit = cmit;
>>>> -    setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
>>>> +    opts->reflog_message = sequencer_reflog_action(opts);
>>>>       return do_pick_commit(r, &item, opts, 0, &check_todo);
>>>
>>> Here you're adding a new memory leak, which you can see if you run
>>> e.g. the 1st test of ./t1013-read-tree-submodule.sh before & after this
>>> change.
>
> What's a read-tree test using rebase for? I find the submodule tests
> completely incomprehensible. It is calling 
> test_submodule_switch_recursing_with_args() which does not call rebase
> directly but who knows what is going on in all the helper functions. 

I don't know, I just worked by way backwards from the leak logs, so...

> Have you got a simple example of a test which shows a new leak?

...yes, e.g. (after make SANITIZE=leak):

	./t3425-rebase-topology-merges.sh -vixd

Will, on "master", emit:
	
	Direct leak of 1408 byte(s) in 1 object(s) allocated from:
	    #0 0x7ff891b5f545 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:75
	    #1 0x6c45e8 in do_xmalloc wrapper.c:51
	    #2 0x6c4670 in xmalloc wrapper.c:72
	    #3 0x6037e2 in parse_options_concat parse-options-cb.c:188
	    #4 0x4c547c in run_sequencer builtin/revert.c:140
	    #5 0x4c5a4c in cmd_revert builtin/revert.c:247
	    #6 0x407a32 in run_builtin git.c:466
	    #7 0x407e0a in handle_builtin git.c:721
	    #8 0x40803d in run_argv git.c:788
	    #9 0x40850f in cmd_main git.c:923
	    #10 0x4eed6f in main common-main.c:57
	    #11 0x7ff8918b9209 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
	    #12 0x7ff8918b92bb in __libc_start_main_impl ../csu/libc-start.c:389
	    #13 0x405fd0 in _start (git+0x405fd0)
	
	Direct leak of 4 byte(s) in 1 object(s) allocated from:
	    #0 0x7ff891b5f545 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:75
	    #1 0x7ff891929caa in __GI___strdup string/strdup.c:42
	    #2 0x6c4591 in xstrdup wrapper.c:39
	    #3 0x4c58f8 in run_sequencer builtin/revert.c:223
	    #4 0x4c5a4c in cmd_revert builtin/revert.c:247
	    #5 0x407a32 in run_builtin git.c:466
	    #6 0x407e0a in handle_builtin git.c:721
	    #7 0x40803d in run_argv git.c:788
	    #8 0x40850f in cmd_main git.c:923
	    #9 0x4eed6f in main common-main.c:57
	    #10 0x7ff8918b9209 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
	    #11 0x7ff8918b92bb in __libc_start_main_impl ../csu/libc-start.c:389
	    #12 0x405fd0 in _start (git+0x405fd0)

After we still have the first leak (which is unrelated), and the second,
but have added this one:
	
	Direct leak of 7 byte(s) in 1 object(s) allocated from:
	    #0 0x7f7cc51e5545 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:75
	    #1 0x7f7cc4fafcaa in __GI___strdup string/strdup.c:42
	    #2 0x6c460b in xstrdup wrapper.c:39
	    #3 0x66df91 in sequencer_reflog_action sequencer.c:3685
	    #4 0x6725ad in single_pick sequencer.c:5124
	    #5 0x6728dd in sequencer_pick_revisions sequencer.c:5178
	    #6 0x4c5a17 in run_sequencer builtin/revert.c:237
	    #7 0x4c5aa9 in cmd_revert builtin/revert.c:247
	    #8 0x407a32 in run_builtin git.c:466
	    #9 0x407e0a in handle_builtin git.c:721
	    #10 0x40803d in run_argv git.c:788
	    #11 0x40850f in cmd_main git.c:923
	    #12 0x4eedcc in main common-main.c:57
	    #13 0x7f7cc4f3f209 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
	    #14 0x7f7cc4f3f2bb in __libc_start_main_impl ../csu/libc-start.c:389
	    #15 0x405fd0 in _start (git+0x405fd0)

But more to the point, if you run the test suite with e.g.:

	GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true

You can find these raw reports in:

	grep -r sequencer test-results/*.leak

Or, from my github.com/avar/git.git use this nice script/alias to
summarize it (I haven't upstreamed this yet):

	$ git help scan-leaks-top
	'scan-leaks-top' is aliased to '!f() { cd t && git cat-file blob avar/add-new-sanitize-leak-test-modes-follow-up:t/aggregate-leaks.perl | perl - | less -S; }; f'

>> I'm not sure how, opts->reflog_message will be a copy of
>> opts->reflog_action which is freed at the end of the rebase. I'll
>> have a proper look tomorrow to see if I'm missing something.
>
> So it is possible this is showing up because I think we only free the
> heap allocated members of opts in sequencer_remove_state() and that is 
> not called when we stop for a conflict resolution, a break command, a
> failed exec or a rescheduled pick/reset etc. The way to fix that would 
> be to refactor sequencer_remove_state() to create
> replay_opts_release() and call that from builtin/{revert,rebase}.c

Yes, I think that's probably the root cause. I have a leak-fixing topic
as a follow-up to my current one, which among other things tried to
address this: https://github.com/avar/git/commit/7a150d1b7e2

I'd just forgot about it. That link currently says committed <24hrs ago,
but I was just rebasing the topic for something unrelated, I hacked this
up in mid-August.

> As that is unrelated to removing the setenv() calls which is the focus
> of this series I will not be doing that in this series.

I'm fine with us leaving this for now, and saying that it's OK that
we're adding some new leaks, if we're addressing the setenv/getenv
issue, and that we can fix the root cause of the current leaks later.

But let's be clear: It's not unrelated to your refactoring in this
topic, we didn't have this leak before, and now we have it. These two
patches are the cause of some new leaks we didn't have before.

And, if we run this on my topic which narrowly attempted to fix these
leaks e.g. that "t3425-rebase-topology-merges.sh" test will have just 1
leak in that failing test, v.s. 3 leaks with this topic (the "4 byte(s)
in 1 object(s)" above).

It's just a nice coincidence that our memory leaks are currently in such
a sorry state overall that this isn't failing e.g. the linux-leaks CI,
because the new leaks are being masked by tests that area already
failing due to other pre-existing leaks.

But all that being said I think the right move is for this topic to
proceed, perhaps with an updated commit message noting some of this.

It's really just running into the existing problem of
replay_opts_release(). If that destructor isn't reliable (which it
isn't) we can still make new use of it, and then fix how we call it
later for all its callers.

Which I've just tested: I cherry-pick that 7a150d1b7e2 and the few
preceding commits it needs (dcc104aef89..7a150d1b7e2), and then apply
this on top:
	
	diff --git a/builtin/revert.c b/builtin/revert.c
	index ee32c714a76..0abd805beed 100644
	--- a/builtin/revert.c
	+++ b/builtin/revert.c
	@@ -250,6 +250,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
	 	if (opts.revs)
	 		release_revisions(opts.revs);
	 	free(opts.revs);
	+	replay_opts_release(&opts);
	 	return res;
	 }

The leaks above are down to just the unrelated parse_options_concat()
leak. I.e. this really is just a case of us missing the destructor due
to a more general issue.

1. https://lore.kernel.org/git/8eec228d-d392-523d-2415-149b946f642e@dunelm.org.uk/

[PATCH v2 0/2] rebase: stop setting GIT_REFLOG_ACTION

[Phillip Wood via GitGitGadget]

This is a follow up to pw/rebase-reflog-fixes that moves away from using
GIT_REFLOG_ACTION internally.

Thanks to Taylor & Ævar for their comments on V1. I've updated the commit
message of patch 1 as suggested by Taylor, the code is unchanged.

Phillip Wood (2):
  sequencer: stop exporting GIT_REFLOG_ACTION
  rebase: stop exporting GIT_REFLOG_ACTION

 builtin/rebase.c | 27 +++++++++++++++------------
 sequencer.c      | 45 +++++++++++++++++++++++++--------------------
 sequencer.h      |  6 ++++++
 3 files changed, 46 insertions(+), 32 deletions(-)


base-commit: 3b08839926fcc7cc48cf4c759737c1a71af430c1
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1405%2Fphillipwood%2Fmore-rebase-reflog-fixes-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1405/phillipwood/more-rebase-reflog-fixes-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1405

Range-diff vs v1:

 1:  e9c3f5ac5c6 ! 1:  655b4e89f59 sequencer: stop exporting GIT_REFLOG_ACTION
     @@ Commit message
          pass the reflog action around in a variable and use it to set
          GIT_REFLOG_ACTION in the child environment when running "git commit".
      
     +    Within the sequencer GIT_REFLOG_ACTION is no longer set and is only read
     +    by sequencer_reflog_action(). It is still set by rebase before calling
     +    the sequencer, that will be addressed in the next commit. cherry-pick
     +    and revert are unaffected as they do not set GIT_REFLOG_ACTION before
     +    calling the sequencer.
     +
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
       ## sequencer.c ##
 2:  d3747bcc8d1 = 2:  31df037eafe rebase: stop exporting GIT_REFLOG_ACTION

-- 
gitgitgadget

[PATCH v2 1/2] sequencer: stop exporting GIT_REFLOG_ACTION

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Each time it picks a commit the sequencer copies the GIT_REFLOG_ACITON
environment variable so it can temporarily change it and then restore
the previous value. This results in code that is hard to follow and also
leaks memory because (i) we fail to free the copy when we've finished
with it and (ii) each call to setenv() leaks the previous value. Instead
pass the reflog action around in a variable and use it to set
GIT_REFLOG_ACTION in the child environment when running "git commit".

Within the sequencer GIT_REFLOG_ACTION is no longer set and is only read
by sequencer_reflog_action(). It is still set by rebase before calling
the sequencer, that will be addressed in the next commit. cherry-pick
and revert are unaffected as they do not set GIT_REFLOG_ACTION before
calling the sequencer.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 45 +++++++++++++++++++++++++--------------------
 sequencer.h |  6 ++++++
 2 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index e658df7e8ff..e23f6f0b718 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -375,6 +375,7 @@ int sequencer_remove_state(struct replay_opts *opts)
 	}
 
 	free(opts->gpg_sign);
+	free(opts->reflog_action);
 	free(opts->default_strategy);
 	free(opts->strategy);
 	for (i = 0; i < opts->xopts_nr; i++)
@@ -1050,6 +1051,8 @@ static int run_git_commit(const char *defmsg,
 			     gpg_opt, gpg_opt);
 	}
 
+	strvec_pushf(&cmd.env, GIT_REFLOG_ACTION "=%s", opts->reflog_message);
+
 	if (opts->committer_date_is_author_date)
 		strvec_pushf(&cmd.env, "GIT_COMMITTER_DATE=%s",
 			     opts->ignore_date ?
@@ -1589,8 +1592,8 @@ static int try_to_commit(struct repository *r,
 		goto out;
 	}
 
-	if (update_head_with_reflog(current_head, oid,
-				    getenv("GIT_REFLOG_ACTION"), msg, &err)) {
+	if (update_head_with_reflog(current_head, oid, opts->reflog_message,
+				    msg, &err)) {
 		res = error("%s", err.buf);
 		goto out;
 	}
@@ -3674,17 +3677,28 @@ static int do_label(struct repository *r, const char *name, int len)
 	return ret;
 }
 
+static const char *sequencer_reflog_action(struct replay_opts *opts)
+{
+	if (!opts->reflog_action) {
+		opts->reflog_action = getenv(GIT_REFLOG_ACTION);
+		opts->reflog_action =
+			xstrdup(opts->reflog_action ? opts->reflog_action
+						    : action_name(opts));
+	}
+
+	return opts->reflog_action;
+}
+
 __attribute__((format (printf, 3, 4)))
 static const char *reflog_message(struct replay_opts *opts,
 	const char *sub_action, const char *fmt, ...)
 {
 	va_list ap;
 	static struct strbuf buf = STRBUF_INIT;
-	char *reflog_action = getenv(GIT_REFLOG_ACTION);
 
 	va_start(ap, fmt);
 	strbuf_reset(&buf);
-	strbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));
+	strbuf_addstr(&buf, sequencer_reflog_action(opts));
 	if (sub_action)
 		strbuf_addf(&buf, " (%s)", sub_action);
 	if (fmt) {
@@ -4497,7 +4511,7 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,
 				RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
 		.head_msg = reflog_message(opts, "start", "checkout %s",
 					   onto_name),
-		.default_reflog_action = "rebase"
+		.default_reflog_action = sequencer_reflog_action(opts)
 	};
 	if (reset_head(r, &ropts)) {
 		apply_autostash(rebase_path_autostash());
@@ -4566,11 +4580,8 @@ static int pick_commits(struct repository *r,
 			struct replay_opts *opts)
 {
 	int res = 0, reschedule = 0;
-	char *prev_reflog_action;
 
-	/* Note that 0 for 3rd parameter of setenv means set only if not set */
-	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
-	prev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));
+	opts->reflog_message = sequencer_reflog_action(opts);
 	if (opts->allow_ff)
 		assert(!(opts->signoff || opts->no_commit ||
 			 opts->record_origin || should_edit(opts) ||
@@ -4618,14 +4629,12 @@ static int pick_commits(struct repository *r,
 		}
 		if (item->command <= TODO_SQUASH) {
 			if (is_rebase_i(opts))
-				setenv(GIT_REFLOG_ACTION, reflog_message(opts,
-					command_to_string(item->command), NULL),
-					1);
+				opts->reflog_message = reflog_message(opts,
+				      command_to_string(item->command), NULL);
+
 			res = do_pick_commit(r, item, 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),
@@ -5050,8 +5059,6 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
 	if (read_populate_opts(opts))
 		return -1;
 	if (is_rebase_i(opts)) {
-		char *previous_reflog_action;
-
 		if ((res = read_populate_todo(r, &todo_list, opts)))
 			goto release_todo_list;
 
@@ -5062,13 +5069,11 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
 			unlink(rebase_path_dropped());
 		}
 
-		previous_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));
-		setenv(GIT_REFLOG_ACTION, reflog_message(opts, "continue", NULL), 1);
+		opts->reflog_message = reflog_message(opts, "continue", NULL);
 		if (commit_staged_changes(r, opts, &todo_list)) {
 			res = -1;
 			goto release_todo_list;
 		}
-		setenv(GIT_REFLOG_ACTION, previous_reflog_action, 1);
 	} else if (!file_exists(get_todo_path(opts)))
 		return continue_single_pick(r, opts);
 	else if ((res = read_populate_todo(r, &todo_list, opts)))
@@ -5116,7 +5121,7 @@ static int single_pick(struct repository *r,
 			TODO_PICK : TODO_REVERT;
 	item.commit = cmit;
 
-	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
+	opts->reflog_message = sequencer_reflog_action(opts);
 	return do_pick_commit(r, &item, opts, 0, &check_todo);
 }
 
diff --git a/sequencer.h b/sequencer.h
index 563fe599334..888c18aad71 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -63,6 +63,9 @@ struct replay_opts {
 	char **xopts;
 	size_t xopts_nr, xopts_alloc;
 
+	/* Reflog */
+	char *reflog_action;
+
 	/* Used by fixup/squash */
 	struct strbuf current_fixups;
 	int current_fixup_count;
@@ -73,6 +76,9 @@ struct replay_opts {
 
 	/* Only used by REPLAY_NONE */
 	struct rev_info *revs;
+
+	/* Private use */
+	const char *reflog_message;
 };
 #define REPLAY_OPTS_INIT { .edit = -1, .action = -1, .current_fixups = STRBUF_INIT }
 
-- 
gitgitgadget

[PATCH v2 2/2] rebase: stop exporting GIT_REFLOG_ACTION

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Now that struct replay_opts has a reflog_action member we no longer
need to export GIT_REFLOG_ACTION when starting a rebase. If the user
has set GIT_REFLOG_ACTION then we use it when initializing
reflog_action.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5d855fd8f51..4d6839a5785 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -30,8 +30,6 @@
 #include "reset.h"
 #include "hook.h"
 
-#define DEFAULT_REFLOG_ACTION "rebase"
-
 static char const * const builtin_rebase_usage[] = {
 	N_("git rebase [-i] [options] [--exec <cmd>] "
 		"[--onto <newbase> | --keep-base] [<upstream> [<branch>]]"),
@@ -106,6 +104,7 @@ struct rebase_options {
 	} flags;
 	struct strvec git_am_opts;
 	enum action action;
+	char *reflog_action;
 	int signoff;
 	int allow_rerere_autoupdate;
 	int keep_empty;
@@ -159,6 +158,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
 					opts->committer_date_is_author_date;
 	replay.ignore_date = opts->ignore_date;
 	replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
+	replay.reflog_action = xstrdup(opts->reflog_action);
 	if (opts->strategy)
 		replay.strategy = xstrdup_or_null(opts->strategy);
 	else if (!replay.strategy && replay.default_strategy) {
@@ -585,10 +585,10 @@ static int move_to_original_branch(struct rebase_options *opts)
 		BUG("move_to_original_branch without onto");
 
 	strbuf_addf(&branch_reflog, "%s (finish): %s onto %s",
-		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
+		    opts->reflog_action,
 		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
 	strbuf_addf(&head_reflog, "%s (finish): returning to %s",
-		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), opts->head_name);
+		    opts->reflog_action, opts->head_name);
 	ropts.branch = opts->head_name;
 	ropts.flags = RESET_HEAD_REFS_ONLY;
 	ropts.branch_msg = branch_reflog.buf;
@@ -618,7 +618,7 @@ static int run_am(struct rebase_options *opts)
 	am.git_cmd = 1;
 	strvec_push(&am.args, "am");
 	strvec_pushf(&am.env, GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)",
-		     getenv(GIT_REFLOG_ACTION_ENVIRONMENT));
+		     opts->reflog_action);
 	if (opts->action == ACTION_CONTINUE) {
 		strvec_push(&am.args, "--resolved");
 		strvec_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
@@ -685,7 +685,7 @@ static int run_am(struct rebase_options *opts)
 
 		ropts.oid = &opts->orig_head->object.oid;
 		ropts.branch = opts->head_name;
-		ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
+		ropts.default_reflog_action = opts->reflog_action;
 		reset_head(the_repository, &ropts);
 		error(_("\ngit encountered an error while preparing the "
 			"patches to replay\n"
@@ -834,8 +834,7 @@ static int checkout_up_to_date(struct rebase_options *options)
 	int ret = 0;
 
 	strbuf_addf(&buf, "%s: checkout %s",
-		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
-		    options->switch_to);
+		    options->reflog_action, options->switch_to);
 	ropts.oid = &options->orig_head->object.oid;
 	ropts.branch = options->head_name;
 	ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
@@ -1243,7 +1242,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 
 	if (options.action != ACTION_NONE && !in_progress)
 		die(_("No rebase in progress?"));
-	setenv(GIT_REFLOG_ACTION_ENVIRONMENT, "rebase", 0);
 
 	if (options.action == ACTION_EDIT_TODO && !is_merge(&options))
 		die(_("The --edit-todo action can only be used during "
@@ -1258,6 +1256,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			trace2_cmd_mode(action_names[options.action]);
 	}
 
+	options.reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
+	options.reflog_action =
+		xstrdup(options.reflog_action ? options.reflog_action : "rebase");
+
 	switch (options.action) {
 	case ACTION_CONTINUE: {
 		struct object_id head;
@@ -1310,7 +1312,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			exit(1);
 
 		strbuf_addf(&head_msg, "%s (abort): returning to %s",
-			    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
+			    options.reflog_action,
 			    options.head_name ? options.head_name
 					      : oid_to_hex(&options.orig_head->object.oid));
 		ropts.oid = &options.orig_head->object.oid;
@@ -1786,13 +1788,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			 "it...\n"));
 
 	strbuf_addf(&msg, "%s (start): checkout %s",
-		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
+		    options.reflog_action, options.onto_name);
 	ropts.oid = &options.onto->object.oid;
 	ropts.orig_head = &options.orig_head->object.oid,
 	ropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |
 			RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
 	ropts.head_msg = msg.buf;
-	ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
+	ropts.default_reflog_action = options.reflog_action;
 	if (reset_head(the_repository, &ropts))
 		die(_("Could not detach HEAD"));
 	strbuf_release(&msg);
@@ -1824,6 +1826,7 @@ run_rebase:
 cleanup:
 	strbuf_release(&buf);
 	strbuf_release(&revisions);
+	free(options.reflog_action);
 	free(options.head_name);
 	free(options.gpg_sign_opt);
 	free(options.cmd);
-- 
gitgitgadget

Re: [PATCH v2 0/2] rebase: stop setting GIT_REFLOG_ACTION

[Ævar Arnfjörð Bjarmason]

On Wed, Nov 09 2022, Phillip Wood via GitGitGadget wrote:

> This is a follow up to pw/rebase-reflog-fixes that moves away from using
> GIT_REFLOG_ACTION internally.
>
> Thanks to Taylor & Ævar for their comments on V1. I've updated the commit
> message of patch 1 as suggested by Taylor, the code is unchanged.
>
> Phillip Wood (2):
>   sequencer: stop exporting GIT_REFLOG_ACTION
>   rebase: stop exporting GIT_REFLOG_ACTION
>
>  builtin/rebase.c | 27 +++++++++++++++------------
>  sequencer.c      | 45 +++++++++++++++++++++++++--------------------
>  sequencer.h      |  6 ++++++
>  3 files changed, 46 insertions(+), 32 deletions(-)
>
>
> base-commit: 3b08839926fcc7cc48cf4c759737c1a71af430c1
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1405%2Fphillipwood%2Fmore-rebase-reflog-fixes-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1405/phillipwood/more-rebase-reflog-fixes-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1405
>
> Range-diff vs v1:
>
>  1:  e9c3f5ac5c6 ! 1:  655b4e89f59 sequencer: stop exporting GIT_REFLOG_ACTION
>      @@ Commit message
>           pass the reflog action around in a variable and use it to set
>           GIT_REFLOG_ACTION in the child environment when running "git commit".
>       
>      +    Within the sequencer GIT_REFLOG_ACTION is no longer set and is only read
>      +    by sequencer_reflog_action(). It is still set by rebase before calling
>      +    the sequencer, that will be addressed in the next commit. cherry-pick
>      +    and revert are unaffected as they do not set GIT_REFLOG_ACTION before
>      +    calling the sequencer.
>      +
>           Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>       
>        ## sequencer.c ##
>  2:  d3747bcc8d1 = 2:  31df037eafe rebase: stop exporting GIT_REFLOG_ACTION

Thanks, FWIW I'm happy to give this my "Reviewed-by", per [1] I've
looked this over carefully.

The tl;dr of that is that this fixes a leak, and adds another one, but
the root cause of the added one is that you're using an existing
destructor that we sometimes don't call, which we can just address as a
follow-up generic issue (I've got patches to fix it).

But for now this is a good step forward, and fixes the leak that's
"unique" t this codepath.

And of course, just makes managing the "reflog" variable nicer in
general, as we're no longer talking to ourselves within the same process
with setenv()/getenv().

1. https://lore.kernel.org/git/221108.864jv9sc9r.gmgdl@evledraar.gmail.com/

Re: [PATCH v2 0/2] rebase: stop setting GIT_REFLOG_ACTION

[Phillip Wood]

On 09/11/2022 16:05, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Nov 09 2022, Phillip Wood via GitGitGadget wrote:
> 
>> This is a follow up to pw/rebase-reflog-fixes that moves away from using
>> GIT_REFLOG_ACTION internally.
>>
>> Thanks to Taylor & Ævar for their comments on V1. I've updated the commit
>> message of patch 1 as suggested by Taylor, the code is unchanged.
>>
>> Phillip Wood (2):
>>    sequencer: stop exporting GIT_REFLOG_ACTION
>>    rebase: stop exporting GIT_REFLOG_ACTION
>>
>>   builtin/rebase.c | 27 +++++++++++++++------------
>>   sequencer.c      | 45 +++++++++++++++++++++++++--------------------
>>   sequencer.h      |  6 ++++++
>>   3 files changed, 46 insertions(+), 32 deletions(-)
>>
>>
>> base-commit: 3b08839926fcc7cc48cf4c759737c1a71af430c1
>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1405%2Fphillipwood%2Fmore-rebase-reflog-fixes-v2
>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1405/phillipwood/more-rebase-reflog-fixes-v2
>> Pull-Request: https://github.com/gitgitgadget/git/pull/1405
>>
>> Range-diff vs v1:
>>
>>   1:  e9c3f5ac5c6 ! 1:  655b4e89f59 sequencer: stop exporting GIT_REFLOG_ACTION
>>       @@ Commit message
>>            pass the reflog action around in a variable and use it to set
>>            GIT_REFLOG_ACTION in the child environment when running "git commit".
>>        
>>       +    Within the sequencer GIT_REFLOG_ACTION is no longer set and is only read
>>       +    by sequencer_reflog_action(). It is still set by rebase before calling
>>       +    the sequencer, that will be addressed in the next commit. cherry-pick
>>       +    and revert are unaffected as they do not set GIT_REFLOG_ACTION before
>>       +    calling the sequencer.
>>       +
>>            Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>>        
>>         ## sequencer.c ##
>>   2:  d3747bcc8d1 = 2:  31df037eafe rebase: stop exporting GIT_REFLOG_ACTION
> 
> Thanks, FWIW I'm happy to give this my "Reviewed-by", per [1] I've
> looked this over carefully.
> 
> The tl;dr of that is that this fixes a leak, and adds another one, but
> the root cause of the added one is that you're using an existing
> destructor that we sometimes don't call, which we can just address as a
> follow-up generic issue (I've got patches to fix it).

It is worth noting that the leak that is fixed was unbounded and grew 
with each commit whereas the new one is a fixed size and is only a 
"leak" because we don't clean up properly when exiting.

> But for now this is a good step forward, and fixes the leak that's
> "unique" t this codepath.
> 
> And of course, just makes managing the "reflog" variable nicer in
> general, as we're no longer talking to ourselves within the same process
> with setenv()/getenv().

Yes, that was the main aim really

Best Wishes

Phillip

> 1. https://lore.kernel.org/git/221108.864jv9sc9r.gmgdl@evledraar.gmail.com/

Re: [PATCH v2 0/2] rebase: stop setting GIT_REFLOG_ACTION

[Taylor Blau]

On Wed, Nov 09, 2022 at 04:30:47PM +0000, Phillip Wood wrote:
> > But for now this is a good step forward, and fixes the leak that's
> > "unique" t this codepath.
> >
> > And of course, just makes managing the "reflog" variable nicer in
> > general, as we're no longer talking to ourselves within the same process
> > with setenv()/getenv().
>
> Yes, that was the main aim really

Thanks, both. Let's start merging this one down.

Thanks,
Taylor