From: Phillip Wood <phillip.wood123@gmail•com>
To: Elijah Newren via GitGitGadget <gitgitgadget@gmail•com>,
git@vger•kernel.org
Cc: jrnieder@gmail•com, elbrus@debian•org,
ijackson@chiark•greenend.org.uk, phillip.wood@dunelm•org.uk,
alban.gruin@gmail•com, Johannes.Schindelin@gmx•de,
Elijah Newren <newren@gmail•com>
Subject: Re: [PATCH] sequencer: honor GIT_REFLOG_ACTION
Date: Thu, 2 Apr 2020 10:25:29 +0100 [thread overview]
Message-ID: <b187cb5f-a6c8-2908-e3fd-e1210e6970e0@gmail.com> (raw)
In-Reply-To: <pull.746.git.git.1585773096145.gitgitgadget@gmail.com>
Hi Elijah
Thanks for fixing this
On 01/04/2020 21:31, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail•com>
>
> 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 <newren@gmail•com>
> ---
> 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
> 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?
Best Wishes
Phillip
> if (opts->allow_ff)
> assert(!(opts->signoff || opts->no_commit ||
> opts->record_origin || opts->edit));
> @@ -3845,12 +3848,14 @@ static int pick_commits(struct repository *r,
> }
> if (item->command <= TODO_SQUASH) {
> if (is_rebase_i(opts))
> - setenv("GIT_REFLOG_ACTION", reflog_message(opts,
> + setenv(GIT_REFLOG_ACTION, reflog_message(opts,
> command_to_string(item->command), NULL),
> 1);
> res = do_pick_commit(r, item->command, item->commit,
> 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),
> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> index 61b76f33019..927a4f4a4e4 100755
> --- a/t/t3406-rebase-message.sh
> +++ b/t/t3406-rebase-message.sh
> @@ -89,22 +89,22 @@ test_expect_success 'GIT_REFLOG_ACTION' '
> git checkout -b reflog-topic start &&
> test_commit reflog-to-rebase &&
>
> - git rebase --apply reflog-onto &&
> + git rebase reflog-onto &&
> git log -g --format=%gs -3 >actual &&
> cat >expect <<-\EOF &&
> - rebase finished: returning to refs/heads/reflog-topic
> - rebase: reflog-to-rebase
> - rebase: checkout reflog-onto
> + rebase (finish): returning to refs/heads/reflog-topic
> + rebase (pick): reflog-to-rebase
> + rebase (start): checkout reflog-onto
> EOF
> test_cmp expect actual &&
>
> git checkout -b reflog-prefix reflog-to-rebase &&
> - GIT_REFLOG_ACTION=change-the-reflog git rebase --apply reflog-onto &&
> + GIT_REFLOG_ACTION=change-the-reflog git rebase reflog-onto &&
> git log -g --format=%gs -3 >actual &&
> cat >expect <<-\EOF &&
> - rebase finished: returning to refs/heads/reflog-prefix
> - change-the-reflog: reflog-to-rebase
> - change-the-reflog: checkout reflog-onto
> + change-the-reflog (finish): returning to refs/heads/reflog-prefix
> + change-the-reflog (pick): reflog-to-rebase
> + change-the-reflog (start): checkout reflog-onto
> EOF
> test_cmp expect actual
> '
>
> base-commit: 274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925
>
next prev parent reply other threads:[~2020-04-02 9:25 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-01 20:31 [PATCH] sequencer: honor GIT_REFLOG_ACTION Elijah Newren via GitGitGadget
2020-04-01 20:46 ` Junio C Hamano
2020-04-01 23:29 ` Ian Jackson
2020-04-02 5:15 ` Elijah Newren
2020-04-02 9:39 ` Phillip Wood
2020-04-02 17:40 ` Elijah Newren
2020-04-02 9:25 ` Phillip Wood [this message]
2020-04-02 17:01 ` Elijah Newren
2020-04-02 19:05 ` Phillip Wood
2020-04-07 14:50 ` Johannes Schindelin
2020-04-07 15:18 ` Elijah Newren
2020-04-07 22:37 ` Johannes Schindelin
2020-04-07 23:05 ` Junio C Hamano
2020-04-07 16:59 ` [PATCH v2] " Elijah Newren via GitGitGadget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b187cb5f-a6c8-2908-e3fd-e1210e6970e0@gmail.com \
--to=phillip.wood123@gmail$(echo .)com \
--cc=Johannes.Schindelin@gmx$(echo .)de \
--cc=alban.gruin@gmail$(echo .)com \
--cc=elbrus@debian$(echo .)org \
--cc=git@vger$(echo .)kernel.org \
--cc=gitgitgadget@gmail$(echo .)com \
--cc=ijackson@chiark$(echo .)greenend.org.uk \
--cc=jrnieder@gmail$(echo .)com \
--cc=newren@gmail$(echo .)com \
--cc=phillip.wood@dunelm$(echo .)org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox