Hi Elijah
On 02/04/2020 18:01, Elijah Newren wrote:
Hi Phillip,
On Thu, Apr 2, 2020 at 2:25 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
Hi Elijah
Thanks for fixing this
On 01/04/2020 21:31, Elijah Newren via GitGitGadget wrote:
From: Elijah Newren <newren@xxxxxxxxx>
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@xxxxxxxxx>
---
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
