[v4,0/8] rebase: make reflog messages independent of the backend

Message

Derrick Stolee via GitGitGadget Oct. 21, 2022, 9:21 a.m. UTC

This series fixes some bugs in the reflog messages when rebasing and changes
the reflog messages of "rebase --apply" to match "rebase --merge" with the
aim of making the reflog easier to parse.

Thanks to Junio for his comments, here are the changes since V3:

 * Patch 1: Swapped with patch 2. Small change to fast-forward tests to make
   them pass
 * Patch 2: Swapped with patch 1. Reworded commit message and added a change
   to the fast-forward test to reflect the change in reflog message
   introduced by this patch

The rest of the patches are unchanged.

V2 Cover Letter

Thanks to everyone who commented on V2, I've added the review club
participants that I've got address for to the cc list. I've rebased onto
pw/rebase-keep-base-fixes.

Change since V2:

 * Patch 1: Reworded the commit message to address the concerns in [1,2]
   about the behavior when head_name is NULL. There is also a small change
   due to being rebased.

 * Patch 2: Unchanged. There wasn't much love for parameterized tests in
   review club but we want to ensure both backends produce the same messages
   I think this is the safest way to achieve that. Using separate tests
   makes it too easy to introduce subtle differences in the testing of the
   two backends.

 * Patch 3: Added a note to the commit message to address the concerns in
   [1] about not resetting GIT_REFLOG_ACTION when we return early.

 * Patches 4 & 5: Unchanged.

 * Patch 6: Reworded the commit message to make a stronger argument for this
   change. There are concerns about backwards compatibility in [1,3,4] but
   (i) we have made similar changes in the past without complaints and (ii)
   we're changing the message to an existing format. There is also a small
   change due to being rebased.

 * Patches 7 & 8: Small changes due to rebase.

[1]
https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w/edit?pli=1#heading=h.t6g5l2t5ibzw
[2] https://lore.kernel.org/git/xmqq35i7r4rj.fsf@gitster.g/ [3]
https://lore.kernel.org/git/xmqq4k2nmmeg.fsf@gitster.g/ [4]
https://lore.kernel.org/git/[e-mail]/

V2 Cover Letter:

Thanks to Christian and Elijah for their comments on V1.

I've updated commit message for patch 1 to try and be clearer about the
removal of a call to strbuf_release() and spilt out the test changes from
the old patch 2 into a separate preparatory patch.

V1 Cover Letter:

This is a series of rebase reflog related patches with the aim of unifying
the reflog messages from the two rebase backends.

 * improve rebase reflog test coverage
 * rebase --merge: fix reflog messages for --continue and --skip
 * rebase --apply: respect GIT_REFLOG_ACTION
 * rebase --abort: improve reflog message
 * unify reflog messages between the two rebase backends

This series is based on pw/use-inprocess-checkout-in-rebase

Phillip Wood (8):
  t3406: rework rebase reflog tests
  rebase --apply: improve fast-forward reflog message
  rebase --merge: fix reflog when continuing
  rebase --merge: fix reflog message after skipping
  rebase --apply: respect GIT_REFLOG_ACTION
  rebase --apply: make reflog messages match rebase --merge
  rebase --abort: improve reflog message
  rebase: cleanup action handling

 builtin/rebase.c          | 146 ++++++++++++------------------
 sequencer.c               |   5 ++
 t/t3406-rebase-message.sh | 185 +++++++++++++++++++++++++++++++-------
 3 files changed, 215 insertions(+), 121 deletions(-)


base-commit: aa1df8146d70bb85c63b0999868fe29aebc1173e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1150%2Fphillipwood%2Fwip%2Frebase-reflog-fixes-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1150/phillipwood/wip/rebase-reflog-fixes-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1150

