sequencer: honor GIT_REFLOG_ACTION

From: Elijah Newren <[email protected]>

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,

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?

Hi. Thanks for looking at this.

Elijah Newren via GitGitGadget writes ("[PATCH] sequencer: honor GIT_REFLOG_ACTION"):

Quoted 4 lines

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?

I can try to help. It's not entirely trivial.

The setenv interface is a wrapper around putenv. putenv has had a variety of different semantics. Some of these sets of semantics cannot be used to re-set the same environment variable without a memory leak - and even figuring out what semantics you have would be complex and tend to produce code which would fail in bad ways. There's a short summary of the situation in Linux's putenv(3).

Would it be possible for git to arrange to set GIT_REFLOG_ACTION only when it is invoking subprocesses ? Otherwise it would update, and look at, a global variable of its own. (Or a parameter to relevant functions if one doesn't like the action-at-a-distance effect of a global.)

And, it seems to me that the reflog handling should be centralised.

sequencer.c static const char *reflog_message(struct replay_opts *opts, · line 3715

Quoted 6 lines

sequencer.c +2 -1

+     char *reflog_action = getenv("GIT_REFLOG_ACTION");
 
      va_start(ap, fmt);
      strbuf_reset(&buf);
-     strbuf_addstr(&buf, action_name(opts));
+     strbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));

Open coding this kind of thing at every site which needs to think about the reflog actions will surely result in some of the instances having bugs.

Writing a single function that contans this (or most of it) would happily decouple all of its call sites from literally asking about getenv("GIT_REFLOG_ACTION") thereby making it easier to do the indirection-through-program-variables I suggest.

Having said that,

t/t3406-rebase-message.sh

Quoted 4 lines

t/t3406-rebase-message.sh +0 -0

index 61b76f33019..927a4f4a4e4 100755

This test case convinces me that the patch has the right behaviour for at least the case I care about :-).

From: Elijah Newren <[email protected]>

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 <[email protected]>

---
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(-)

sequencer.c +0 -0

index e528225e787..5837fdaabbe 100644

sequencer.c +1 -1

  	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) {

sequencer.c +3 -1

  	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),

t/t3406-rebase-message.sh +8 -8

index 61b76f33019..927a4f4a4e4 100755
@@ -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

Quoted 36 lines · Ian Jackson

Hi. Thanks for looking at this.

Elijah Newren via GitGitGadget writes ("[PATCH] sequencer: honor GIT_REFLOG_ACTION"):

Quoted 4 lines

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?

I can try to help. It's not entirely trivial.

The setenv interface is a wrapper around putenv. putenv has had a variety of different semantics. Some of these sets of semantics cannot be used to re-set the same environment variable without a memory leak - and even figuring out what semantics you have would be complex and tend to produce code which would fail in bad ways. There's a short summary of the situation in Linux's putenv(3).

Would it be possible for git to arrange to set GIT_REFLOG_ACTION only
when it is invoking subprocesses ? Otherwise it would update, and >> look at, a global variable of its own. (Or a parameter to relevant functions if one doesn't like the action-at-a-distance effect of a
global.)

And, it seems to me that the reflog handling should be centralised.

sequencer.c static const char *reflog_message(struct replay_opts *opts, · line 3715

Quoted 6 lines

sequencer.c +2 -1

+     char *reflog_action = getenv("GIT_REFLOG_ACTION");
 
       va_start(ap, fmt);
       strbuf_reset(&buf);
-     strbuf_addstr(&buf, action_name(opts));
+     strbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));

Open coding this kind of thing at every site which needs to think about the reflog actions will surely result in some of the instances having bugs.

Quoted 4 lines

Writing a single function that contans this (or most of it) would happily decouple all of its call sites from literally asking about getenv("GIT_REFLOG_ACTION") thereby making it easier to do the indirection-through-program-variables I suggest.

That sounds great, but I'm not sure that "only when invoking subprocesses" will limit the places where we set the environment variable all that much; it might actually expand it.

I wasn't there
for the whole history, but my understanding is the rebase code has slowly transformed from the original all-shell rebase implementation(s), to being a helper program that the shell could call into for parts of its operations and passing control back and forth between shell and C, to being a reimplementation of just invoking the same commands that the shell script would have, to slowly transforming into an actual library where invocations of other git subprocesses are being replaced with relevant function calls. It's a long cleanup process that is still ongoing. I'd like to get to the point where we only invoke subprocesses if the user specifies --exec or a special merge strategy, but that's a goal with a longer term timeframe than fixing a 2.26 regression.

Quoted 9 lines

Having said that,

t/t3406-rebase-message.sh

Quoted 4 lines

t/t3406-rebase-message.sh +0 -0

index 61b76f33019..927a4f4a4e4 100755

This test case convinces me that the patch has the right behaviour for at least the case I care about :-).

Cool, sounds like it's a good immediate fix for the 2.26 regression, and then longer term as we continue refactoring we can hopefully isolate subprocess handling and writing of state.

As a heads up, though, my personal plans for rebase (subject to buy-in from other stakeholders) is to make it do a lot more in-memory work. In particular, this means for common cases there will be no subprocess invocations, no writing of any state unless/until you hit a conflict, no updating of any files in the working tree until all commits have been created (or a conflict is hit),

