{"thread":{"id":"58747","subject":"[PATCH 0/2] rebase: stop setting GIT_REFLOG_ACTION","startedAt":"2022-11-04T15:19:07Z","lastAt":"2022-11-09T23:17:26Z","messageCount":18,"participants":["Phillip Wood via GitGitGadget","Taylor Blau","Ævar Arnfjörð Bjarmason","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"466536","messageId":"pull.1405.git.1667575142.gitgitgadget@gmail.com","threadId":"58747","inReplyTo":null,"subject":"[PATCH 0/2] rebase: stop setting GIT_REFLOG_ACTION","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-04T15:19:00Z","receivedAt":"2022-11-04T15:19:07Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"This is a follow up to pw/rebase-reflog-fixes that moves away from using\nGIT_REFLOG_ACTION internally. This conflicts with patches 12 & 14 in [1]. As\nthis series replaces the code being changed in those patches I think the\nbest solution would be to just drop them.\n\n[1]\nhttps://lore.kernel.org/git/cover-00.17-00000000000-20221103T164632Z-avarab@gmail.com/\n\nPhillip Wood (2):\n  sequencer: stop exporting GIT_REFLOG_ACTION\n  rebase: stop exporting GIT_REFLOG_ACTION\n\n builtin/rebase.c | 27 +++++++++++++++------------\n sequencer.c      | 45 +++++++++++++++++++++++++--------------------\n sequencer.h      |  6 ++++++\n 3 files changed, 46 insertions(+), 32 deletions(-)\n\n\nbase-commit: 3b08839926fcc7cc48cf4c759737c1a71af430c1\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1405%2Fphillipwood%2Fmore-rebase-reflog-fixes-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1405/phillipwood/more-rebase-reflog-fixes-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1405\n-- \ngitgitgadget\n"},{"id":"466537","messageId":"e9c3f5ac5c6b7a01ee9e43730889d8066a270271.1667575142.git.gitgitgadget@gmail.com","threadId":"58747","inReplyTo":"pull.1405.git.1667575142.gitgitgadget@gmail.com","subject":"[PATCH 1/2] sequencer: stop exporting GIT_REFLOG_ACTION","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-04T15:19:01Z","receivedAt":"2022-11-04T15:19:11Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nEach time it picks a commit the sequencer copies the GIT_REFLOG_ACITON\nenvironment variable so it can temporarily change it and then restore\nthe previous value. This results in code that is hard to follow and also\nleaks memory because (i) we fail to free the copy when we've finished\nwith it and (ii) each call to setenv() leaks the previous value. Instead\npass the reflog action around in a variable and use it to set\nGIT_REFLOG_ACTION in the child environment when running \"git commit\".\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c | 45 +++++++++++++++++++++++++--------------------\n sequencer.h |  6 ++++++\n 2 files changed, 31 insertions(+), 20 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex e658df7e8ff..e23f6f0b718 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -375,6 +375,7 @@ int sequencer_remove_state(struct replay_opts *opts)\n \t}\n \n \tfree(opts->gpg_sign);\n+\tfree(opts->reflog_action);\n \tfree(opts->default_strategy);\n \tfree(opts->strategy);\n \tfor (i = 0; i < opts->xopts_nr; i++)\n@@ -1050,6 +1051,8 @@ static int run_git_commit(const char *defmsg,\n \t\t\t     gpg_opt, gpg_opt);\n \t}\n \n+\tstrvec_pushf(&cmd.env, GIT_REFLOG_ACTION \"=%s\", opts->reflog_message);\n+\n \tif (opts->committer_date_is_author_date)\n \t\tstrvec_pushf(&cmd.env, \"GIT_COMMITTER_DATE=%s\",\n \t\t\t     opts->ignore_date ?\n@@ -1589,8 +1592,8 @@ static int try_to_commit(struct repository *r,\n \t\tgoto out;\n \t}\n \n-\tif (update_head_with_reflog(current_head, oid,\n-\t\t\t\t    getenv(\"GIT_REFLOG_ACTION\"), msg, &err)) {\n+\tif (update_head_with_reflog(current_head, oid, opts->reflog_message,\n+\t\t\t\t    msg, &err)) {\n \t\tres = error(\"%s\", err.buf);\n \t\tgoto out;\n \t}\n@@ -3674,17 +3677,28 @@ static int do_label(struct repository *r, const char *name, int len)\n \treturn ret;\n }\n \n+static const char *sequencer_reflog_action(struct replay_opts *opts)\n+{\n+\tif (!opts->reflog_action) {\n+\t\topts->reflog_action = getenv(GIT_REFLOG_ACTION);\n+\t\topts->reflog_action =\n+\t\t\txstrdup(opts->reflog_action ? opts->reflog_action\n+\t\t\t\t\t\t    : action_name(opts));\n+\t}\n+\n+\treturn opts->reflog_action;\n+}\n+\n __attribute__((format (printf, 3, 4)))\n static const char *reflog_message(struct replay_opts *opts,\n \tconst char *sub_action, const char *fmt, ...)\n {\n \tva_list ap;\n \tstatic struct strbuf buf = STRBUF_INIT;\n-\tchar *reflog_action = getenv(GIT_REFLOG_ACTION);\n \n \tva_start(ap, fmt);\n \tstrbuf_reset(&buf);\n-\tstrbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));\n+\tstrbuf_addstr(&buf, sequencer_reflog_action(opts));\n \tif (sub_action)\n \t\tstrbuf_addf(&buf, \" (%s)\", sub_action);\n \tif (fmt) {\n@@ -4497,7 +4511,7 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,\n \t\t\t\tRESET_HEAD_RUN_POST_CHECKOUT_HOOK,\n \t\t.head_msg = reflog_message(opts, \"start\", \"checkout %s\",\n \t\t\t\t\t   onto_name),\n-\t\t.default_reflog_action = \"rebase\"\n+\t\t.default_reflog_action = sequencer_reflog_action(opts)\n \t};\n \tif (reset_head(r, &ropts)) {\n \t\tapply_autostash(rebase_path_autostash());\n@@ -4566,11 +4580,8 @@ static int pick_commits(struct repository *r,\n \t\t\tstruct replay_opts *opts)\n {\n \tint res = 0, reschedule = 0;\n-\tchar *prev_reflog_action;\n \n-\t/* Note that 0 for 3rd parameter of setenv means set only if not set */\n-\tsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n-\tprev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));\n+\topts->reflog_message = sequencer_reflog_action(opts);\n \tif (opts->allow_ff)\n \t\tassert(!(opts->signoff || opts->no_commit ||\n \t\t\t opts->record_origin || should_edit(opts) ||\n@@ -4618,14 +4629,12 @@ static int pick_commits(struct repository *r,\n \t\t}\n \t\tif (item->command <= TODO_SQUASH) {\n \t\t\tif (is_rebase_i(opts))\n-\t\t\t\tsetenv(GIT_REFLOG_ACTION, reflog_message(opts,\n-\t\t\t\t\tcommand_to_string(item->command), NULL),\n-\t\t\t\t\t1);\n+\t\t\t\topts->reflog_message = reflog_message(opts,\n+\t\t\t\t      command_to_string(item->command), NULL);\n+\n \t\t\tres = do_pick_commit(r, item, opts,\n \t\t\t\t\t     is_final_fixup(todo_list),\n \t\t\t\t\t     &check_todo);\n-\t\t\tif (is_rebase_i(opts))\n-\t\t\t\tsetenv(GIT_REFLOG_ACTION, prev_reflog_action, 1);\n \t\t\tif (is_rebase_i(opts) && res < 0) {\n \t\t\t\t/* Reschedule */\n \t\t\t\tadvise(_(rescheduled_advice),\n@@ -5050,8 +5059,6 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)\n \tif (read_populate_opts(opts))\n \t\treturn -1;\n \tif (is_rebase_i(opts)) {\n-\t\tchar *previous_reflog_action;\n-\n \t\tif ((res = read_populate_todo(r, &todo_list, opts)))\n \t\t\tgoto release_todo_list;\n \n@@ -5062,13 +5069,11 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)\n \t\t\tunlink(rebase_path_dropped());\n \t\t}\n \n-\t\tprevious_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));\n-\t\tsetenv(GIT_REFLOG_ACTION, reflog_message(opts, \"continue\", NULL), 1);\n+\t\topts->reflog_message = reflog_message(opts, \"continue\", NULL);\n \t\tif (commit_staged_changes(r, opts, &todo_list)) {\n \t\t\tres = -1;\n \t\t\tgoto release_todo_list;\n \t\t}\n-\t\tsetenv(GIT_REFLOG_ACTION, previous_reflog_action, 1);\n \t} else if (!file_exists(get_todo_path(opts)))\n \t\treturn continue_single_pick(r, opts);\n \telse if ((res = read_populate_todo(r, &todo_list, opts)))\n@@ -5116,7 +5121,7 @@ static int single_pick(struct repository *r,\n \t\t\tTODO_PICK : TODO_REVERT;\n \titem.commit = cmit;\n \n-\tsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n+\topts->reflog_message = sequencer_reflog_action(opts);\n \treturn do_pick_commit(r, &item, opts, 0, &check_todo);\n }\n \ndiff --git a/sequencer.h b/sequencer.h\nindex 563fe599334..888c18aad71 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -63,6 +63,9 @@ struct replay_opts {\n \tchar **xopts;\n \tsize_t xopts_nr, xopts_alloc;\n \n+\t/* Reflog */\n+\tchar *reflog_action;\n+\n \t/* Used by fixup/squash */\n \tstruct strbuf current_fixups;\n \tint current_fixup_count;\n@@ -73,6 +76,9 @@ struct replay_opts {\n \n \t/* Only used by REPLAY_NONE */\n \tstruct rev_info *revs;\n+\n+\t/* Private use */\n+\tconst char *reflog_message;\n };\n #define REPLAY_OPTS_INIT { .edit = -1, .action = -1, .current_fixups = STRBUF_INIT }\n \n-- \ngitgitgadget\n\n"},{"id":"466538","messageId":"d3747bcc8d138192592727a3e63b2ccede8b9fc5.1667575142.git.gitgitgadget@gmail.com","threadId":"58747","inReplyTo":"pull.1405.git.1667575142.gitgitgadget@gmail.com","subject":"[PATCH 2/2] rebase: stop exporting GIT_REFLOG_ACTION","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-04T15:19:02Z","receivedAt":"2022-11-04T15:19:15Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nNow that struct replay_opts has a reflog_action member we no longer\nneed to export GIT_REFLOG_ACTION when starting a rebase. If the user\nhas set GIT_REFLOG_ACTION then we use it when initializing\nreflog_action.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n builtin/rebase.c | 27 +++++++++++++++------------\n 1 file changed, 15 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 5d855fd8f51..4d6839a5785 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -30,8 +30,6 @@\n #include \"reset.h\"\n #include \"hook.h\"\n \n-#define DEFAULT_REFLOG_ACTION \"rebase\"\n-\n static char const * const builtin_rebase_usage[] = {\n \tN_(\"git rebase [-i] [options] [--exec <cmd>] \"\n \t\t\"[--onto <newbase> | --keep-base] [<upstream> [<branch>]]\"),\n@@ -106,6 +104,7 @@ struct rebase_options {\n \t} flags;\n \tstruct strvec git_am_opts;\n \tenum action action;\n+\tchar *reflog_action;\n \tint signoff;\n \tint allow_rerere_autoupdate;\n \tint keep_empty;\n@@ -159,6 +158,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n \t\t\t\t\topts->committer_date_is_author_date;\n \treplay.ignore_date = opts->ignore_date;\n \treplay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);\n+\treplay.reflog_action = xstrdup(opts->reflog_action);\n \tif (opts->strategy)\n \t\treplay.strategy = xstrdup_or_null(opts->strategy);\n \telse if (!replay.strategy && replay.default_strategy) {\n@@ -585,10 +585,10 @@ static int move_to_original_branch(struct rebase_options *opts)\n \t\tBUG(\"move_to_original_branch without onto\");\n \n \tstrbuf_addf(&branch_reflog, \"%s (finish): %s onto %s\",\n-\t\t    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),\n+\t\t    opts->reflog_action,\n \t\t    opts->head_name, oid_to_hex(&opts->onto->object.oid));\n \tstrbuf_addf(&head_reflog, \"%s (finish): returning to %s\",\n-\t\t    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), opts->head_name);\n+\t\t    opts->reflog_action, opts->head_name);\n \tropts.branch = opts->head_name;\n \tropts.flags = RESET_HEAD_REFS_ONLY;\n \tropts.branch_msg = branch_reflog.buf;\n@@ -618,7 +618,7 @@ static int run_am(struct rebase_options *opts)\n \tam.git_cmd = 1;\n \tstrvec_push(&am.args, \"am\");\n \tstrvec_pushf(&am.env, GIT_REFLOG_ACTION_ENVIRONMENT \"=%s (pick)\",\n-\t\t     getenv(GIT_REFLOG_ACTION_ENVIRONMENT));\n+\t\t     opts->reflog_action);\n \tif (opts->action == ACTION_CONTINUE) {\n \t\tstrvec_push(&am.args, \"--resolved\");\n \t\tstrvec_pushf(&am.args, \"--resolvemsg=%s\", resolvemsg);\n@@ -685,7 +685,7 @@ static int run_am(struct rebase_options *opts)\n \n \t\tropts.oid = &opts->orig_head->object.oid;\n \t\tropts.branch = opts->head_name;\n-\t\tropts.default_reflog_action = DEFAULT_REFLOG_ACTION;\n+\t\tropts.default_reflog_action = opts->reflog_action;\n \t\treset_head(the_repository, &ropts);\n \t\terror(_(\"\\ngit encountered an error while preparing the \"\n \t\t\t\"patches to replay\\n\"\n@@ -834,8 +834,7 @@ static int checkout_up_to_date(struct rebase_options *options)\n \tint ret = 0;\n \n \tstrbuf_addf(&buf, \"%s: checkout %s\",\n-\t\t    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),\n-\t\t    options->switch_to);\n+\t\t    options->reflog_action, options->switch_to);\n \tropts.oid = &options->orig_head->object.oid;\n \tropts.branch = options->head_name;\n \tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n@@ -1243,7 +1242,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \n \tif (options.action != ACTION_NONE && !in_progress)\n \t\tdie(_(\"No rebase in progress?\"));\n-\tsetenv(GIT_REFLOG_ACTION_ENVIRONMENT, \"rebase\", 0);\n \n \tif (options.action == ACTION_EDIT_TODO && !is_merge(&options))\n \t\tdie(_(\"The --edit-todo action can only be used during \"\n@@ -1258,6 +1256,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\ttrace2_cmd_mode(action_names[options.action]);\n \t}\n \n+\toptions.reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);\n+\toptions.reflog_action =\n+\t\txstrdup(options.reflog_action ? options.reflog_action : \"rebase\");\n+\n \tswitch (options.action) {\n \tcase ACTION_CONTINUE: {\n \t\tstruct object_id head;\n@@ -1310,7 +1312,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\texit(1);\n \n \t\tstrbuf_addf(&head_msg, \"%s (abort): returning to %s\",\n-\t\t\t    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),\n+\t\t\t    options.reflog_action,\n \t\t\t    options.head_name ? options.head_name\n \t\t\t\t\t      : oid_to_hex(&options.orig_head->object.oid));\n \t\tropts.oid = &options.orig_head->object.oid;\n@@ -1786,13 +1788,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t \"it...\\n\"));\n \n \tstrbuf_addf(&msg, \"%s (start): checkout %s\",\n-\t\t    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);\n+\t\t    options.reflog_action, options.onto_name);\n \tropts.oid = &options.onto->object.oid;\n \tropts.orig_head = &options.orig_head->object.oid,\n \tropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |\n \t\t\tRESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n \tropts.head_msg = msg.buf;\n-\tropts.default_reflog_action = DEFAULT_REFLOG_ACTION;\n+\tropts.default_reflog_action = options.reflog_action;\n \tif (reset_head(the_repository, &ropts))\n \t\tdie(_(\"Could not detach HEAD\"));\n \tstrbuf_release(&msg);\n@@ -1824,6 +1826,7 @@ run_rebase:\n cleanup:\n \tstrbuf_release(&buf);\n \tstrbuf_release(&revisions);\n+\tfree(options.reflog_action);\n \tfree(options.head_name);\n \tfree(options.gpg_sign_opt);\n \tfree(options.cmd);\n-- \ngitgitgadget\n"},{"id":"466558","messageId":"Y2WIz5qvjQMfXPgP@nand.local","threadId":"58747","inReplyTo":"pull.1405.git.1667575142.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] rebase: stop setting GIT_REFLOG_ACTION","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-04T21:49:03Z","receivedAt":"2022-11-04T21:49:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Nov 04, 2022 at 03:19:00PM +0000, Phillip Wood via GitGitGadget wrote:\n> This is a follow up to pw/rebase-reflog-fixes that moves away from using\n> GIT_REFLOG_ACTION internally. This conflicts with patches 12 & 14 in [1]. As\n> this series replaces the code being changed in those patches I think the\n> best solution would be to just drop them.\n\nThanks, I appreciate the updated round.\n\nThe conflict you noted in [1] is a perfect example of why I dislike\nqueuing sweeping leak cleanups like in that series. Those two patches\nneed to get dropped in order to queue this series. OK, except what\nhappens if a different part of [1] marks a test as leak-free when that\nis no longer the case because of something in this series?\n\nI haven't queued this topic yet, so perhaps all of this is moot with\nrespect to these particular two series. But in general, such a problem\nis not hard to imagine.\n\nIt is greatly appreciated to err on the side of smaller, more targeted\nseries instead of sweeping changes when they can be avoided.\n\nThanks,\nTaylor\n"},{"id":"466559","messageId":"Y2WI/C0E479jf6l7@nand.local","threadId":"58747","inReplyTo":"Y2WIz5qvjQMfXPgP@nand.local","subject":"Re: [PATCH 0/2] rebase: stop setting GIT_REFLOG_ACTION","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-04T21:49:48Z","receivedAt":"2022-11-04T21:49:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"[dropping broken email from the To: list]\n\nOn Fri, Nov 04, 2022 at 05:49:03PM -0400, Taylor Blau wrote:\n> On Fri, Nov 04, 2022 at 03:19:00PM +0000, Phillip Wood via GitGitGadget wrote:\n> > This is a follow up to pw/rebase-reflog-fixes that moves away from using\n> > GIT_REFLOG_ACTION internally. This conflicts with patches 12 & 14 in [1]. As\n> > this series replaces the code being changed in those patches I think the\n> > best solution would be to just drop them.\n>\n> Thanks, I appreciate the updated round.\n>\n> The conflict you noted in [1] is a perfect example of why I dislike\n> queuing sweeping leak cleanups like in that series. Those two patches\n> need to get dropped in order to queue this series. OK, except what\n> happens if a different part of [1] marks a test as leak-free when that\n> is no longer the case because of something in this series?\n>\n> I haven't queued this topic yet, so perhaps all of this is moot with\n> respect to these particular two series. But in general, such a problem\n> is not hard to imagine.\n>\n> It is greatly appreciated to err on the side of smaller, more targeted\n> series instead of sweeping changes when they can be avoided.\n>\n> Thanks,\n> Taylor\n"},{"id":"466560","messageId":"Y2WKjyksxc8/WySs@nand.local","threadId":"58747","inReplyTo":"e9c3f5ac5c6b7a01ee9e43730889d8066a270271.1667575142.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] sequencer: stop exporting GIT_REFLOG_ACTION","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-04T21:56:31Z","receivedAt":"2022-11-04T21:56:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Nov 04, 2022 at 03:19:01PM +0000, Phillip Wood via GitGitGadget wrote:\n> ---\n>  sequencer.c | 45 +++++++++++++++++++++++++--------------------\n>  sequencer.h |  6 ++++++\n>  2 files changed, 31 insertions(+), 20 deletions(-)\n\nThe end result is much cleaner, thanks. Are there any spots left in the\ncode that still expect to read GIT_REFLOG_ACTION from their environment?\n\nI did a quick look around and based on my understanding of the sequencer\ncode this should all be OK. But some analysis in the patch message is\nappreciated.\n\nThanks,\nTaylor\n"},{"id":"466673","messageId":"221107.86mt92vjul.gmgdl@evledraar.gmail.com","threadId":"58747","inReplyTo":"Y2WIz5qvjQMfXPgP@nand.local","subject":"Re: [PATCH 0/2] rebase: stop setting GIT_REFLOG_ACTION","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-07T15:51:38Z","receivedAt":"2022-11-07T15:57:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Nov 04 2022, Taylor Blau wrote:\n\n> On Fri, Nov 04, 2022 at 03:19:00PM +0000, Phillip Wood via GitGitGadget wrote:\n>> This is a follow up to pw/rebase-reflog-fixes that moves away from using\n>> GIT_REFLOG_ACTION internally. This conflicts with patches 12 & 14 in [1]. As\n>> this series replaces the code being changed in those patches I think the\n>> best solution would be to just drop them.\n>\n> Thanks, I appreciate the updated round.\n>\n> The conflict you noted in [1] is a perfect example of why I dislike\n> queuing sweeping leak cleanups like in that series. Those two patches\n> need to get dropped in order to queue this series. OK, except what\n> happens if a different part of [1] marks a test as leak-free when that\n> is no longer the case because of something in this series?\n\nI'm about to rebase my v2 on this topic, which I think is the best way\nforward, so this is about to become a moot point.\n\nBut I think this is a good example of why it's better to solve the merge\nconflict rather than dropping patches from one topic:\n\nIn this case the merge conflict is trivial to solve: Keep the side of\nthis topic over mine, and after remove the one function call the\ncompiler was alerting about.\n\n> I haven't queued this topic yet, so perhaps all of this is moot with\n> respect to these particular two series. But in general, such a problem\n> is not hard to imagine.\n\nYes, by ejecting the two patches from my topic it doesn't pass anymore\nwith:\n\n\tgit rebase -i -x 'GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true make SANITIZE=leak test'\n\nWhich is not really a problem, it's an obscure test mode that only I'm\nusing, and before that topic we failed on \"master\" anyway. Just say'n,\nin general... :)\n"},{"id":"466676","messageId":"221107.86iljqvhzf.gmgdl@evledraar.gmail.com","threadId":"58747","inReplyTo":"e9c3f5ac5c6b7a01ee9e43730889d8066a270271.1667575142.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] sequencer: stop exporting GIT_REFLOG_ACTION","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-07T16:12:08Z","receivedAt":"2022-11-07T16:37:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Nov 04 2022, Phillip Wood via GitGitGadget wrote:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\n> +static const char *sequencer_reflog_action(struct replay_opts *opts)\n> +{\n> +\tif (!opts->reflog_action) {\n> +\t\topts->reflog_action = getenv(GIT_REFLOG_ACTION);\n> +\t\topts->reflog_action =\n> +\t\t\txstrdup(opts->reflog_action ? opts->reflog_action\n> +\t\t\t\t\t\t    : action_name(opts));\n> +\t}\n> +\n> +\treturn opts->reflog_action;\n> +}\n\nWe always return an xstrdup'd here, so this should be a \"char *\" return\nvalue, not a \"const char *\"., but\n\n>  __attribute__((format (printf, 3, 4)))\n>  static const char *reflog_message(struct replay_opts *opts,\n>  \tconst char *sub_action, const char *fmt, ...)\n>  {\n>  \tva_list ap;\n>  \tstatic struct strbuf buf = STRBUF_INIT;\n> -\tchar *reflog_action = getenv(GIT_REFLOG_ACTION);\n>  \n>  \tva_start(ap, fmt);\n>  \tstrbuf_reset(&buf);\n\nHere we just reset the strbuf...\n\n> -\tstrbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));\n> +\tstrbuf_addstr(&buf, sequencer_reflog_action(opts));\n\nHere we leak the freshly xstrdup'd value, mostly untested, but shouldn't\nthis instead be:\n\t\n\tdiff --git a/sequencer.c b/sequencer.c\n\tindex e23f6f0b718..58a97e04c67 100644\n\t--- a/sequencer.c\n\t+++ b/sequencer.c\n\t@@ -3695,10 +3695,11 @@ static const char *reflog_message(struct replay_opts *opts,\n\t {\n\t \tva_list ap;\n\t \tstatic struct strbuf buf = STRBUF_INIT;\n\t+\tchar *msg = sequencer_reflog_action(opts);\n\t \n\t \tva_start(ap, fmt);\n\t \tstrbuf_reset(&buf);\n\t-\tstrbuf_addstr(&buf, sequencer_reflog_action(opts));\n\t+\tstrbuf_attach(&buf, msg, strlen(msg), strlen(msg) + 1);\n\t \tif (sub_action)\n\t \t\tstrbuf_addf(&buf, \" (%s)\", sub_action);\n\t \tif (fmt) {\n\nOf course that requires dropping the \"const\", per the above...\n\n\n>  \tif (sub_action)\n>  \t\tstrbuf_addf(&buf, \" (%s)\", sub_action);\n>  \tif (fmt) {\n> @@ -4497,7 +4511,7 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,\n>  \t\t\t\tRESET_HEAD_RUN_POST_CHECKOUT_HOOK,\n>  \t\t.head_msg = reflog_message(opts, \"start\", \"checkout %s\",\n>  \t\t\t\t\t   onto_name),\n> -\t\t.default_reflog_action = \"rebase\"\n> +\t\t.default_reflog_action = sequencer_reflog_action(opts)\n\nHere we'd before hand a fixed string to reset_head(), but now it's\nxstrdup()'d, but the corresponding free() on that side is missing.\n\nBut aren't we always just returing \"rebase\" here still?\n\n> [...]\n> @@ -5116,7 +5121,7 @@ static int single_pick(struct repository *r,\n>  \t\t\tTODO_PICK : TODO_REVERT;\n>  \titem.commit = cmit;\n>  \n> -\tsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n> +\topts->reflog_message = sequencer_reflog_action(opts);\n>  \treturn do_pick_commit(r, &item, opts, 0, &check_todo);\n\nHere you're adding a new memory leak, which you can see if you run\ne.g. the 1st test of ./t1013-read-tree-submodule.sh before & after this\nchange.\n"},{"id":"466722","messageId":"03cd4cdb-5f6b-11b9-1959-d58d5b544565@dunelm.org.uk","threadId":"58747","inReplyTo":"221107.86iljqvhzf.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/2] sequencer: stop exporting GIT_REFLOG_ACTION","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-11-07T19:35:22Z","receivedAt":"2022-11-07T19:35:31Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ævar\n\nOn 07/11/2022 16:12, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Fri, Nov 04 2022, Phillip Wood via GitGitGadget wrote:\n> \n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n> \n>> +static const char *sequencer_reflog_action(struct replay_opts *opts)\n>> +{\n>> +\tif (!opts->reflog_action) {\n>> +\t\topts->reflog_action = getenv(GIT_REFLOG_ACTION);\n>> +\t\topts->reflog_action =\n>> +\t\t\txstrdup(opts->reflog_action ? opts->reflog_action\n>> +\t\t\t\t\t\t    : action_name(opts));\n>> +\t}\n>> +\n>> +\treturn opts->reflog_action;\n>> +}\n> \n> We always return an xstrdup'd here, so this should be a \"char *\" return\n> value, not a \"const char *\"., but\n\nWe always return the same string, it is only allocated on the first \ncall. opt->reflog_action is freed at the end of the rebase in \nsequencer_remove_state().\n\n>>   __attribute__((format (printf, 3, 4)))\n>>   static const char *reflog_message(struct replay_opts *opts,\n>>   \tconst char *sub_action, const char *fmt, ...)\n>>   {\n>>   \tva_list ap;\n>>   \tstatic struct strbuf buf = STRBUF_INIT;\n>> -\tchar *reflog_action = getenv(GIT_REFLOG_ACTION);\n>>   \n>>   \tva_start(ap, fmt);\n>>   \tstrbuf_reset(&buf);\n> \n> Here we just reset the strbuf...\n> \n>> -\tstrbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));\n>> +\tstrbuf_addstr(&buf, sequencer_reflog_action(opts));\n> \n> Here we leak the freshly xstrdup'd value, mostly untested, but shouldn't\n> this instead be:\n> \t\n> \tdiff --git a/sequencer.c b/sequencer.c\n> \tindex e23f6f0b718..58a97e04c67 100644\n> \t--- a/sequencer.c\n> \t+++ b/sequencer.c\n> \t@@ -3695,10 +3695,11 @@ static const char *reflog_message(struct replay_opts *opts,\n> \t {\n> \t \tva_list ap;\n> \t \tstatic struct strbuf buf = STRBUF_INIT;\n> \t+\tchar *msg = sequencer_reflog_action(opts);\n> \t\n> \t \tva_start(ap, fmt);\n> \t \tstrbuf_reset(&buf);\n> \t-\tstrbuf_addstr(&buf, sequencer_reflog_action(opts));\n> \t+\tstrbuf_attach(&buf, msg, strlen(msg), strlen(msg) + 1);\n> \t \tif (sub_action)\n> \t \t\tstrbuf_addf(&buf, \" (%s)\", sub_action);\n> \t \tif (fmt) {\n> \n> Of course that requires dropping the \"const\", per the above...\n> \n> \n>>   \tif (sub_action)\n>>   \t\tstrbuf_addf(&buf, \" (%s)\", sub_action);\n>>   \tif (fmt) {\n>> @@ -4497,7 +4511,7 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,\n>>   \t\t\t\tRESET_HEAD_RUN_POST_CHECKOUT_HOOK,\n>>   \t\t.head_msg = reflog_message(opts, \"start\", \"checkout %s\",\n>>   \t\t\t\t\t   onto_name),\n>> -\t\t.default_reflog_action = \"rebase\"\n>> +\t\t.default_reflog_action = sequencer_reflog_action(opts)\n> \n> Here we'd before hand a fixed string to reset_head(), but now it's\n> xstrdup()'d, but the corresponding free() on that side is missing.\n> \n> But aren't we always just returing \"rebase\" here still?\n\nYes but I think it makes sense to call squencer_reflog_action() in case \nthis changes in the future. Note I don't think this is leaking anything.\n\n>> [...]\n>> @@ -5116,7 +5121,7 @@ static int single_pick(struct repository *r,\n>>   \t\t\tTODO_PICK : TODO_REVERT;\n>>   \titem.commit = cmit;\n>>   \n>> -\tsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n>> +\topts->reflog_message = sequencer_reflog_action(opts);\n>>   \treturn do_pick_commit(r, &item, opts, 0, &check_todo);\n> \n> Here you're adding a new memory leak, which you can see if you run\n> e.g. the 1st test of ./t1013-read-tree-submodule.sh before & after this\n> change.\n\nI'm not sure how, opts->reflog_message will be a copy of \nopts->reflog_action which is freed at the end of the rebase. I'll have a \nproper look tomorrow to see if I'm missing something.\n\nBest Wishes\n\nPhillip\n"},{"id":"466728","messageId":"Y2li3Xg5vVTEInUB@nand.local","threadId":"58747","inReplyTo":"221107.86mt92vjul.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 0/2] rebase: stop setting GIT_REFLOG_ACTION","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-07T19:56:13Z","receivedAt":"2022-11-07T19:56:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 07, 2022 at 04:51:38PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n> On Fri, Nov 04 2022, Taylor Blau wrote:\n>\n> > On Fri, Nov 04, 2022 at 03:19:00PM +0000, Phillip Wood via GitGitGadget wrote:\n> >> This is a follow up to pw/rebase-reflog-fixes that moves away from using\n> >> GIT_REFLOG_ACTION internally. This conflicts with patches 12 & 14 in [1]. As\n> >> this series replaces the code being changed in those patches I think the\n> >> best solution would be to just drop them.\n> >\n> > Thanks, I appreciate the updated round.\n> >\n> > The conflict you noted in [1] is a perfect example of why I dislike\n> > queuing sweeping leak cleanups like in that series. Those two patches\n> > need to get dropped in order to queue this series. OK, except what\n> > happens if a different part of [1] marks a test as leak-free when that\n> > is no longer the case because of something in this series?\n>\n> I'm about to rebase my v2 on this topic, which I think is the best way\n> forward, so this is about to become a moot point.\n>\n> But I think this is a good example of why it's better to solve the merge\n> conflict rather than dropping patches from one topic:\n>\n> In this case the merge conflict is trivial to solve: Keep the side of\n> this topic over mine, and after remove the one function call the\n> compiler was alerting about.\n\nI agree that it is better to solve the merge conflict. But doing so is\ntime consuming, especially in a series which is unfamiliar to me. I\nappreciate you sending a debased version.\n\nThanks,\nTaylor\n"},{"id":"466801","messageId":"16baa0cd-0797-4427-3e39-e5ffd2dca544@dunelm.org.uk","threadId":"58747","inReplyTo":"03cd4cdb-5f6b-11b9-1959-d58d5b544565@dunelm.org.uk","subject":"Re: [PATCH 1/2] sequencer: stop exporting GIT_REFLOG_ACTION","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-11-08T09:54:05Z","receivedAt":"2022-11-08T09:54:09Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ævar\n\nOn 07/11/2022 19:35, Phillip Wood wrote:\n>>> @@ -5116,7 +5121,7 @@ static int single_pick(struct repository *r,\n>>>               TODO_PICK : TODO_REVERT;\n>>>       item.commit = cmit;\n>>> -    setenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n>>> +    opts->reflog_message = sequencer_reflog_action(opts);\n>>>       return do_pick_commit(r, &item, opts, 0, &check_todo);\n>>\n>> Here you're adding a new memory leak, which you can see if you run\n>> e.g. the 1st test of ./t1013-read-tree-submodule.sh before & after this\n>> change.\n\nWhat's a read-tree test using rebase for? I find the submodule tests \ncompletely incomprehensible. It is calling \ntest_submodule_switch_recursing_with_args() which does not call rebase \ndirectly but who knows what is going on in all the helper functions. \nHave you got a simple example of a test which shows a new leak?\n\n> I'm not sure how, opts->reflog_message will be a copy of \n> opts->reflog_action which is freed at the end of the rebase. I'll have a \n> proper look tomorrow to see if I'm missing something.\n\nSo it is possible this is showing up because I think we only free the \nheap allocated members of opts in sequencer_remove_state() and that is \nnot called when we stop for a conflict resolution, a break command, a \nfailed exec or a rescheduled pick/reset etc. The way to fix that would \nbe to refactor sequencer_remove_state() to create replay_opts_release() \nand call that from builtin/{revert,rebase}.c\n\nAs that is unrelated to removing the setenv() calls which is the focus \nof this series I will not be doing that in this series.\n\n> Best Wishes\n> \n> Phillip\n"},{"id":"466847","messageId":"221108.864jv9sc9r.gmgdl@evledraar.gmail.com","threadId":"58747","inReplyTo":"16baa0cd-0797-4427-3e39-e5ffd2dca544@dunelm.org.uk","subject":"Re: [PATCH 1/2] sequencer: stop exporting GIT_REFLOG_ACTION","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-08T14:51:08Z","receivedAt":"2022-11-08T15:21:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 08 2022, Phillip Wood wrote:\n\n> Hi Ævar\n>\n> On 07/11/2022 19:35, Phillip Wood wrote:\n>>>> @@ -5116,7 +5121,7 @@ static int single_pick(struct repository *r,\n>>>>               TODO_PICK : TODO_REVERT;\n>>>>       item.commit = cmit;\n>>>> -    setenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n>>>> +    opts->reflog_message = sequencer_reflog_action(opts);\n>>>>       return do_pick_commit(r, &item, opts, 0, &check_todo);\n>>>\n>>> Here you're adding a new memory leak, which you can see if you run\n>>> e.g. the 1st test of ./t1013-read-tree-submodule.sh before & after this\n>>> change.\n>\n> What's a read-tree test using rebase for? I find the submodule tests\n> completely incomprehensible. It is calling \n> test_submodule_switch_recursing_with_args() which does not call rebase\n> directly but who knows what is going on in all the helper functions. \n\nI don't know, I just worked by way backwards from the leak logs, so...\n\n> Have you got a simple example of a test which shows a new leak?\n\n...yes, e.g. (after make SANITIZE=leak):\n\n\t./t3425-rebase-topology-merges.sh -vixd\n\nWill, on \"master\", emit:\n\t\n\tDirect leak of 1408 byte(s) in 1 object(s) allocated from:\n\t    #0 0x7ff891b5f545 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:75\n\t    #1 0x6c45e8 in do_xmalloc wrapper.c:51\n\t    #2 0x6c4670 in xmalloc wrapper.c:72\n\t    #3 0x6037e2 in parse_options_concat parse-options-cb.c:188\n\t    #4 0x4c547c in run_sequencer builtin/revert.c:140\n\t    #5 0x4c5a4c in cmd_revert builtin/revert.c:247\n\t    #6 0x407a32 in run_builtin git.c:466\n\t    #7 0x407e0a in handle_builtin git.c:721\n\t    #8 0x40803d in run_argv git.c:788\n\t    #9 0x40850f in cmd_main git.c:923\n\t    #10 0x4eed6f in main common-main.c:57\n\t    #11 0x7ff8918b9209 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58\n\t    #12 0x7ff8918b92bb in __libc_start_main_impl ../csu/libc-start.c:389\n\t    #13 0x405fd0 in _start (git+0x405fd0)\n\t\n\tDirect leak of 4 byte(s) in 1 object(s) allocated from:\n\t    #0 0x7ff891b5f545 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:75\n\t    #1 0x7ff891929caa in __GI___strdup string/strdup.c:42\n\t    #2 0x6c4591 in xstrdup wrapper.c:39\n\t    #3 0x4c58f8 in run_sequencer builtin/revert.c:223\n\t    #4 0x4c5a4c in cmd_revert builtin/revert.c:247\n\t    #5 0x407a32 in run_builtin git.c:466\n\t    #6 0x407e0a in handle_builtin git.c:721\n\t    #7 0x40803d in run_argv git.c:788\n\t    #8 0x40850f in cmd_main git.c:923\n\t    #9 0x4eed6f in main common-main.c:57\n\t    #10 0x7ff8918b9209 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58\n\t    #11 0x7ff8918b92bb in __libc_start_main_impl ../csu/libc-start.c:389\n\t    #12 0x405fd0 in _start (git+0x405fd0)\n\nAfter we still have the first leak (which is unrelated), and the second,\nbut have added this one:\n\t\n\tDirect leak of 7 byte(s) in 1 object(s) allocated from:\n\t    #0 0x7f7cc51e5545 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:75\n\t    #1 0x7f7cc4fafcaa in __GI___strdup string/strdup.c:42\n\t    #2 0x6c460b in xstrdup wrapper.c:39\n\t    #3 0x66df91 in sequencer_reflog_action sequencer.c:3685\n\t    #4 0x6725ad in single_pick sequencer.c:5124\n\t    #5 0x6728dd in sequencer_pick_revisions sequencer.c:5178\n\t    #6 0x4c5a17 in run_sequencer builtin/revert.c:237\n\t    #7 0x4c5aa9 in cmd_revert builtin/revert.c:247\n\t    #8 0x407a32 in run_builtin git.c:466\n\t    #9 0x407e0a in handle_builtin git.c:721\n\t    #10 0x40803d in run_argv git.c:788\n\t    #11 0x40850f in cmd_main git.c:923\n\t    #12 0x4eedcc in main common-main.c:57\n\t    #13 0x7f7cc4f3f209 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58\n\t    #14 0x7f7cc4f3f2bb in __libc_start_main_impl ../csu/libc-start.c:389\n\t    #15 0x405fd0 in _start (git+0x405fd0)\n\nBut more to the point, if you run the test suite with e.g.:\n\n\tGIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true\n\nYou can find these raw reports in:\n\n\tgrep -r sequencer test-results/*.leak\n\nOr, from my github.com/avar/git.git use this nice script/alias to\nsummarize it (I haven't upstreamed this yet):\n\n\t$ git help scan-leaks-top\n\t'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'\n\n>> I'm not sure how, opts->reflog_message will be a copy of\n>> opts->reflog_action which is freed at the end of the rebase. I'll\n>> have a proper look tomorrow to see if I'm missing something.\n>\n> So it is possible this is showing up because I think we only free the\n> heap allocated members of opts in sequencer_remove_state() and that is \n> not called when we stop for a conflict resolution, a break command, a\n> failed exec or a rescheduled pick/reset etc. The way to fix that would \n> be to refactor sequencer_remove_state() to create\n> replay_opts_release() and call that from builtin/{revert,rebase}.c\n\nYes, I think that's probably the root cause. I have a leak-fixing topic\nas a follow-up to my current one, which among other things tried to\naddress this: https://github.com/avar/git/commit/7a150d1b7e2\n\nI'd just forgot about it. That link currently says committed <24hrs ago,\nbut I was just rebasing the topic for something unrelated, I hacked this\nup in mid-August.\n\n> As that is unrelated to removing the setenv() calls which is the focus\n> of this series I will not be doing that in this series.\n\nI'm fine with us leaving this for now, and saying that it's OK that\nwe're adding some new leaks, if we're addressing the setenv/getenv\nissue, and that we can fix the root cause of the current leaks later.\n\nBut let's be clear: It's not unrelated to your refactoring in this\ntopic, we didn't have this leak before, and now we have it. These two\npatches are the cause of some new leaks we didn't have before.\n\nAnd, if we run this on my topic which narrowly attempted to fix these\nleaks e.g. that \"t3425-rebase-topology-merges.sh\" test will have just 1\nleak in that failing test, v.s. 3 leaks with this topic (the \"4 byte(s)\nin 1 object(s)\" above).\n\nIt's just a nice coincidence that our memory leaks are currently in such\na sorry state overall that this isn't failing e.g. the linux-leaks CI,\nbecause the new leaks are being masked by tests that area already\nfailing due to other pre-existing leaks.\n\nBut all that being said I think the right move is for this topic to\nproceed, perhaps with an updated commit message noting some of this.\n\nIt's really just running into the existing problem of\nreplay_opts_release(). If that destructor isn't reliable (which it\nisn't) we can still make new use of it, and then fix how we call it\nlater for all its callers.\n\nWhich I've just tested: I cherry-pick that 7a150d1b7e2 and the few\npreceding commits it needs (dcc104aef89..7a150d1b7e2), and then apply\nthis on top:\n\t\n\tdiff --git a/builtin/revert.c b/builtin/revert.c\n\tindex ee32c714a76..0abd805beed 100644\n\t--- a/builtin/revert.c\n\t+++ b/builtin/revert.c\n\t@@ -250,6 +250,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n\t \tif (opts.revs)\n\t \t\trelease_revisions(opts.revs);\n\t \tfree(opts.revs);\n\t+\treplay_opts_release(&opts);\n\t \treturn res;\n\t }\n\nThe leaks above are down to just the unrelated parse_options_concat()\nleak. I.e. this really is just a case of us missing the destructor due\nto a more general issue.\n\n1. https://lore.kernel.org/git/8eec228d-d392-523d-2415-149b946f642e@dunelm.org.uk/\n"},{"id":"466955","messageId":"pull.1405.v2.git.1668003718.gitgitgadget@gmail.com","threadId":"58747","inReplyTo":"pull.1405.git.1667575142.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] rebase: stop setting GIT_REFLOG_ACTION","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-09T14:21:56Z","receivedAt":"2022-11-09T14:24:28Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"This is a follow up to pw/rebase-reflog-fixes that moves away from using\nGIT_REFLOG_ACTION internally.\n\nThanks to Taylor & Ævar for their comments on V1. I've updated the commit\nmessage of patch 1 as suggested by Taylor, the code is unchanged.\n\nPhillip Wood (2):\n  sequencer: stop exporting GIT_REFLOG_ACTION\n  rebase: stop exporting GIT_REFLOG_ACTION\n\n builtin/rebase.c | 27 +++++++++++++++------------\n sequencer.c      | 45 +++++++++++++++++++++++++--------------------\n sequencer.h      |  6 ++++++\n 3 files changed, 46 insertions(+), 32 deletions(-)\n\n\nbase-commit: 3b08839926fcc7cc48cf4c759737c1a71af430c1\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1405%2Fphillipwood%2Fmore-rebase-reflog-fixes-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1405/phillipwood/more-rebase-reflog-fixes-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1405\n\nRange-diff vs v1:\n\n 1:  e9c3f5ac5c6 ! 1:  655b4e89f59 sequencer: stop exporting GIT_REFLOG_ACTION\n     @@ Commit message\n          pass the reflog action around in a variable and use it to set\n          GIT_REFLOG_ACTION in the child environment when running \"git commit\".\n      \n     +    Within the sequencer GIT_REFLOG_ACTION is no longer set and is only read\n     +    by sequencer_reflog_action(). It is still set by rebase before calling\n     +    the sequencer, that will be addressed in the next commit. cherry-pick\n     +    and revert are unaffected as they do not set GIT_REFLOG_ACTION before\n     +    calling the sequencer.\n     +\n          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n      \n       ## sequencer.c ##\n 2:  d3747bcc8d1 = 2:  31df037eafe rebase: stop exporting GIT_REFLOG_ACTION\n\n-- \ngitgitgadget\n"},{"id":"466956","messageId":"655b4e89f596b397865b67b58656f380f27d02fc.1668003719.git.gitgitgadget@gmail.com","threadId":"58747","inReplyTo":"pull.1405.v2.git.1668003718.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] sequencer: stop exporting GIT_REFLOG_ACTION","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-09T14:21:57Z","receivedAt":"2022-11-09T14:24:30Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nEach time it picks a commit the sequencer copies the GIT_REFLOG_ACITON\nenvironment variable so it can temporarily change it and then restore\nthe previous value. This results in code that is hard to follow and also\nleaks memory because (i) we fail to free the copy when we've finished\nwith it and (ii) each call to setenv() leaks the previous value. Instead\npass the reflog action around in a variable and use it to set\nGIT_REFLOG_ACTION in the child environment when running \"git commit\".\n\nWithin the sequencer GIT_REFLOG_ACTION is no longer set and is only read\nby sequencer_reflog_action(). It is still set by rebase before calling\nthe sequencer, that will be addressed in the next commit. cherry-pick\nand revert are unaffected as they do not set GIT_REFLOG_ACTION before\ncalling the sequencer.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c | 45 +++++++++++++++++++++++++--------------------\n sequencer.h |  6 ++++++\n 2 files changed, 31 insertions(+), 20 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex e658df7e8ff..e23f6f0b718 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -375,6 +375,7 @@ int sequencer_remove_state(struct replay_opts *opts)\n \t}\n \n \tfree(opts->gpg_sign);\n+\tfree(opts->reflog_action);\n \tfree(opts->default_strategy);\n \tfree(opts->strategy);\n \tfor (i = 0; i < opts->xopts_nr; i++)\n@@ -1050,6 +1051,8 @@ static int run_git_commit(const char *defmsg,\n \t\t\t     gpg_opt, gpg_opt);\n \t}\n \n+\tstrvec_pushf(&cmd.env, GIT_REFLOG_ACTION \"=%s\", opts->reflog_message);\n+\n \tif (opts->committer_date_is_author_date)\n \t\tstrvec_pushf(&cmd.env, \"GIT_COMMITTER_DATE=%s\",\n \t\t\t     opts->ignore_date ?\n@@ -1589,8 +1592,8 @@ static int try_to_commit(struct repository *r,\n \t\tgoto out;\n \t}\n \n-\tif (update_head_with_reflog(current_head, oid,\n-\t\t\t\t    getenv(\"GIT_REFLOG_ACTION\"), msg, &err)) {\n+\tif (update_head_with_reflog(current_head, oid, opts->reflog_message,\n+\t\t\t\t    msg, &err)) {\n \t\tres = error(\"%s\", err.buf);\n \t\tgoto out;\n \t}\n@@ -3674,17 +3677,28 @@ static int do_label(struct repository *r, const char *name, int len)\n \treturn ret;\n }\n \n+static const char *sequencer_reflog_action(struct replay_opts *opts)\n+{\n+\tif (!opts->reflog_action) {\n+\t\topts->reflog_action = getenv(GIT_REFLOG_ACTION);\n+\t\topts->reflog_action =\n+\t\t\txstrdup(opts->reflog_action ? opts->reflog_action\n+\t\t\t\t\t\t    : action_name(opts));\n+\t}\n+\n+\treturn opts->reflog_action;\n+}\n+\n __attribute__((format (printf, 3, 4)))\n static const char *reflog_message(struct replay_opts *opts,\n \tconst char *sub_action, const char *fmt, ...)\n {\n \tva_list ap;\n \tstatic struct strbuf buf = STRBUF_INIT;\n-\tchar *reflog_action = getenv(GIT_REFLOG_ACTION);\n \n \tva_start(ap, fmt);\n \tstrbuf_reset(&buf);\n-\tstrbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));\n+\tstrbuf_addstr(&buf, sequencer_reflog_action(opts));\n \tif (sub_action)\n \t\tstrbuf_addf(&buf, \" (%s)\", sub_action);\n \tif (fmt) {\n@@ -4497,7 +4511,7 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,\n \t\t\t\tRESET_HEAD_RUN_POST_CHECKOUT_HOOK,\n \t\t.head_msg = reflog_message(opts, \"start\", \"checkout %s\",\n \t\t\t\t\t   onto_name),\n-\t\t.default_reflog_action = \"rebase\"\n+\t\t.default_reflog_action = sequencer_reflog_action(opts)\n \t};\n \tif (reset_head(r, &ropts)) {\n \t\tapply_autostash(rebase_path_autostash());\n@@ -4566,11 +4580,8 @@ static int pick_commits(struct repository *r,\n \t\t\tstruct replay_opts *opts)\n {\n \tint res = 0, reschedule = 0;\n-\tchar *prev_reflog_action;\n \n-\t/* Note that 0 for 3rd parameter of setenv means set only if not set */\n-\tsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n-\tprev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));\n+\topts->reflog_message = sequencer_reflog_action(opts);\n \tif (opts->allow_ff)\n \t\tassert(!(opts->signoff || opts->no_commit ||\n \t\t\t opts->record_origin || should_edit(opts) ||\n@@ -4618,14 +4629,12 @@ static int pick_commits(struct repository *r,\n \t\t}\n \t\tif (item->command <= TODO_SQUASH) {\n \t\t\tif (is_rebase_i(opts))\n-\t\t\t\tsetenv(GIT_REFLOG_ACTION, reflog_message(opts,\n-\t\t\t\t\tcommand_to_string(item->command), NULL),\n-\t\t\t\t\t1);\n+\t\t\t\topts->reflog_message = reflog_message(opts,\n+\t\t\t\t      command_to_string(item->command), NULL);\n+\n \t\t\tres = do_pick_commit(r, item, opts,\n \t\t\t\t\t     is_final_fixup(todo_list),\n \t\t\t\t\t     &check_todo);\n-\t\t\tif (is_rebase_i(opts))\n-\t\t\t\tsetenv(GIT_REFLOG_ACTION, prev_reflog_action, 1);\n \t\t\tif (is_rebase_i(opts) && res < 0) {\n \t\t\t\t/* Reschedule */\n \t\t\t\tadvise(_(rescheduled_advice),\n@@ -5050,8 +5059,6 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)\n \tif (read_populate_opts(opts))\n \t\treturn -1;\n \tif (is_rebase_i(opts)) {\n-\t\tchar *previous_reflog_action;\n-\n \t\tif ((res = read_populate_todo(r, &todo_list, opts)))\n \t\t\tgoto release_todo_list;\n \n@@ -5062,13 +5069,11 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)\n \t\t\tunlink(rebase_path_dropped());\n \t\t}\n \n-\t\tprevious_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));\n-\t\tsetenv(GIT_REFLOG_ACTION, reflog_message(opts, \"continue\", NULL), 1);\n+\t\topts->reflog_message = reflog_message(opts, \"continue\", NULL);\n \t\tif (commit_staged_changes(r, opts, &todo_list)) {\n \t\t\tres = -1;\n \t\t\tgoto release_todo_list;\n \t\t}\n-\t\tsetenv(GIT_REFLOG_ACTION, previous_reflog_action, 1);\n \t} else if (!file_exists(get_todo_path(opts)))\n \t\treturn continue_single_pick(r, opts);\n \telse if ((res = read_populate_todo(r, &todo_list, opts)))\n@@ -5116,7 +5121,7 @@ static int single_pick(struct repository *r,\n \t\t\tTODO_PICK : TODO_REVERT;\n \titem.commit = cmit;\n \n-\tsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n+\topts->reflog_message = sequencer_reflog_action(opts);\n \treturn do_pick_commit(r, &item, opts, 0, &check_todo);\n }\n \ndiff --git a/sequencer.h b/sequencer.h\nindex 563fe599334..888c18aad71 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -63,6 +63,9 @@ struct replay_opts {\n \tchar **xopts;\n \tsize_t xopts_nr, xopts_alloc;\n \n+\t/* Reflog */\n+\tchar *reflog_action;\n+\n \t/* Used by fixup/squash */\n \tstruct strbuf current_fixups;\n \tint current_fixup_count;\n@@ -73,6 +76,9 @@ struct replay_opts {\n \n \t/* Only used by REPLAY_NONE */\n \tstruct rev_info *revs;\n+\n+\t/* Private use */\n+\tconst char *reflog_message;\n };\n #define REPLAY_OPTS_INIT { .edit = -1, .action = -1, .current_fixups = STRBUF_INIT }\n \n-- \ngitgitgadget\n\n"},{"id":"466957","messageId":"31df037eafede799c2ef27df66c6da309b719b1b.1668003719.git.gitgitgadget@gmail.com","threadId":"58747","inReplyTo":"pull.1405.v2.git.1668003718.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] rebase: stop exporting GIT_REFLOG_ACTION","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-09T14:21:58Z","receivedAt":"2022-11-09T14:24:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nNow that struct replay_opts has a reflog_action member we no longer\nneed to export GIT_REFLOG_ACTION when starting a rebase. If the user\nhas set GIT_REFLOG_ACTION then we use it when initializing\nreflog_action.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n builtin/rebase.c | 27 +++++++++++++++------------\n 1 file changed, 15 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 5d855fd8f51..4d6839a5785 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -30,8 +30,6 @@\n #include \"reset.h\"\n #include \"hook.h\"\n \n-#define DEFAULT_REFLOG_ACTION \"rebase\"\n-\n static char const * const builtin_rebase_usage[] = {\n \tN_(\"git rebase [-i] [options] [--exec <cmd>] \"\n \t\t\"[--onto <newbase> | --keep-base] [<upstream> [<branch>]]\"),\n@@ -106,6 +104,7 @@ struct rebase_options {\n \t} flags;\n \tstruct strvec git_am_opts;\n \tenum action action;\n+\tchar *reflog_action;\n \tint signoff;\n \tint allow_rerere_autoupdate;\n \tint keep_empty;\n@@ -159,6 +158,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n \t\t\t\t\topts->committer_date_is_author_date;\n \treplay.ignore_date = opts->ignore_date;\n \treplay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);\n+\treplay.reflog_action = xstrdup(opts->reflog_action);\n \tif (opts->strategy)\n \t\treplay.strategy = xstrdup_or_null(opts->strategy);\n \telse if (!replay.strategy && replay.default_strategy) {\n@@ -585,10 +585,10 @@ static int move_to_original_branch(struct rebase_options *opts)\n \t\tBUG(\"move_to_original_branch without onto\");\n \n \tstrbuf_addf(&branch_reflog, \"%s (finish): %s onto %s\",\n-\t\t    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),\n+\t\t    opts->reflog_action,\n \t\t    opts->head_name, oid_to_hex(&opts->onto->object.oid));\n \tstrbuf_addf(&head_reflog, \"%s (finish): returning to %s\",\n-\t\t    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), opts->head_name);\n+\t\t    opts->reflog_action, opts->head_name);\n \tropts.branch = opts->head_name;\n \tropts.flags = RESET_HEAD_REFS_ONLY;\n \tropts.branch_msg = branch_reflog.buf;\n@@ -618,7 +618,7 @@ static int run_am(struct rebase_options *opts)\n \tam.git_cmd = 1;\n \tstrvec_push(&am.args, \"am\");\n \tstrvec_pushf(&am.env, GIT_REFLOG_ACTION_ENVIRONMENT \"=%s (pick)\",\n-\t\t     getenv(GIT_REFLOG_ACTION_ENVIRONMENT));\n+\t\t     opts->reflog_action);\n \tif (opts->action == ACTION_CONTINUE) {\n \t\tstrvec_push(&am.args, \"--resolved\");\n \t\tstrvec_pushf(&am.args, \"--resolvemsg=%s\", resolvemsg);\n@@ -685,7 +685,7 @@ static int run_am(struct rebase_options *opts)\n \n \t\tropts.oid = &opts->orig_head->object.oid;\n \t\tropts.branch = opts->head_name;\n-\t\tropts.default_reflog_action = DEFAULT_REFLOG_ACTION;\n+\t\tropts.default_reflog_action = opts->reflog_action;\n \t\treset_head(the_repository, &ropts);\n \t\terror(_(\"\\ngit encountered an error while preparing the \"\n \t\t\t\"patches to replay\\n\"\n@@ -834,8 +834,7 @@ static int checkout_up_to_date(struct rebase_options *options)\n \tint ret = 0;\n \n \tstrbuf_addf(&buf, \"%s: checkout %s\",\n-\t\t    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),\n-\t\t    options->switch_to);\n+\t\t    options->reflog_action, options->switch_to);\n \tropts.oid = &options->orig_head->object.oid;\n \tropts.branch = options->head_name;\n \tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n@@ -1243,7 +1242,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \n \tif (options.action != ACTION_NONE && !in_progress)\n \t\tdie(_(\"No rebase in progress?\"));\n-\tsetenv(GIT_REFLOG_ACTION_ENVIRONMENT, \"rebase\", 0);\n \n \tif (options.action == ACTION_EDIT_TODO && !is_merge(&options))\n \t\tdie(_(\"The --edit-todo action can only be used during \"\n@@ -1258,6 +1256,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\ttrace2_cmd_mode(action_names[options.action]);\n \t}\n \n+\toptions.reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);\n+\toptions.reflog_action =\n+\t\txstrdup(options.reflog_action ? options.reflog_action : \"rebase\");\n+\n \tswitch (options.action) {\n \tcase ACTION_CONTINUE: {\n \t\tstruct object_id head;\n@@ -1310,7 +1312,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\texit(1);\n \n \t\tstrbuf_addf(&head_msg, \"%s (abort): returning to %s\",\n-\t\t\t    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),\n+\t\t\t    options.reflog_action,\n \t\t\t    options.head_name ? options.head_name\n \t\t\t\t\t      : oid_to_hex(&options.orig_head->object.oid));\n \t\tropts.oid = &options.orig_head->object.oid;\n@@ -1786,13 +1788,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t \"it...\\n\"));\n \n \tstrbuf_addf(&msg, \"%s (start): checkout %s\",\n-\t\t    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);\n+\t\t    options.reflog_action, options.onto_name);\n \tropts.oid = &options.onto->object.oid;\n \tropts.orig_head = &options.orig_head->object.oid,\n \tropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |\n \t\t\tRESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n \tropts.head_msg = msg.buf;\n-\tropts.default_reflog_action = DEFAULT_REFLOG_ACTION;\n+\tropts.default_reflog_action = options.reflog_action;\n \tif (reset_head(the_repository, &ropts))\n \t\tdie(_(\"Could not detach HEAD\"));\n \tstrbuf_release(&msg);\n@@ -1824,6 +1826,7 @@ run_rebase:\n cleanup:\n \tstrbuf_release(&buf);\n \tstrbuf_release(&revisions);\n+\tfree(options.reflog_action);\n \tfree(options.head_name);\n \tfree(options.gpg_sign_opt);\n \tfree(options.cmd);\n-- \ngitgitgadget\n"},{"id":"466965","messageId":"221109.86leokp0vm.gmgdl@evledraar.gmail.com","threadId":"58747","inReplyTo":"pull.1405.v2.git.1668003718.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] rebase: stop setting GIT_REFLOG_ACTION","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-09T16:05:24Z","receivedAt":"2022-11-09T16:07:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Nov 09 2022, Phillip Wood via GitGitGadget wrote:\n\n> This is a follow up to pw/rebase-reflog-fixes that moves away from using\n> GIT_REFLOG_ACTION internally.\n>\n> Thanks to Taylor & Ævar for their comments on V1. I've updated the commit\n> message of patch 1 as suggested by Taylor, the code is unchanged.\n>\n> Phillip Wood (2):\n>   sequencer: stop exporting GIT_REFLOG_ACTION\n>   rebase: stop exporting GIT_REFLOG_ACTION\n>\n>  builtin/rebase.c | 27 +++++++++++++++------------\n>  sequencer.c      | 45 +++++++++++++++++++++++++--------------------\n>  sequencer.h      |  6 ++++++\n>  3 files changed, 46 insertions(+), 32 deletions(-)\n>\n>\n> base-commit: 3b08839926fcc7cc48cf4c759737c1a71af430c1\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1405%2Fphillipwood%2Fmore-rebase-reflog-fixes-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1405/phillipwood/more-rebase-reflog-fixes-v2\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1405\n>\n> Range-diff vs v1:\n>\n>  1:  e9c3f5ac5c6 ! 1:  655b4e89f59 sequencer: stop exporting GIT_REFLOG_ACTION\n>      @@ Commit message\n>           pass the reflog action around in a variable and use it to set\n>           GIT_REFLOG_ACTION in the child environment when running \"git commit\".\n>       \n>      +    Within the sequencer GIT_REFLOG_ACTION is no longer set and is only read\n>      +    by sequencer_reflog_action(). It is still set by rebase before calling\n>      +    the sequencer, that will be addressed in the next commit. cherry-pick\n>      +    and revert are unaffected as they do not set GIT_REFLOG_ACTION before\n>      +    calling the sequencer.\n>      +\n>           Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n>       \n>        ## sequencer.c ##\n>  2:  d3747bcc8d1 = 2:  31df037eafe rebase: stop exporting GIT_REFLOG_ACTION\n\nThanks, FWIW I'm happy to give this my \"Reviewed-by\", per [1] I've\nlooked this over carefully.\n\nThe tl;dr of that is that this fixes a leak, and adds another one, but\nthe root cause of the added one is that you're using an existing\ndestructor that we sometimes don't call, which we can just address as a\nfollow-up generic issue (I've got patches to fix it).\n\nBut for now this is a good step forward, and fixes the leak that's\n\"unique\" t this codepath.\n\nAnd of course, just makes managing the \"reflog\" variable nicer in\ngeneral, as we're no longer talking to ourselves within the same process\nwith setenv()/getenv().\n\n1. https://lore.kernel.org/git/221108.864jv9sc9r.gmgdl@evledraar.gmail.com/\n"},{"id":"466968","messageId":"28100f66-bd3d-d564-cb40-44b65ef415bc@dunelm.org.uk","threadId":"58747","inReplyTo":"221109.86leokp0vm.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 0/2] rebase: stop setting GIT_REFLOG_ACTION","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-11-09T16:30:47Z","receivedAt":"2022-11-09T16:30:58Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 09/11/2022 16:05, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Nov 09 2022, Phillip Wood via GitGitGadget wrote:\n> \n>> This is a follow up to pw/rebase-reflog-fixes that moves away from using\n>> GIT_REFLOG_ACTION internally.\n>>\n>> Thanks to Taylor & Ævar for their comments on V1. I've updated the commit\n>> message of patch 1 as suggested by Taylor, the code is unchanged.\n>>\n>> Phillip Wood (2):\n>>    sequencer: stop exporting GIT_REFLOG_ACTION\n>>    rebase: stop exporting GIT_REFLOG_ACTION\n>>\n>>   builtin/rebase.c | 27 +++++++++++++++------------\n>>   sequencer.c      | 45 +++++++++++++++++++++++++--------------------\n>>   sequencer.h      |  6 ++++++\n>>   3 files changed, 46 insertions(+), 32 deletions(-)\n>>\n>>\n>> base-commit: 3b08839926fcc7cc48cf4c759737c1a71af430c1\n>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1405%2Fphillipwood%2Fmore-rebase-reflog-fixes-v2\n>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1405/phillipwood/more-rebase-reflog-fixes-v2\n>> Pull-Request: https://github.com/gitgitgadget/git/pull/1405\n>>\n>> Range-diff vs v1:\n>>\n>>   1:  e9c3f5ac5c6 ! 1:  655b4e89f59 sequencer: stop exporting GIT_REFLOG_ACTION\n>>       @@ Commit message\n>>            pass the reflog action around in a variable and use it to set\n>>            GIT_REFLOG_ACTION in the child environment when running \"git commit\".\n>>        \n>>       +    Within the sequencer GIT_REFLOG_ACTION is no longer set and is only read\n>>       +    by sequencer_reflog_action(). It is still set by rebase before calling\n>>       +    the sequencer, that will be addressed in the next commit. cherry-pick\n>>       +    and revert are unaffected as they do not set GIT_REFLOG_ACTION before\n>>       +    calling the sequencer.\n>>       +\n>>            Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>        \n>>         ## sequencer.c ##\n>>   2:  d3747bcc8d1 = 2:  31df037eafe rebase: stop exporting GIT_REFLOG_ACTION\n> \n> Thanks, FWIW I'm happy to give this my \"Reviewed-by\", per [1] I've\n> looked this over carefully.\n> \n> The tl;dr of that is that this fixes a leak, and adds another one, but\n> the root cause of the added one is that you're using an existing\n> destructor that we sometimes don't call, which we can just address as a\n> follow-up generic issue (I've got patches to fix it).\n\nIt is worth noting that the leak that is fixed was unbounded and grew \nwith each commit whereas the new one is a fixed size and is only a \n\"leak\" because we don't clean up properly when exiting.\n\n> But for now this is a good step forward, and fixes the leak that's\n> \"unique\" t this codepath.\n> \n> And of course, just makes managing the \"reflog\" variable nicer in\n> general, as we're no longer talking to ourselves within the same process\n> with setenv()/getenv().\n\nYes, that was the main aim really\n\nBest Wishes\n\nPhillip\n\n> 1. https://lore.kernel.org/git/221108.864jv9sc9r.gmgdl@evledraar.gmail.com/\n"},{"id":"467005","messageId":"Y2w07ci8Lg0GQXSQ@nand.local","threadId":"58747","inReplyTo":"28100f66-bd3d-d564-cb40-44b65ef415bc@dunelm.org.uk","subject":"Re: [PATCH v2 0/2] rebase: stop setting GIT_REFLOG_ACTION","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-09T23:17:01Z","receivedAt":"2022-11-09T23:17:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 09, 2022 at 04:30:47PM +0000, Phillip Wood wrote:\n> > But for now this is a good step forward, and fixes the leak that's\n> > \"unique\" t this codepath.\n> >\n> > And of course, just makes managing the \"reflog\" variable nicer in\n> > general, as we're no longer talking to ourselves within the same process\n> > with setenv()/getenv().\n>\n> Yes, that was the main aim really\n\nThanks, both. Let's start merging this one down.\n\nThanks,\nTaylor\n"}]}