[PATCH v3 00/14] rebase: reset_head() related fixes and improvements

["Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com>]

Thanks to Junio and Elijah for their comments on V2. There aren't too many
changes this time.

This series started out with the aim of converting 'rebase -i' to use
reset_head() instead of forking 'git checkout'. As 'rebase --apply' already
uses reset_head() I assumed this would be straight forward. However it has
morphed into a series of fixes for reset_head() followed by the intended
conversion of 'rebase -i'.

reset.c::reset_head() started its life at ac7f467f (builtin/rebase: support
running "git rebase ", 2018-08-07) as a way to detach the HEAD to replay the
commits during "git rebase", but over time it learned to do many things,
like switching the tip of the branch to another commit, recording the old
value of HEAD in ORIG_HEAD while it does so, recording reflog entries for
both HEAD and for the branch.

The API into the function got clunky and it is harder than necessary for the
callers to use the function correctly, which led to a handful of bugs that
are fixed by this series. The bugs include

 * passing the wrong oid to the post-checkout hook
 * removing untracked files on checkout
 * running the post-checkout hook if the checkout fails
 * passing parameters to reset_head() that it does not use
 * incorrect reflog messages for 'rebase --apply'
 * sometimes setting ORIG_HEAD incorrectly at the start 'rebase --apply'

Later steps of this series revamps the API so that it is harder to use it
incorrectly to prevent future bugs and finally convert 'rebase -i' to use
reset_head()

Changes since V2:

 * Updated cover letter as suggested by Junio
 * Patch 4 - fixed typos in the commit message spotted by Junio
 * Patch 9 - Moved later in the series to simplify the autostash canges as
   suggested by Junio. This used to be patch 7
 * Patch 10 - Added a comment to the commit message explaining why we cannot
   BUG() on an invalid parameter until a change is made in a later commit
 * Patch 13 - Reworded the first sentence commit message as suggest by
   Elijah.

Cover letter for V2:

Thanks for the comments on V1. I have tried to improve the commit messages
to explain better the motivation and implications of the changes in this
series and I have added some more tests. I have rebased onto v2.34.0 to
avoid some merges conflicts.

Changes since V1:

 * Patch 1 - unchanged.
 * Patches 2, 3 - these are new and fix an bug I noticed while adding a test
   to patch 4.
 * Patches 4, 5 - improved commit messages and added tests.
 * Patch 6 - reworded commit message.
 * Patch 7 - split out some changes that used to be in patch 9.
 * Patch 8 - in principle the same but the range-diff is noisy due to the
   addition of patch 3.
 * Patch 9 - reworded commit message.
 * Patch 10 - unchanged.
 * Patch 11 - reworded commit message and a couple of comments.
 * Patch 12 - minor changes to comments.
 * Patch 13 - cosmetic changes to commit message and tests.
 * Patch 14 - cosmetic changes to commit message.

Cover letter for V1: Fix some issues with the implementation and use of
reset_head(). The last patch was previously posted as [1], I have updated
the commit message and rebased it onto the fixes in this series. There are a
couple of small conflicts merging this into seen, I think they should be
easy to resolve (in rebase.c take both sides in reset.c take the changed
lines from each side). These patches are based on pw/rebase-of-a-tag-fix

[1]
https://lore.kernel.org/git/39ad40c9297531a2d42b7263a1d41b1ecbc23c0a.1631108472.git.gitgitgadget@gmail.com/

Phillip Wood (14):
  rebase: factor out checkout for up to date branch
  t5403: refactor rebase post-checkout hook tests
  rebase: pass correct arguments to post-checkout hook
  rebase: do not remove untracked files on checkout
  rebase --apply: don't run post-checkout hook if there is an error
  reset_head(): remove action parameter
  reset_head(): factor out ref updates
  reset_head(): make default_reflog_action optional
  create_autostash(): remove unneeded parameter
  rebase: cleanup reset_head() calls
  reset_head(): take struct rebase_head_opts
  rebase --apply: fix reflog
  rebase --apply: set ORIG_HEAD correctly
  rebase -m: don't fork git checkout

 builtin/merge.c               |   6 +-
 builtin/rebase.c              | 101 +++++++++++++----------
 reset.c                       | 149 ++++++++++++++++++++--------------
 reset.h                       |  48 ++++++++++-
 sequencer.c                   |  47 ++++-------
 sequencer.h                   |   3 +-
 t/t3406-rebase-message.sh     |  23 ++++++
 t/t3418-rebase-continue.sh    |  26 ++++++
 t/t5403-post-checkout-hook.sh |  67 +++++++++++----
 9 files changed, 312 insertions(+), 158 deletions(-)


base-commit: cd3e606211bb1cf8bc57f7d76bab98cc17a150bc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1049%2Fphillipwood%2Fwip%2Frebase-reset-head-fixes-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1049/phillipwood/wip/rebase-reset-head-fixes-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1049

Range-diff vs v2:

  1:  0e84d00572e =  1:  0e84d00572e rebase: factor out checkout for up to date branch
  2:  a67a5a03b94 =  2:  a67a5a03b94 t5403: refactor rebase post-checkout hook tests
  3:  07867760e68 =  3:  07867760e68 rebase: pass correct arguments to post-checkout hook
  4:  2b499704c8f !  4:  f4b925508e7 rebase: do not remove untracked files on checkout
     @@ Commit message
          <upstream> is an ancestor of <branch> then it will fast-forward and
          checkout <branch>. Normally a checkout or picking a commit during a
          rebase will refuse to overwrite untracked files, however rebase does
     -    overwrite untracked files when checking <branch>.
     +    overwrite untracked files when checking out <branch>.
      
          The fix is to only set reset in `unpack_tree_opts` if flags contains
          `RESET_HEAD_HARD`. t5403 may seem like an odd home for the new test
          but it will be extended in the next commit to check that the
          post-checkout hook is not run when the checkout fails.
      
     -    The test for `!deatch_head` dates back to the
     +    The test for `!detach_head` dates back to the
          original implementation of reset_head() in
          ac7f467fef ("builtin/rebase: support running "git rebase <upstream>"",
          2018-08-07) and was correct until e65123a71d
  5:  04e7340a7e7 =  5:  4de5104d22d rebase --apply: don't run post-checkout hook if there is an error
  6:  32ffa98c1bc =  6:  ff23498e93e reset_head(): remove action parameter
  8:  29e06e7d36d =  7:  688ebc45bf7 reset_head(): factor out ref updates
  9:  9d00a218daf !  8:  a5cc7eaa925 reset_head(): make default_reflog_action optional
     @@ reset.c: int reset_head(struct repository *r, struct object_id *oid,
       
       leave_reset_head:
       	rollback_lock_file(&lock);
     -
     - ## sequencer.c ##
     -@@ sequencer.c: void create_autostash(struct repository *r, const char *path)
     - 		write_file(path, "%s", oid_to_hex(&oid));
     - 		printf(_("Created autostash: %s\n"), buf.buf);
     - 		if (reset_head(r, NULL, NULL, RESET_HEAD_HARD, NULL, NULL,
     --			       "") < 0)
     -+			       NULL) < 0)
     - 			die(_("could not reset --hard"));
     - 
     - 		if (discard_index(r->index) < 0 ||
  7:  341fe183c18 !  9:  dd3a22384d2 create_autostash(): remove unneeded parameter
     @@ sequencer.c: void create_autostash(struct repository *r, const char *path,
       		printf(_("Created autostash: %s\n"), buf.buf);
       		if (reset_head(r, NULL, NULL, RESET_HEAD_HARD, NULL, NULL,
      -			       default_reflog_action) < 0)
     -+			       "") < 0)
     ++			       NULL) < 0)
       			die(_("could not reset --hard"));
       
       		if (discard_index(r->index) < 0 ||
 10:  5ea636009e7 ! 10:  ad7c6467987 rebase: cleanup reset_head() calls
     @@ Commit message
          currently possible so we delay fixing that caller until we can pass
          the message as the branch reflog.
      
     +    A later commit will make it a BUG() to pass reflog_orig_head without
     +    RESET_ORIG_HEAD, that changes cannot be done here as it needs to wait
     +    for move_to_original_branch() to be fixed first.
     +
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
       ## builtin/rebase.c ##
 11:  24b0566aba5 = 11:  d170703e833 reset_head(): take struct rebase_head_opts
 12:  dc5d11291e7 = 12:  4973892561e rebase --apply: fix reflog
 13:  45a5b5e9818 ! 13:  0ef0e978112 rebase --apply: set ORIG_HEAD correctly
     @@ Metadata
       ## Commit message ##
          rebase --apply: set ORIG_HEAD correctly
      
     -    At the start of a rebase ORIG_HEAD is updated to tip of the branch
     -    being rebased. Unfortunately reset_head() always uses the current
     -    value of HEAD for this which is incorrect if the rebase is started
     -    with "git rebase <upstream> <branch>" as in that case ORIG_HEAD should
     -    be updated to <branch>. This only affects the "apply" backend as the
     -    "merge" backend does not yet use reset_head() for the initial
     -    checkout. Fix this by passing in orig_head when calling reset_head()
     -    and add some regression tests.
     +    At the start of a rebase, ORIG_HEAD is updated to the tip of the
     +    branch being rebased. Unfortunately reset_head() always uses the
     +    current value of HEAD for this which is incorrect if the rebase is
     +    started with "git rebase <upstream> <branch>" as in that case
     +    ORIG_HEAD should be updated to <branch>. This only affects the "apply"
     +    backend as the "merge" backend does not yet use reset_head() for the
     +    initial checkout. Fix this by passing in orig_head when calling
     +    reset_head() and add some regression tests.
      
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
 14:  3f64b9274b5 = 14:  9b9560ef676 rebase -m: don't fork git checkout

-- 
gitgitgadget