From: Phillip Wood Date: Thu, 02 Apr 2020 19:05:17 GMT Subject: Re: [PATCH] sequencer: honor GIT_REFLOG_ACTION Message-ID: <09397e37-a22b-5159-b760-bae238ae3ed6@gmail.com> In-Reply-To: Hi Elijah On 02/04/2020 18:01, Elijah Newren wrote: > Hi Phillip, > > On Thu, Apr 2, 2020 at 2:25 AM Phillip Wood wrote: >> >> Hi Elijah >> >> Thanks for fixing this >> >> On 01/04/2020 21:31, Elijah Newren via GitGitGadget wrote: >>> From: Elijah Newren >>> >>> There is a lot of code to honor GIT_REFLOG_ACTION throughout git, >>> including some in sequencer.c; unfortunately, reflog_message() and its >>> callers ignored it. Instruct reflog_message() to check the existing >>> environment variable, and use it when present as an override to >>> action_name(). >>> >>> Also restructure pick_commits() to only temporarily modify >>> GIT_REFLOG_ACTION for a short duration and then restore the old value, >>> so that when we do this setting within a loop we do not keep adding " >>> (pick)" substrings and end up with a reflog message of the form >>> rebase (pick) (pick) (pick) (finish): returning to refs/heads/master >>> >>> Signed-off-by: Elijah Newren >>> --- >>> sequencer: honor GIT_REFLOG_ACTION >>> >>> I'm not the best with getenv/setenv. The xstrdup() wrapping is >>> apparently necessary on mac and bsd. The xstrdup seems like it leaves us >>> with a memory leak, but since setenv(3) says to not alter or free it, I >>> think it's right. Anyone have any alternative suggestions? >>> >>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-746%2Fnewren%2Fhonor-reflog-action-in-sequencer-v1 >>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-746/newren/honor-reflog-action-in-sequencer-v1 >>> Pull-Request: https://github.com/git/git/pull/746 >>> >>> sequencer.c | 9 +++++++-- >>> t/t3406-rebase-message.sh | 16 ++++++++-------- >>> 2 files changed, 15 insertions(+), 10 deletions(-) >>> >>> diff --git a/sequencer.c b/sequencer.c >>> index e528225e787..5837fdaabbe 100644 >>> --- a/sequencer.c >>> +++ b/sequencer.c >>> @@ -3708,10 +3708,11 @@ static const char *reflog_message(struct replay_opts *opts, >>> { >>> va_list ap; >>> static struct strbuf buf = STRBUF_INIT; >>> + char *reflog_action = getenv("GIT_REFLOG_ACTION"); >> >> Minor nit - you're using a string here rather that the pre-processor >> constant that is used below > > Yeah, true. However, using a mixture of both styles is consistent > with the current code's inconsistency about which one should be used. > :-) Nice! >>> va_start(ap, fmt); >>> strbuf_reset(&buf); >>> - strbuf_addstr(&buf, action_name(opts)); >>> + strbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts)); >>> if (sub_action) >>> strbuf_addf(&buf, " (%s)", sub_action); >>> if (fmt) { >>> @@ -3799,8 +3800,10 @@ static int pick_commits(struct repository *r, >>> struct replay_opts *opts) >>> { >>> int res = 0, reschedule = 0; >>> + char *prev_reflog_action; >>> >>> setenv(GIT_REFLOG_ACTION, action_name(opts), 0); >>> + prev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION)); >> >> I'm confused as to why saving the environment variable immediately after >> setting it works but the test shows it does - why doesn't this clobber >> the value of GIT_REFLOG_ACTION set by the user? > > The third parameter, 0, means only set the environment variable if > it's not already set. Ah thanks, I thought I must be missing something fairly obvious but couldn't see what it was Best Wishes Phillip