Skip to contentSkip to conversation

rebase: stop setting GIT_REFLOG_ACTION

PATCH Superseded Phillip Wood via GitGitGadget 4 participants 18 messages Nov 4, 2022 – Nov 9, 2022

lore
Share & tools
lore

Phillip Wood via GitGitGadget

author
#0 lore raw

cover rebase: stop setting GIT_REFLOG_ACTION

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/[email protected]/

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

Signature · 1 line
gitgitgadget
author
#1 lore raw

PATCH 1/2 sequencer: stop exporting GIT_REFLOG_ACTION

From: Phillip Wood <[email protected]>

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 <[email protected]>
---
 sequencer.c | 45 +++++++++++++++++++++++++--------------------
 sequencer.h |  6 ++++++
 2 files changed, 31 insertions(+), 20 deletions(-)
sequencer.c +25 -20
index e658df7e8ff..e23f6f0b718 100644
@@ -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);
 }
sequencer.h +6 -0
index 563fe599334..888c18aad71 100644
@@ -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 }
Signature · 1 line
gitgitgadget
author
#7 lore raw

PATCH 2/2 rebase: stop exporting GIT_REFLOG_ACTION

In reply to Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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 <[email protected]>
---
 builtin/rebase.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)
builtin/rebase.c +15 -12
index 5d855fd8f51..4d6839a5785 100644
@@ -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);
Signature · 1 line
gitgitgadget
#8 lore raw
In reply to Phillip Wood via GitGitGadget
Quoted 4 lines · 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.

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

#9 lore raw

[dropping broken email from the To: list]

Quoted 23 lines · Taylor Blau
Quoted 4 lines · 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.

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

#2 lore raw

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

In reply to Phillip Wood via GitGitGadget
Quoted 4 lines · Phillip Wood via GitGitGadget
---
 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

#10 lore raw
In reply to Taylor Blau
Quoted 6 lines · Taylor Blau
Quoted 4 lines · 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.

Quoted 7 lines

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.

Quoted 3 lines

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... :)

#3 lore raw

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

In reply to Phillip Wood via GitGitGadget

sequencer.c static int do_label(struct repository *r, const char *name, int len) · line 3690

Quoted 13 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

sequencer.c +11 -0
+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

sequencer.c static int do_label(struct repository *r, const char *name, int len) · line 3700

Quoted 10 lines

__attribute__((format (printf, 3, 4)))
static const char *reflog_message(struct replay_opts *opts,

sequencer.c +0 -1
 	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...

sequencer.c static int do_label(struct repository *r, const char *name, int len) · line 3701

Quoted 2 lines
sequencer.c +1 -1
-	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...

sequencer.c static int checkout_onto(struct repository *r, struct replay_opts *opts, · line 4514

Quoted 9 lines
sequencer.c +1 -1
 	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?

sequencer.c static int single_pick(struct repository *r, · line 5125

Quoted 8 lines

[...]

sequencer.c +1 -1
@@ -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.

#4 lore raw

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

Hi Ævar

Quoted 5 lines · Ævar Arnfjörð Bjarmason
Quoted 1 line · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

Quoted 14 lines

sequencer.c static int do_label(struct repository *r, const char *name, int len) · line 3690

Quoted 11 lines
sequencer.c +11 -0
+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().

Quoted 53 lines

sequencer.c static int do_label(struct repository *r, const char *name, int len) · line 3700

Quoted 10 lines
  __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;
sequencer.c +0 -1
-	char *reflog_action = getenv(GIT_REFLOG_ACTION);
 
  	va_start(ap, fmt);
  	strbuf_reset(&buf);

Here we just reset the strbuf...

sequencer.c static int do_label(struct repository *r, const char *name, int len) · line 3701

Quoted 2 lines
sequencer.c +1 -1
-	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...

sequencer.c static int checkout_onto(struct repository *r, struct replay_opts *opts, · line 4514

Quoted 9 lines
  	if (sub_action)
  		strbuf_addf(&buf, " (%s)", sub_action);
  	if (fmt) {
sequencer.c +1 -1
@@ -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.

Quoted 1 line

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.

Quoted 5 lines
Quoted 5 lines

[...]

sequencer.c +0 -0
@@ -5116,7 +5121,7 @@ static int single_pick(struct repository *r,
  			TODO_PICK : TODO_REVERT;
  	item.commit = cmit;
Quoted 7 lines

sequencer.c static int single_pick(struct repository *r, · line 5125

Quoted 3 lines
sequencer.c +1 -1
-	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

#11 lore raw
In reply to Ævar Arnfjörð Bjarmason
Quoted 20 lines · Ævar Arnfjörð Bjarmason
Quoted 6 lines · Taylor Blau
Quoted 4 lines · 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.

Quoted 7 lines

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.

Quoted 6 lines

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

#5 lore raw

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

In reply to Phillip Wood

Hi Ævar

Quoted 10 lines · Phillip Wood
Quoted 10 lines

sequencer.c static int single_pick(struct repository *r, · line 5124

Quoted 6 lines
sequencer.c +1 -1
@@ -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?

Quoted 1 line

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.

Quoted 3 lines

Best Wishes

Phillip

#6 lore raw

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

Quoted 14 lines · Phillip Wood

Hi Ævar

Quoted 10 lines · Phillip Wood
Quoted 10 lines

sequencer.c static int single_pick(struct repository *r, · line 5124

Quoted 6 lines
sequencer.c +1 -1
@@ -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.

Quoted 4 lines

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...

Quoted 1 line

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'
Quoted 10 lines
Quoted 3 lines

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.

Quoted 2 lines

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/[email protected]/

author
#12 lore raw

v2 cover rebase: stop setting GIT_REFLOG_ACTION

In reply to 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 <[email protected]>

## sequencer.c ##
2: d3747bcc8d1 = 2: 31df037eafe rebase: stop exporting GIT_REFLOG_ACTION

Signature · 1 line
gitgitgadget
author
#13 lore raw

PATCH v2 1/2 sequencer: stop exporting GIT_REFLOG_ACTION

From: Phillip Wood <[email protected]>

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 <[email protected]>
---
 sequencer.c | 45 +++++++++++++++++++++++++--------------------
 sequencer.h |  6 ++++++
 2 files changed, 31 insertions(+), 20 deletions(-)
sequencer.c +25 -20
index e658df7e8ff..e23f6f0b718 100644
@@ -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);
 }
sequencer.h +6 -0
index 563fe599334..888c18aad71 100644
@@ -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 }
Signature · 1 line
gitgitgadget
author
#14 lore raw

PATCH v2 2/2 rebase: stop exporting GIT_REFLOG_ACTION

In reply to Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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 <[email protected]>
---
 builtin/rebase.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)
builtin/rebase.c +15 -12
index 5d855fd8f51..4d6839a5785 100644
@@ -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);
Signature · 1 line
gitgitgadget
#15 lore raw
In reply to Phillip Wood via GitGitGadget
Quoted 34 lines · 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.
+

Quoted 4 lines
Signed-off-by Phillip Wood <[email protected]>

## 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/[email protected]/

#16 lore raw
Quoted 42 lines · Ævar Arnfjörð Bjarmason
Quoted 34 lines · 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.
+

Quoted 4 lines
Signed-off-by Phillip Wood <[email protected]>

## sequencer.c ##
2: d3747bcc8d1 = 2: 31df037eafe rebase: stop exporting GIT_REFLOG_ACTION

Quoted 7 lines

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.

Quoted 6 lines

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

Quoted 1 line

1. https://lore.kernel.org/git/[email protected]/

#17 lore raw
Quoted 8 lines · Phillip Wood
Quoted 6 lines

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