From d188a60d7228f051bb4004924d20d961ada636f3 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Wed, 9 Nov 2022 14:21:57 +0000 Subject: [PATCH 1/2] sequencer: stop exporting GIT_REFLOG_ACTION MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Ævar Arnfjörð Bjarmason Signed-off-by: Taylor Blau --- sequencer.c | 45 +++++++++++++++++++++++++-------------------- sequencer.h | 6 ++++++ 2 files changed, 31 insertions(+), 20 deletions(-) diff --git a/sequencer.c b/sequencer.c index e658df7e8f..e23f6f0b71 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 563fe59933..888c18aad7 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 } From 0e34efb31d89a143e42788f26f07e2bf8776a851 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Wed, 9 Nov 2022 14:21:58 +0000 Subject: [PATCH 2/2] rebase: stop exporting GIT_REFLOG_ACTION MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Ævar Arnfjörð Bjarmason Signed-off-by: Taylor Blau --- builtin/rebase.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index 5d855fd8f5..4d6839a578 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 ] " "[--onto | --keep-base] [ []]"), @@ -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);