Range-diff vs v3:

 2:  b9255ad35d2 ! 1:  8d5ae067ce3 t3406: rework rebase reflog tests
     @@ Commit message
          the "merge" and "apply" backends. The test coverage for the "apply"
          backend now includes setting GIT_REFLOG_ACTION.
      
     -    Note that rebasing the "conflicts" branch does not create any
     -    conflicts yet. A commit to do that will be added in the next commit
     -    and the diff ends up smaller if we have don't rename the branch when
     -    it is added.
     +    Note that rebasing the "conflicts" branch does not create any conflicts
     +    yet. A commit to do that will be added shortly and the diff ends up
     +    smaller if we have don't rename the branch when it is added.
      
          Signed-off-by: Phillip Wood <[e-mail]>
      
     @@ t/t3406-rebase-message.sh: test_expect_success 'error out early upon -C<n> or --
      +	) &&
      +
      +	git log -g --format=%gs -2 >actual &&
     ++	if test "$mode" = "--apply"
     ++	then
     ++		finish_msg="refs/heads/fast-forward onto $(git rev-parse main)"
     ++	else
     ++		finish_msg="returning to refs/heads/fast-forward"
     ++	fi &&
      +	write_reflog_expect <<-EOF &&
     -+	${reflog_action:-rebase} (finish): returning to refs/heads/fast-forward
     ++	${reflog_action:-rebase} (finish): $finish_msg
      +	${reflog_action:-rebase} (start): checkout main
       	EOF
       	test_cmp expect actual &&
 1:  a84cf971a75 ! 2:  12701ef7c7c rebase --apply: remove duplicated code
     @@ Metadata
      Author: Phillip Wood <[e-mail]>
      
       ## Commit message ##
     -    rebase --apply: remove duplicated code
     +    rebase --apply: improve fast-forward reflog message
      
     -    Use move_to_original_branch() when reattaching HEAD after a fast-forward
     -    rather than open coding a copy of that code. move_to_original_branch()
     -    does not call reset_head() if head_name is NULL but there should be no
     -    user visible changes even though we currently call reset_head() in that
     -    case. The reason for this is that the reset_head() call does not add a
     -    message to the reflog because we're not changing the commit that HEAD
     -    points to and so lock_ref_for_update() elides the update. When head_name
     -    is not NULL then reset_head() behaves like "git symbolic-ref" and so the
     -    reflog is updated.
     +    Using move_to_original_branch() when reattaching HEAD after a
     +    fast-forward improves HEAD's reflog message when rebasing a branch. This
     +    is because it uses separate reflog messages for "HEAD" and
     +    "branch". HEAD's reflog will now record which branch we're returning to.
      
     -    Note that the removal of "strbuf_release(&msg)" is safe as there is an
     -    identical call just above this hunk which can be seen by viewing the
     -    diff with -U6.
     +    When rebasing a detached HEAD there is no change in behavior. We
     +    currently call reset_head() when head_name is NULL but
     +    move_to_original_branch() does not. However the existing call does not
     +    add a message to the reflog because we're not changing the commit that
     +    HEAD points to and so lock_ref_for_update() skips the update.
      
     +    Note that the removal of "strbuf_reset(&msg)" is safe as there is a call
     +    to strbuf_release(&msf) just above this hunk which can be seen by
     +    viewing the diff with -U6.
     +
     +    Helped-by: Junio C Hamano <[e-mail]>
          Signed-off-by: Phillip Wood <[e-mail]>
      
       ## builtin/rebase.c ##
     @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
       		ret = finish_rebase(&options);
       		goto cleanup;
       	}
     +
     + ## t/t3406-rebase-message.sh ##
     +@@ t/t3406-rebase-message.sh: test_reflog () {
     + 	) &&
     + 
     + 	git log -g --format=%gs -2 >actual &&
     +-	if test "$mode" = "--apply"
     +-	then
     +-		finish_msg="refs/heads/fast-forward onto $(git rev-parse main)"
     +-	else
     +-		finish_msg="returning to refs/heads/fast-forward"
     +-	fi &&
     + 	write_reflog_expect <<-EOF &&
     +-	${reflog_action:-rebase} (finish): $finish_msg
     ++	${reflog_action:-rebase} (finish): returning to refs/heads/fast-forward
     + 	${reflog_action:-rebase} (start): checkout main
     + 	EOF
     + 	test_cmp expect actual &&
 3:  ea4da25a19c = 3:  2c965f4b97c rebase --merge: fix reflog when continuing
 4:  225ff4baef7 = 4:  17138d910f0 rebase --merge: fix reflog message after skipping
 5:  1094681eb11 = 5:  0c71c732904 rebase --apply: respect GIT_REFLOG_ACTION
 6:  a5338e6bdd8 = 6:  3f6b2e39f40 rebase --apply: make reflog messages match rebase --merge
 7:  aa808725fb8 = 7:  c8fa57f129d rebase --abort: improve reflog message
 8:  f9c8664b883 = 8:  ed800844ba1 rebase: cleanup action handling