On Tue, Sep 21, 2021 at 3:24 AM Phillip Wood via GitGitGadget
<gitgitgadget@xxxxxxxxx> 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 <phillip.wood@xxxxxxxxxxxxx>
> ---
>
> --
> gitgitgadget
