Re: [PATCH v3 00/10] rebase: dereference tags

On Tue, Sep 21, 2021 at 3:24 AM Phillip Wood via GitGitGadget
[off-list ref] wrote:

Thanks for the comments on V2. Here are the changes from that version:

 * dropped the controversial cleanup that was patch 8
 * reworded patch 10 as suggested by Junio

Cover letter for v2: Thanks to Ævar and Johannes for their comments.

This version looks good to me.

 * Changed "! test_cmp_rev" to "test_cmp_rev !" (suggested by Ævar)
 * Fixed the quoting for the title of the "rebase --quit" tests.
 * Reworked the last commit to handle the error case first (suggested by
   Ævar)
 * Tweaked the commit messages for patches 8 & 11
 * Rebased onto 31e4a0db03 ("Merge branch 'ab/rebase-fatal-fatal-fix'",
   2021-09-08) to avoid a merge conflict that upset gitgitgadget

Cover letter for V1: Aborting a rebase stated with git rebase <upstream>
<tag-object> should checkout the commit pointed to by . Instead it gives

    error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object       710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'


The fix for that is in the last patch, the rest of the patches are cleanups
to t3407 and builtin/rebase.c

Phillip Wood (10):
  t3407: run tests in $TEST_DIRECTORY
  t3407: use test_commit
  t3407: use test_cmp_rev
  t3407: rename a variable
  t3407: use test_path_is_missing
  t3407: strengthen rebase --abort tests
  t3407: rework rebase --quit tests
  rebase: use our standard error return value
  rebase: use lookup_commit_reference_by_name()
  rebase: dereference tags

 builtin/rebase.c        |  49 ++++++++-----------
 t/t3407-rebase-abort.sh | 105 ++++++++++++++++++----------------------
 2 files changed, 67 insertions(+), 87 deletions(-)


base-commit: 31e4a0db0337e2aa972d9b9f11a332dff7c4cbcb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1033%2Fphillipwood%2Fwip%2Frebase-handle-tags-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1033/phillipwood/wip/rebase-handle-tags-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1033

Range-diff vs v2:

  1:  bac009d8543 =  1:  bac009d8543 t3407: run tests in $TEST_DIRECTORY
  2:  abfffb31a56 =  2:  abfffb31a56 t3407: use test_commit
  3:  7755ce17fef =  3:  7755ce17fef t3407: use test_cmp_rev
  4:  38eee11baf5 =  4:  38eee11baf5 t3407: rename a variable
  5:  61a37c89f1e =  5:  61a37c89f1e t3407: use test_path_is_missing
  6:  6866630528b =  6:  6866630528b t3407: strengthen rebase --abort tests
  7:  fd55a3196b1 =  7:  fd55a3196b1 t3407: rework rebase --quit tests
  8:  ad3c4efc027 <  -:  ----------- rebase: remove redundant strbuf
  9:  ad940b633d0 !  8:  d3af62c746c rebase: use our standard error return value
     @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
                } else if (!(options.flags & REBASE_NO_QUIET))
                        ; /* be quiet */
      @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
     -          reset_head(the_repository, NULL, "Fast-forwarded", options.head_name,
     -                     RESET_HEAD_REFS_ONLY, "HEAD", buf.buf,
     +                     RESET_HEAD_REFS_ONLY, "HEAD", msg.buf,
                           DEFAULT_REFLOG_ACTION);
     +          strbuf_release(&msg);
      -         ret = !!finish_rebase(&options);
      +         ret = finish_rebase(&options);
                goto cleanup;
 10:  bc103e703e8 =  9:  4eedd3ae766 rebase: use lookup_commit_reference_by_name()
 11:  951de6bb199 ! 10:  55a6250ab38 rebase: dereference tags
     @@ Metadata
       ## Commit message ##
          rebase: dereference tags

     -    Aborting a rebase stated with 'git rebase <upstream> <tag-object>'
     -    should checkout the commit pointed to by <tag-object>. Instead it gives
     +    A rebase started with 'git rebase <A> <B>' is conceptually to first
     +    checkout <B> and run 'git rebase <A>' starting from that state.  'git
     +    rebase --abort' in the middle of such a rebase should take us back to
     +    the state we checked out <B>.

     -        error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD':
     -        trying to write non-commit object
     -        710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'
     +    This used to work, even when <B> is a tag that points at a commit,
     +    until Git 2.20.0 when the command was reimplemented in C.  The command
     +    now complains that the tag object itself cannot be checked out, which
     +    may be technically correct but is not what the user asked to do.

     -    This is because when we parse the command line arguments although we
     -    check that the tag points to a commit we remember the oid of the tag
     -    and try and checkout that object rather than the commit it points
     -    to. Fix this by using lookup_commit_reference_by_name() when parsing
     -    the command line.
     +    Fix this old regression by using lookup_commit_reference_by_name()
     +    when parsing <B>. The scripted version did not need to peel the tag
     +    because the commands it passed the tag to (e.g 'git reset') peeled the
     +    tag themselves.

          Signed-off-by: Phillip Wood [off-list ref]
          ---

--
gitgitgadget