and no updating of the branch
until after all the commits have been created.

Thus, for the common
cases with no conflicts, there would only be 1 entry in the reflog of HEAD the entire operation, rather than approximately 1 per commit.

I
have a proof-of-concept showing these ideas work for basic cases.

I hope your tests don't depend on the number of entries added to HEAD's reflog.

Hi Elijah

Thanks for fixing this

Quoted 31 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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 <[email protected]>

---
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(-)

sequencer.c static const char *reflog_message(struct replay_opts *opts, · line 3711

Quoted 9 lines

sequencer.c +1 -0

index e528225e787..5837fdaabbe 100644
@@ -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

Quoted 13 lines

sequencer.c +2 -1

      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;
 

sequencer.c static int pick_commits(struct repository *r, · line 3806

Quoted 2 lines

sequencer.c +1 -0

      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?

Quoted 6 lines · Elijah Newren

As a heads up, though, my personal plans for rebase (subject to buy-in from other stakeholders) is to make it do a lot more in-memory work. In particular, this means for common cases there will be no subprocess invocations, no writing of any state unless/until you hit a conflict, no updating of any files in the working tree until all commits have been created (or a conflict is hit),

I'm with you up to here - it sounds fantastic

Quoted 2 lines

and no updating of the branch
until after all the commits have been created.

We only update the branch reflog at the end of the rebase now.

Quoted 3 lines

Thus, for the common
cases with no conflicts, there would only be 1 entry in the reflog of HEAD the entire operation, rather than approximately 1 per commit.

This I'm not so sure about. In the past where I've messed up a rebase and not noticed until after a subsequent rebase it has been really useful to be able to go through HEAD's reflog and find out where exactly I messed up by looking at the sequence of picks for the first rebase. Specifically it shows which commits where squashed together which you cannot get by running git log on the result of the rebase.

Quoted 2 lines

I
have a proof-of-concept showing these ideas work for basic cases.

Sounds exciting

Hi Phillip,

Quoted 37 lines · Phillip Wood

Hi Elijah

Thanks for fixing this

Quoted 31 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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 <[email protected]>

---
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(-)

Quoted 12 lines

sequencer.c static const char *reflog_message(struct replay_opts *opts, · line 3711

Quoted 9 lines

sequencer.c +1 -0

index e528225e787..5837fdaabbe 100644
@@ -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. :-)

Quoted 13 lines

Quoted 13 lines

sequencer.c +2 -1

       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;
 

Quoted 6 lines

sequencer.c static int pick_commits(struct repository *r, · line 3806

Quoted 2 lines

sequencer.c +1 -0

       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.

Quoted 27 lines · Elijah Newren

Quoted 23 lines · Phillip Wood

Quoted 9 lines · Elijah Newren via GitGitGadget

       va_start(ap, fmt);
       strbuf_reset(&buf);

(quoted diff) +1 -1

-     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) {

sequencer.c static int pick_commits(struct repository *r, · line 3806

Quoted 7 lines

sequencer.c +2 -0

@@ -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?

Quoted 2 lines

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

Hi,

Quoted 28 lines · Phillip Wood

Quoted 27 lines · Elijah Newren

Quoted 23 lines · Phillip Wood

Quoted 9 lines · Elijah Newren via GitGitGadget

       va_start(ap, fmt);
       strbuf_reset(&buf);

(quoted diff) +1 -1

-     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) {

sequencer.c static int pick_commits(struct repository *r, · line 3806

Quoted 7 lines

sequencer.c +2 -0

@@ -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?

Quoted 5 lines

Quoted 2 lines

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

FWIW I was also about to comment on that. Maybe that warrants even a code comment above the prev_reflog_action?

Quoted 34 lines · Johannes Schindelin

Quoted 31 lines · Phillip Wood

Quoted 29 lines · Elijah Newren

Quoted 17 lines · Phillip Wood

Quoted 14 lines · Elijah Newren via GitGitGadget

       va_start(ap, fmt);
       strbuf_reset(&buf);

(quoted diff) +1 -1

-     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) {

sequencer.c +1 -0

@@ -3799,8 +3800,10 @@ static int pick_commits(struct repository *r,
                       struct replay_opts *opts)
   {
       int res = 0, reschedule = 0;
+     char *prev_reflog_action;
 

Quoted 6 lines

sequencer.c static int pick_commits(struct repository *r, · line 3806

Quoted 2 lines

sequencer.c +1 -0

       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.

Quoted 5 lines

Quoted 2 lines

Ah thanks, I thought I must be missing something fairly obvious but couldn't see what it was

FWIW I was also about to comment on that. Maybe that warrants even a code comment above the prev_reflog_action?

Yeah, if it tripped you both up, I'll add such a comment to the code to help explain it.

Quoted 5 lines

Quoted 2 lines

FWIW I was also about to comment on that. Maybe that warrants even a code comment above the prev_reflog_action?

Yeah, if it tripped you both up, I'll add such a comment to the code to help explain it.

Thank you!