rebase: dereference tags
cover rebase: dereference tags
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 (11):
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: remove redundant strbuf
rebase: use our standard error return value
rebase: use lookup_commit_reference_by_name()
rebase: dereference tags
builtin/rebase.c | 67 ++++++++++++------------- t/t3407-rebase-abort.sh | 105 ++++++++++++++++++---------------------- 2 files changed, 76 insertions(+), 96 deletions(-)
base-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1033%2Fphillipwood%2Fwip%2Frebase-handle-tags-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1033/phillipwood/wip/rebase-handle-tags-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1033
Signature · 1 line
gitgitgadget
PATCH 1/11 t3407: run tests in $TEST_DIRECTORY
From: Phillip Wood <[email protected]>
Commit 97b88dd58c ("git-rebase.sh: Fix --merge --abort failures when path contains whitespace", 2008-05-04) started running these tests in a subdirectory with a space in its name. At that time $TEST_DIRECTORY did not contain a space but shortly after in 4a7aaccd83 ("Rename the test trash directory to contain spaces.", 2008-05-04) $TEST_DIRECTORY was changed to contain a space so we no longer need to run these tests in a subdirectory.
--- t/t3407-rebase-abort.sh | 13 ------------- 1 file changed, 13 deletions(-)
t/t3407-rebase-abort.sh +0 -13
@@ -7,13 +7,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . ./test-lib.sh -### Test that we handle space characters properly -work_dir="$(pwd)/test dir" - test_expect_success setup ' - mkdir -p "$work_dir" && - cd "$work_dir" && - git init && echo a > a && git add a && git commit -m a &&
@@ -37,7 +31,6 @@ testrebase() { dotest=$2 test_expect_success "rebase$type --abort" ' - cd "$work_dir" && # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase$type main &&
@@ -48,7 +41,6 @@ testrebase() { ' test_expect_success "rebase$type --abort after --skip" ' - cd "$work_dir" && # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase$type main &&
@@ -61,7 +53,6 @@ testrebase() { ' test_expect_success "rebase$type --abort after --continue" ' - cd "$work_dir" && # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase$type main &&
@@ -77,7 +68,6 @@ testrebase() { ' test_expect_success "rebase$type --abort does not update reflog" ' - cd "$work_dir" && # Clean up the state from the previous one git reset --hard pre-rebase && git reflog show to-rebase > reflog_before &&
@@ -89,7 +79,6 @@ testrebase() { ' test_expect_success 'rebase --abort can not be used with other options' ' - cd "$work_dir" && # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase$type main &&
@@ -103,7 +92,6 @@ testrebase " --apply" .git/rebase-apply testrebase " --merge" .git/rebase-merge test_expect_success 'rebase --apply --quit' ' - cd "$work_dir" && # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase --apply main &&
@@ -115,7 +103,6 @@ test_expect_success 'rebase --apply --quit' ' ' test_expect_success 'rebase --merge --quit' ' - cd "$work_dir" && # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase --merge main &&
Signature · 1 line
gitgitgadget
PATCH 2/11 t3407: use test_commit
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
Simplify the setup by using test_commit. Note that this replaces the branch "pre-rebase" with a tag of the same name. "pre-rebase" is only used as a fixed reference point so it makes sense to use a tag rather than a branch.
--- t/t3407-rebase-abort.sh | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)
t/t3407-rebase-abort.sh +5 -12
@@ -8,22 +8,15 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . ./test-lib.sh test_expect_success setup ' - echo a > a && - git add a && - git commit -m a && + test_commit a a a && git branch to-rebase && - echo b > a && - git commit -a -m b && - echo c > a && - git commit -a -m c && + test_commit b a b && + test_commit c a c && git checkout to-rebase && - echo d > a && - git commit -a -m "merge should fail on this" && - echo e > a && - git commit -a -m "merge should fail on this, too" && - git branch pre-rebase + test_commit "merge should fail on this" a d d && + test_commit "merge should fail on this, too" a e pre-rebase ' testrebase() {
Signature · 1 line
gitgitgadget
PATCH 3/11 t3407: use test_cmp_rev
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
This provides better diagnostics if a test fails
--- t/t3407-rebase-abort.sh | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
t/t3407-rebase-abort.sh +7 -7
@@ -29,7 +29,7 @@ testrebase() { test_must_fail git rebase$type main && test_path_is_dir "$dotest" && git rebase --abort && - test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) && + test_cmp_rev to-rebase pre-rebase && test ! -d "$dotest" '
@@ -39,9 +39,9 @@ testrebase() { test_must_fail git rebase$type main && test_path_is_dir "$dotest" && test_must_fail git rebase --skip && - test $(git rev-parse HEAD) = $(git rev-parse main) && + test_cmp_rev HEAD main && git rebase --abort && - test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) && + test_cmp_rev to-rebase pre-rebase && test ! -d "$dotest" '
@@ -54,9 +54,9 @@ testrebase() { echo d >> a && git add a && test_must_fail git rebase --continue && - test $(git rev-parse HEAD) != $(git rev-parse main) && + ! test_cmp_rev HEAD main && git rebase --abort && - test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) && + test_cmp_rev to-rebase pre-rebase && test ! -d "$dotest" '
@@ -91,7 +91,7 @@ test_expect_success 'rebase --apply --quit' ' test_path_is_dir .git/rebase-apply && head_before=$(git rev-parse HEAD) && git rebase --quit && - test $(git rev-parse HEAD) = $head_before && + test_cmp_rev HEAD $head_before && test ! -d .git/rebase-apply '
@@ -102,7 +102,7 @@ test_expect_success 'rebase --merge --quit' ' test_path_is_dir .git/rebase-merge && head_before=$(git rev-parse HEAD) && git rebase --quit && - test $(git rev-parse HEAD) = $head_before && + test_cmp_rev HEAD $head_before && test ! -d .git/rebase-merge '
Signature · 1 line
gitgitgadget
PATCH 4/11 t3407: rename a variable
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
$dotest holds the name of the rebase state directory and was named after the state directory used at the time the test was added. As we no longer use that name rename the variable to reflect its purpose.
--- t/t3407-rebase-abort.sh | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
t/t3407-rebase-abort.sh +7 -7
@@ -21,35 +21,35 @@ test_expect_success setup ' testrebase() { type=$1 - dotest=$2 + state_dir=$2 test_expect_success "rebase$type --abort" ' # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase$type main && - test_path_is_dir "$dotest" && + test_path_is_dir "$state_dir" && git rebase --abort && test_cmp_rev to-rebase pre-rebase && - test ! -d "$dotest" + test ! -d "$state_dir" ' test_expect_success "rebase$type --abort after --skip" ' # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase$type main && - test_path_is_dir "$dotest" && + test_path_is_dir "$state_dir" && test_must_fail git rebase --skip && test_cmp_rev HEAD main && git rebase --abort && test_cmp_rev to-rebase pre-rebase && - test ! -d "$dotest" + test ! -d "$state_dir" ' test_expect_success "rebase$type --abort after --continue" ' # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase$type main && - test_path_is_dir "$dotest" && + test_path_is_dir "$state_dir" && echo c > a && echo d >> a && git add a &&
@@ -57,7 +57,7 @@ testrebase() { ! test_cmp_rev HEAD main && git rebase --abort && test_cmp_rev to-rebase pre-rebase && - test ! -d "$dotest" + test ! -d "$state_dir" ' test_expect_success "rebase$type --abort does not update reflog" '
Signature · 1 line
gitgitgadget
PATCH 6/11 t3407: strengthen rebase --abort tests
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
The existing tests only check that HEAD points to the correct commit after aborting, they do not check that the original branch is checked out.
--- t/t3407-rebase-abort.sh | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
t/t3407-rebase-abort.sh +10 -3
@@ -19,6 +19,13 @@ test_expect_success setup ' test_commit "merge should fail on this, too" a e pre-rebase ' +# Check that HEAD is equal to "pre-rebase" and the current branch is +# "to-rebase" +check_head() { + test_cmp_rev HEAD pre-rebase && + test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase +} + testrebase() { type=$1 state_dir=$2
@@ -29,7 +36,7 @@ testrebase() { test_must_fail git rebase$type main && test_path_is_dir "$state_dir" && git rebase --abort && - test_cmp_rev to-rebase pre-rebase && + check_head && test_path_is_missing "$state_dir" '
@@ -41,7 +48,7 @@ testrebase() { test_must_fail git rebase --skip && test_cmp_rev HEAD main && git rebase --abort && - test_cmp_rev to-rebase pre-rebase && + check_head && test_path_is_missing "$state_dir" '
@@ -56,7 +63,7 @@ testrebase() { test_must_fail git rebase --continue && ! test_cmp_rev HEAD main && git rebase --abort && - test_cmp_rev to-rebase pre-rebase && + check_head && test_path_is_missing "$state_dir" '
Signature · 1 line
gitgitgadget
PATCH 5/11 t3407: use test_path_is_missing
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
At the end of the test we expect the state directory to be missing, but the tests only check it is not a directory.
--- t/t3407-rebase-abort.sh | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
t/t3407-rebase-abort.sh +5 -5
@@ -30,7 +30,7 @@ testrebase() { test_path_is_dir "$state_dir" && git rebase --abort && test_cmp_rev to-rebase pre-rebase && - test ! -d "$state_dir" + test_path_is_missing "$state_dir" ' test_expect_success "rebase$type --abort after --skip" '
@@ -42,7 +42,7 @@ testrebase() { test_cmp_rev HEAD main && git rebase --abort && test_cmp_rev to-rebase pre-rebase && - test ! -d "$state_dir" + test_path_is_missing "$state_dir" ' test_expect_success "rebase$type --abort after --continue" '
@@ -57,7 +57,7 @@ testrebase() { ! test_cmp_rev HEAD main && git rebase --abort && test_cmp_rev to-rebase pre-rebase && - test ! -d "$state_dir" + test_path_is_missing "$state_dir" ' test_expect_success "rebase$type --abort does not update reflog" '
@@ -92,7 +92,7 @@ test_expect_success 'rebase --apply --quit' ' head_before=$(git rev-parse HEAD) && git rebase --quit && test_cmp_rev HEAD $head_before && - test ! -d .git/rebase-apply + test_path_is_missing .git/rebase-apply ' test_expect_success 'rebase --merge --quit' '
@@ -103,7 +103,7 @@ test_expect_success 'rebase --merge --quit' ' head_before=$(git rev-parse HEAD) && git rebase --quit && test_cmp_rev HEAD $head_before && - test ! -d .git/rebase-merge + test_path_is_missing .git/rebase-merge ' test_done
Signature · 1 line
gitgitgadget
PATCH 7/11 t3407: rework rebase --quit tests
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
9512177b68 ("rebase: add --quit to cleanup rebase, leave everything else untouched", 2016-11-12) seems to have copied the --abort tests but added two separate tests for the two rebase backends rather than adding a single test into the existing testrebase() function.
--- t/t3407-rebase-abort.sh | 34 ++++++++++++---------------------- 1 file changed, 12 insertions(+), 22 deletions(-)
t/t3407-rebase-abort.sh +12 -22
@@ -86,31 +86,21 @@ testrebase() { test_must_fail git rebase --abort -v && git rebase --abort ' + + test_expect_success 'rebase$type --quit' ' + test_when_finished "git symbolic-ref HEAD refs/heads/to-rebase" && + # Clean up the state from the previous one + git reset --hard pre-rebase && + test_must_fail git rebase$type main && + test_path_is_dir $state_dir && + head_before=$(git rev-parse HEAD) && + git rebase --quit && + test_cmp_rev HEAD $head_before && + test_path_is_missing .git/rebase-apply + ' } testrebase " --apply" .git/rebase-apply testrebase " --merge" .git/rebase-merge -test_expect_success 'rebase --apply --quit' ' - # Clean up the state from the previous one - git reset --hard pre-rebase && - test_must_fail git rebase --apply main && - test_path_is_dir .git/rebase-apply && - head_before=$(git rev-parse HEAD) && - git rebase --quit && - test_cmp_rev HEAD $head_before && - test_path_is_missing .git/rebase-apply -' - -test_expect_success 'rebase --merge --quit' ' - # Clean up the state from the previous one - git reset --hard pre-rebase && - test_must_fail git rebase --merge main && - test_path_is_dir .git/rebase-merge && - head_before=$(git rev-parse HEAD) && - git rebase --quit && - test_cmp_rev HEAD $head_before && - test_path_is_missing .git/rebase-merge -' - test_done
Signature · 1 line
gitgitgadget
PATCH 8/11 rebase: remove redundant strbuf
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
There is already an strbuf that can be reused for creating messages. msg is not freed if there is an error and there is a logic error where we call strbuf_release(&msg) followed by strbuf_reset(&msg) and strbuf_addf(&msg).
--- builtin/rebase.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
builtin/rebase.c +6 -8
@@ -1299,7 +1299,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)int ret, flags, total_argc, in_progress =0 ;int keep_base =0 ;int ok_to_skip_pre_rebase =0 ; -struct strbuf msg =STRBUF_INIT ;struct strbuf revisions =STRBUF_INIT ;struct strbuf buf =STRBUF_INIT ;struct object_id merge_base;
@@ -2063,30 +2062,29 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)printf (_ ("First, rewinding head to replay your work on top of " "it...\n" )); -strbuf_addf (&msg,"%s: checkout %s" , +strbuf_reset (&buf); +strbuf_addf (&buf,"%s: checkout %s" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), options.onto_name);if (reset_head (the_repository, &options.onto->object.oid,"checkout" ,NULL ,RESET_HEAD_DETACH |RESET_ORIG_HEAD |RESET_HEAD_RUN_POST_CHECKOUT_HOOK , -NULL , msg.buf,DEFAULT_REFLOG_ACTION )) +NULL , buf.buf,DEFAULT_REFLOG_ACTION ))die (_ ("Could not detach HEAD" )); -strbuf_release (&msg);/* * If the onto is a proper descendant of the tip of the branch, then * we just fast-forwarded. */ -strbuf_reset (&msg); +strbuf_reset (&buf);if (oideq (&merge_base, &options.orig_head)) {printf (_ ("Fast-forwarded %s to %s.\n" ), branch_name, options.onto_name); -strbuf_addf (&msg,"rebase finished: %s onto %s" , +strbuf_addf (&buf,"rebase finished: %s onto %s" , options.head_name ? options.head_name :"detached HEAD" ,oid_to_hex (&options.onto->object.oid));reset_head (the_repository,NULL ,"Fast-forwarded" , options.head_name, -RESET_HEAD_REFS_ONLY ,"HEAD" , msg.buf, +RESET_HEAD_REFS_ONLY ,"HEAD" , buf.buf,DEFAULT_REFLOG_ACTION ); -strbuf_release (&msg); ret = !!finish_rebase (&options); goto cleanup; }
Signature · 1 line
gitgitgadget
PATCH 9/11 rebase: use our standard error return value
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
Git uses −1 to signal an error. The builtin rebase converts these to +1 all over the place using !! (presumably because the in the scripted version an error was signalled by +1). This is confusing and clutters the code, we only need to convert the value when the function returns.
--- builtin/rebase.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
builtin/rebase.c +9 -9
@@ -1573,7 +1573,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)die (_ ("could not move back to %s" ),oid_to_hex (&options.orig_head));remove_branch_state (the_repository,0 ); - ret = !!finish_rebase (&options); + ret =finish_rebase (&options); goto cleanup; }case ACTION_QUIT : {
@@ -1582,11 +1582,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)struct replay_opts replay =REPLAY_OPTS_INIT ; replay.action =REPLAY_INTERACTIVE_REBASE ; - ret = !!sequencer_remove_state (&replay); + ret =sequencer_remove_state (&replay); }else {strbuf_reset (&buf);strbuf_addstr (&buf, options.state_dir); - ret = !!remove_dir_recursively (&buf,0 ); + ret =remove_dir_recursively (&buf,0 );if (ret)error (_ ("could not remove '%s'" ), options.state_dir);
@@ -1958,7 +1958,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)if (require_clean_work_tree (the_repository,"rebase" ,_ ("Please commit or stash them." ),1 ,1 )) { - ret =1 ; + ret =-1 ; goto cleanup; }
@@ -1993,7 +1993,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) RESET_HEAD_RUN_POST_CHECKOUT_HOOK, NULL, buf.buf, DEFAULT_REFLOG_ACTION) < 0) { - ret = !!error (_ ("could not switch to " + ret =error (_ ("could not switch to " "%s" ), options.switch_to); goto cleanup;
@@ -2008,7 +2008,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)else printf (_ ("Current branch %s is up to date.\n" ), branch_name); - ret = !!finish_rebase (&options); + ret =finish_rebase (&options); goto cleanup; }else if (!(options.flags &REBASE_NO_QUIET )) ;/* be quiet */
@@ -2085,7 +2085,7 @@ 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,DEFAULT_REFLOG_ACTION ); - ret = !!finish_rebase (&options); + ret =finish_rebase (&options); goto cleanup; }
@@ -2099,7 +2099,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) options.revisions = revisions.buf; run_rebase: - ret = !!run_specific_rebase (&options, action); + ret =run_specific_rebase (&options, action); cleanup:strbuf_release (&buf);
@@ -2110,5 +2110,5 @@ cleanup:free (options.strategy);strbuf_release (&options.git_format_patch_opt);free (squash_onto_name); -return ret; +return !!ret; }
Signature · 1 line
gitgitgadget
PATCH 11/11 rebase: dereference tags
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
Aborting a rebase stated with 'git rebase <upstream> <tag-object>' should checkout the commit pointed to by <tag-object>. Instead it gives
error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD':
trying to write non-commit object 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'
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.
---
--- builtin/rebase.c | 18 +++++++++++------- t/t3407-rebase-abort.sh | 18 ++++++++++++++---- 2 files changed, 25 insertions(+), 11 deletions(-)
builtin/rebase.c +11 -7
@@ -1903,13 +1903,17 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)die_if_checked_out (buf.buf,1 ); options.head_name =xstrdup (buf.buf);/* If not is it a valid ref (branch or commit)? */ - }else if (!get_oid (branch_name, &options.orig_head) && -lookup_commit_reference (the_repository, - &options.orig_head)) - options.head_name =NULL ; -else -die (_ ("fatal: no such branch/commit '%s'" ), - branch_name); + }else { +struct commit *commit = +lookup_commit_reference_by_name (branch_name); +if (commit) { +oidcpy (&options.orig_head, &commit->object.oid); + options.head_name =NULL ; + }else { +die (_ ("fatal: no such branch/commit '%s'" ), + branch_name); + } + } }else if (argc ==0 ) {/* Do not need to switch branches, we are already on it. */ options.head_name =
t/t3407-rebase-abort.sh +14 -4
@@ -11,18 +11,18 @@ test_expect_success setup ' test_commit a a a && git branch to-rebase && - test_commit b a b && - test_commit c a c && + test_commit --annotate b a b && + test_commit --annotate c a c && git checkout to-rebase && test_commit "merge should fail on this" a d d && - test_commit "merge should fail on this, too" a e pre-rebase + test_commit --annotate "merge should fail on this, too" a e pre-rebase ' # Check that HEAD is equal to "pre-rebase" and the current branch is # "to-rebase" check_head() { - test_cmp_rev HEAD pre-rebase && + test_cmp_rev HEAD pre-rebase^{commit} && test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase }
@@ -67,6 +67,16 @@ testrebase() { test_path_is_missing "$state_dir" ' + test_expect_success "rebase$type --abort when checking out a tag" ' + test_when_finished "git symbolic-ref HEAD refs/heads/to-rebase" && + git reset --hard a -- && + test_must_fail git rebase$type --onto b c pre-rebase && + test_cmp_rev HEAD b^{commit} && + git rebase --abort && + test_cmp_rev HEAD pre-rebase^{commit} && + ! git symbolic-ref HEAD + ' + test_expect_success "rebase$type --abort does not update reflog" ' # Clean up the state from the previous one git reset --hard pre-rebase &&
Signature · 1 line
gitgitgadget
PATCH 10/11 rebase: use lookup_commit_reference_by_name()
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
peel_committish() appears to have been copied from the scripted rebase but it duplicates the functionality of lookup_commit_reference_by_name() so lets use that instead.
--- builtin/rebase.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-)
builtin/rebase.c +4 -13
@@ -762,17 +762,6 @@ static int finish_rebase(struct rebase_options *opts)return ret; } -static struct commit *peel_committish (const char *name) -{ -struct object *obj; -struct object_id oid; - -if (get_oid (name, &oid)) -return NULL ; - obj =parse_object (the_repository, &oid); -return (struct commit *)peel_to_type (name,0 , obj,OBJ_COMMIT ); -} -static void add_var (struct strbuf *buf,const char *name,const char *value) {if (!value)
@@ -1844,7 +1833,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)if (!strcmp (options.upstream_name,"-" )) options.upstream_name ="@{-1}" ; } - options.upstream =peel_committish (options.upstream_name); + options.upstream = +lookup_commit_reference_by_name (options.upstream_name);if (!options.upstream)die (_ ("invalid upstream '%s'" ), options.upstream_name); options.upstream_arg = options.upstream_name;
@@ -1887,7 +1877,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) options.onto =lookup_commit_or_die (&merge_base, options.onto_name); }else { - options.onto =peel_committish (options.onto_name); + options.onto = +lookup_commit_reference_by_name (options.onto_name);if (!options.onto)die (_ ("Does not point to a valid commit '%s'" ), options.onto_name);
Signature · 1 line
gitgitgadget
Re: [PATCH 02/11] t3407: use test_commit
In reply to Phillip Wood via GitGitGadgetQuoted 6 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
Simplify the setup by using
test_commit. Note that this replaces the branch "pre-rebase" with a tag of the same name. "pre-rebase" is only used as a fixed reference point so it makes sense to use a tag rather than a branch.
FWIW you could use --no-tag to test_commit to keep the behavior, but changing it here seems fine.
Re: [PATCH 03/11] t3407: use test_cmp_rev
In reply to Phillip Wood via GitGitGadgett/t3407-rebase-abort.sh testrebase() { · line 57
Quoted 6 lines · Phillip Wood via GitGitGadget
t/t3407-rebase-abort.sh +1
-1@@ -54,9 +54,9 @@ testrebase() { echo d >> a && git add a && test_must_fail git rebase --continue && - test $(git rev-parse HEAD) != $(git rev-parse main) && + ! test_cmp_rev HEAD main &&
Use "test_cmp_rev !", making the shell do the negation is troublesome for the same reason we don't do it with git command, it potentially hides segfaults (and that test helper invokes "git", hence the optional "!" first rgument).
Re: [PATCH 01/11] t3407: run tests in $TEST_DIRECTORY
In reply to Phillip Wood via GitGitGadgetQuoted 9 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
Commit
97b88dd58c("git-rebase.sh: Fix --merge --abort failures when path contains whitespace", 2008-05-04) started running these tests in a subdirectory with a space in its name. At that time $TEST_DIRECTORY did not contain a space but shortly after in4a7aaccd83("Rename the test trash directory to contain spaces.", 2008-05-04) $TEST_DIRECTORY was changed to contain a space so we no longer need to run these tests in a subdirectory.
Well spotted & a nice cleanup!
Re: [PATCH 06/11] t3407: strengthen rebase --abort tests
In reply to Phillip Wood via GitGitGadgett/t3407-rebase-abort.sh test_expect_success setup ' · line 27
Quoted 10 lines · Phillip Wood via GitGitGadget
The existing tests only check that HEAD points to the correct commit after aborting, they do not check that the original branch is checked out.
[...]t/t3407-rebase-abort.sh +6
-0+# Check that HEAD is equal to "pre-rebase" and the current branch is +# "to-rebase" +check_head() { + test_cmp_rev HEAD pre-rebase && + test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase +}
I reflexively thought "use test_cmp here!", but in this case that's pointless as any difference is noted by test_cmp_rev, we're really seeing what HEAD is checked out to here. I see we use the same pattern in various other tests...
Quoted 38 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
Aborting a rebase stated with 'git rebase <upstream> <tag-object>' should checkout the commit pointed to by <tag-object>. Instead it gives
error:
update_reffailed for ref 'HEAD': cannot update ref 'HEAD':
trying to write non-commit object710d743b2b9892457fdcc3970f397e6ec07447e0to branch 'HEAD'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.Signed-off-by Phillip Wood <[email protected]>---
--- builtin/rebase.c | 18 +++++++++++------- t/t3407-rebase-abort.sh | 18 ++++++++++++++---- 2 files changed, 25 insertions(+), 11 deletions(-)builtin/rebase.c +1
-7@@ -1903,13 +1903,17 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)die_if_checked_out (buf.buf,1 ); options.head_name =xstrdup (buf.buf);/* If not is it a valid ref (branch or commit)? */ - }else if (!get_oid (branch_name, &options.orig_head) && -lookup_commit_reference (the_repository, - &options.orig_head)) - options.head_name =NULL ; -else -die (_ ("fatal: no such branch/commit '%s'" ), - branch_name); + }else {
builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1916
Quoted 10 lines
builtin/rebase.c +10
-0+struct commit *commit = +lookup_commit_reference_by_name (branch_name); +if (commit) { +oidcpy (&options.orig_head, &commit->object.oid); + options.head_name =NULL ; + }else { +die (_ ("fatal: no such branch/commit '%s'" ), + branch_name); + } + }
Suggested style nit:
builtin/rebase.c +4 -5
@@ -1906,13 +1906,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) }else {struct commit *commit =lookup_commit_reference_by_name (branch_name); -if (commit) { -oidcpy (&options.orig_head, &commit->object.oid); - options.head_name =NULL ; - }else { +if (!commit)die (_ ("fatal: no such branch/commit '%s'" ), branch_name); - } + +oidcpy (&options.orig_head, &commit->object.oid); + options.head_name =NULL ; } }else if (argc ==0 ) {/* Do not need to switch branches, we are already on it. */
I.e. handle the "die" case right away & skip the indenting of the non-assert code.
(Also grepping around we could really use a fatal non-gentle version of lookup_commit_reference_by_name(), but that's another more general cleanup...)
Quoted 37 lines
builtin/rebase.c +0
-0}else if (argc ==0 ) {/* Do not need to switch branches, we are already on it. */ options.head_name =t/t3407-rebase-abort.sh +7
-4@@ -11,18 +11,18 @@ test_expect_success setup ' test_commit a a a && git branch to-rebase && - test_commit b a b && - test_commit c a c && + test_commit --annotate b a b && + test_commit --annotate c a c && git checkout to-rebase && test_commit "merge should fail on this" a d d && - test_commit "merge should fail on this, too" a e pre-rebase + test_commit --annotate "merge should fail on this, too" a e pre-rebase ' # Check that HEAD is equal to "pre-rebase" and the current branch is # "to-rebase" check_head() { - test_cmp_rev HEAD pre-rebase && + test_cmp_rev HEAD pre-rebase^{commit} && test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase }@@ -67,6 +67,16 @@ testrebase() { test_path_is_missing "$state_dir" ' + test_expect_success "rebase$type --abort when checking out a tag" ' + test_when_finished "git symbolic-ref HEAD refs/heads/to-rebase" && + git reset --hard a -- &&
t/t3407-rebase-abort.sh testrebase() { · line 82
Quoted 10 lines
t/t3407-rebase-abort.sh +7
-0+ test_must_fail git rebase$type --onto b c pre-rebase && + test_cmp_rev HEAD b^{commit} && + git rebase --abort && + test_cmp_rev HEAD pre-rebase^{commit} && + ! git symbolic-ref HEAD + ' + test_expect_success "rebase$type --abort does not update reflog" ' # Clean up the state from the previous one git reset --hard pre-rebase &&
This whole series looks good to me, left some comments on other patches. Consider the above suggested squash highly optional :)
Re: [PATCH 03/11] t3407: use test_cmp_rev
In reply to Ævar Arnfjörð BjarmasonQuoted 14 lines · Ævar Arnfjörð Bjarmason
t/t3407-rebase-abort.sh testrebase() { · line 57
Quoted 6 lines · Phillip Wood via GitGitGadget
t/t3407-rebase-abort.sh +1
-1@@ -54,9 +54,9 @@ testrebase() { echo d >> a && git add a && test_must_fail git rebase --continue && - test $(git rev-parse HEAD) != $(git rev-parse main) && + ! test_cmp_rev HEAD main &&Use "
test_cmp_rev!", making the shell do the negation is troublesome for the same reason we don't do it with git command, it potentially hides segfaults (and that test helper invokes "git", hence the optional "!" first argument).
Thanks, I didn't realize one could pass "!" to test_cmp_rev - it gives a better message on failure as well as avoiding hidden segfaults
Best Wishes
Phillip
Re: [PATCH 08/11] rebase: remove redundant strbuf
In reply to Phillip Wood via GitGitGadgetHi Phillip,
Quoted 6 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
There is already an strbuf that can be reused for creating messages. msg is not freed if there is an error and there is a logic error where we call
strbuf_release(&msg)followed bystrbuf_reset(&msg)andstrbuf_addf(&msg).
In some instances, we use a buf variable to construct a string that is then assigned to a const char * and is used elsewhere throughout the function. If this was the case in the code you modified, that would be a problem.
I did verify manually that this is not the case, though, so this patch is good to go.
Thanks,
Dscho
v2 cover rebase: dereference tags
In reply to Phillip Wood via GitGitGadgetThanks to Ævar and Johannes for their comments.
* 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 (11):
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: remove redundant strbuf
rebase: use our standard error return value
rebase: use lookup_commit_reference_by_name()
rebase: dereference tags
builtin/rebase.c | 63 +++++++++++------------- t/t3407-rebase-abort.sh | 105 ++++++++++++++++++---------------------- 2 files changed, 73 insertions(+), 95 deletions(-)
base-commit: 31e4a0db0337e2aa972d9b9f11a332dff7c4cbcb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1033%2Fphillipwood%2Fwip%2Frebase-handle-tags-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1033/phillipwood/wip/rebase-handle-tags-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1033
Range-diff vs v1:
1: 0f5992e8cab = 1: bac009d8543 t3407: run tests in $TEST_DIRECTORY
2: 79b6dec910e = 2: abfffb31a56 t3407: use test_commit
3: dfa27d7a8e7 ! 3: 7755ce17fef t3407: use test_cmp_rev
@@ t/t3407-rebase-abort.sh: testrebase() {
git add a &&
test_must_fail git rebase --continue &&
- test $(git rev-parse HEAD) != $(git rev-parse main) &&
-+ ! test_cmp_rev HEAD main &&
++ test_cmp_rev ! HEAD main &&
git rebase --abort &&
- test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&
+ test_cmp_rev to-rebase pre-rebase &&
4: bef70d86d53 ! 4: 38eee11baf5 t3407: rename a variable
@@ t/t3407-rebase-abort.sh: test_expect_success setup '
echo d >> a &&
git add a &&
@@ t/t3407-rebase-abort.sh: testrebase() {
- ! test_cmp_rev HEAD main &&
+ test_cmp_rev ! HEAD main &&
git rebase --abort &&
test_cmp_rev to-rebase pre-rebase &&
- test ! -d "$dotest"
5: d9376fe0818 ! 5: 61a37c89f1e t3407: use test_path_is_missing
@@ t/t3407-rebase-abort.sh: testrebase() {
test_expect_success "rebase$type --abort after --continue" '
@@ t/t3407-rebase-abort.sh: testrebase() {
- ! test_cmp_rev HEAD main &&
+ test_cmp_rev ! HEAD main &&
git rebase --abort &&
test_cmp_rev to-rebase pre-rebase &&
- test ! -d "$state_dir"
6: 87d7e9bf2d4 ! 6: 6866630528b t3407: strengthen rebase --abort tests
@@ t/t3407-rebase-abort.sh: testrebase() {
@@ t/t3407-rebase-abort.sh: testrebase() {
test_must_fail git rebase --continue &&
- ! test_cmp_rev HEAD main &&
+ test_cmp_rev ! HEAD main &&
git rebase --abort &&
- test_cmp_rev to-rebase pre-rebase &&
+ check_head &&
7: 9a4f6ea73c5 ! 7: fd55a3196b1 t3407: rework rebase --quit tests
@@ t/t3407-rebase-abort.sh: testrebase() {
git rebase --abort
'
+
-+ test_expect_success 'rebase$type --quit' '
++ test_expect_success "rebase$type --quit" '
+ test_when_finished "git symbolic-ref HEAD refs/heads/to-rebase" &&
+ # Clean up the state from the previous one
+ git reset --hard pre-rebase &&
8: 35b6c26c8f9 = 8: ad3c4efc027 rebase: remove redundant strbuf
9: f0644cde725 = 9: ad940b633d0 rebase: use our standard error return value
10: c537897006c = 10: bc103e703e8 rebase: use lookup_commit_reference_by_name()
11: e87ce4fe253 ! 11: 951de6bb199 rebase: dereference tags
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
- } else if (!get_oid(branch_name, &options.orig_head) &&
- lookup_commit_reference(the_repository,
- &options.orig_head))
-- options.head_name = NULL;
-- else
-- die(_("fatal: no such branch/commit '%s'"),
-- branch_name);
+ } else {
+ struct commit *commit =
+ lookup_commit_reference_by_name(branch_name);
-+ if (commit) {
-+ oidcpy(&options.orig_head, &commit->object.oid);
-+ options.head_name = NULL;
-+ } else {
-+ die(_("fatal: no such branch/commit '%s'"),
++ if (!commit)
++ die(_("no such branch/commit '%s'"),
+ branch_name);
-+ }
++ oidcpy(&options.orig_head, &commit->object.oid);
+ options.head_name = NULL;
+- else
+- die(_("no such branch/commit '%s'"),
+- branch_name);
+ }
} else if (argc == 0) {
/* Do not need to switch branches, we are already on it. */Signature · 1 line
gitgitgadget
PATCH v2 1/11 t3407: run tests in $TEST_DIRECTORY
From: Phillip Wood <[email protected]>
Commit 97b88dd58c ("git-rebase.sh: Fix --merge --abort failures when path contains whitespace", 2008-05-04) started running these tests in a subdirectory with a space in its name. At that time $TEST_DIRECTORY did not contain a space but shortly after in 4a7aaccd83 ("Rename the test trash directory to contain spaces.", 2008-05-04) $TEST_DIRECTORY was changed to contain a space so we no longer need to run these tests in a subdirectory.
--- t/t3407-rebase-abort.sh | 13 ------------- 1 file changed, 13 deletions(-)
t/t3407-rebase-abort.sh +0 -13
@@ -7,13 +7,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . ./test-lib.sh -### Test that we handle space characters properly -work_dir="$(pwd)/test dir" - test_expect_success setup ' - mkdir -p "$work_dir" && - cd "$work_dir" && - git init && echo a > a && git add a && git commit -m a &&
@@ -37,7 +31,6 @@ testrebase() { dotest=$2 test_expect_success "rebase$type --abort" ' - cd "$work_dir" && # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase$type main &&
@@ -48,7 +41,6 @@ testrebase() { ' test_expect_success "rebase$type --abort after --skip" ' - cd "$work_dir" && # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase$type main &&
@@ -61,7 +53,6 @@ testrebase() { ' test_expect_success "rebase$type --abort after --continue" ' - cd "$work_dir" && # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase$type main &&
@@ -77,7 +68,6 @@ testrebase() { ' test_expect_success "rebase$type --abort does not update reflog" ' - cd "$work_dir" && # Clean up the state from the previous one git reset --hard pre-rebase && git reflog show to-rebase > reflog_before &&
@@ -89,7 +79,6 @@ testrebase() { ' test_expect_success 'rebase --abort can not be used with other options' ' - cd "$work_dir" && # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase$type main &&
@@ -103,7 +92,6 @@ testrebase " --apply" .git/rebase-apply testrebase " --merge" .git/rebase-merge test_expect_success 'rebase --apply --quit' ' - cd "$work_dir" && # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase --apply main &&
@@ -115,7 +103,6 @@ test_expect_success 'rebase --apply --quit' ' ' test_expect_success 'rebase --merge --quit' ' - cd "$work_dir" && # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase --merge main &&
Signature · 1 line
gitgitgadget
PATCH v2 2/11 t3407: use test_commit
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
Simplify the setup by using test_commit. Note that this replaces the branch "pre-rebase" with a tag of the same name. "pre-rebase" is only used as a fixed reference point so it makes sense to use a tag rather than a branch.
--- t/t3407-rebase-abort.sh | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)
t/t3407-rebase-abort.sh +5 -12
@@ -8,22 +8,15 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . ./test-lib.sh test_expect_success setup ' - echo a > a && - git add a && - git commit -m a && + test_commit a a a && git branch to-rebase && - echo b > a && - git commit -a -m b && - echo c > a && - git commit -a -m c && + test_commit b a b && + test_commit c a c && git checkout to-rebase && - echo d > a && - git commit -a -m "merge should fail on this" && - echo e > a && - git commit -a -m "merge should fail on this, too" && - git branch pre-rebase + test_commit "merge should fail on this" a d d && + test_commit "merge should fail on this, too" a e pre-rebase ' testrebase() {
Signature · 1 line
gitgitgadget
PATCH v2 3/11 t3407: use test_cmp_rev
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
This provides better diagnostics if a test fails
--- t/t3407-rebase-abort.sh | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
t/t3407-rebase-abort.sh +7 -7
@@ -29,7 +29,7 @@ testrebase() { test_must_fail git rebase$type main && test_path_is_dir "$dotest" && git rebase --abort && - test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) && + test_cmp_rev to-rebase pre-rebase && test ! -d "$dotest" '
@@ -39,9 +39,9 @@ testrebase() { test_must_fail git rebase$type main && test_path_is_dir "$dotest" && test_must_fail git rebase --skip && - test $(git rev-parse HEAD) = $(git rev-parse main) && + test_cmp_rev HEAD main && git rebase --abort && - test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) && + test_cmp_rev to-rebase pre-rebase && test ! -d "$dotest" '
@@ -54,9 +54,9 @@ testrebase() { echo d >> a && git add a && test_must_fail git rebase --continue && - test $(git rev-parse HEAD) != $(git rev-parse main) && + test_cmp_rev ! HEAD main && git rebase --abort && - test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) && + test_cmp_rev to-rebase pre-rebase && test ! -d "$dotest" '
@@ -91,7 +91,7 @@ test_expect_success 'rebase --apply --quit' ' test_path_is_dir .git/rebase-apply && head_before=$(git rev-parse HEAD) && git rebase --quit && - test $(git rev-parse HEAD) = $head_before && + test_cmp_rev HEAD $head_before && test ! -d .git/rebase-apply '
@@ -102,7 +102,7 @@ test_expect_success 'rebase --merge --quit' ' test_path_is_dir .git/rebase-merge && head_before=$(git rev-parse HEAD) && git rebase --quit && - test $(git rev-parse HEAD) = $head_before && + test_cmp_rev HEAD $head_before && test ! -d .git/rebase-merge '
Signature · 1 line
gitgitgadget
PATCH v2 4/11 t3407: rename a variable
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
$dotest holds the name of the rebase state directory and was named after the state directory used at the time the test was added. As we no longer use that name rename the variable to reflect its purpose.
--- t/t3407-rebase-abort.sh | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
t/t3407-rebase-abort.sh +7 -7
@@ -21,35 +21,35 @@ test_expect_success setup ' testrebase() { type=$1 - dotest=$2 + state_dir=$2 test_expect_success "rebase$type --abort" ' # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase$type main && - test_path_is_dir "$dotest" && + test_path_is_dir "$state_dir" && git rebase --abort && test_cmp_rev to-rebase pre-rebase && - test ! -d "$dotest" + test ! -d "$state_dir" ' test_expect_success "rebase$type --abort after --skip" ' # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase$type main && - test_path_is_dir "$dotest" && + test_path_is_dir "$state_dir" && test_must_fail git rebase --skip && test_cmp_rev HEAD main && git rebase --abort && test_cmp_rev to-rebase pre-rebase && - test ! -d "$dotest" + test ! -d "$state_dir" ' test_expect_success "rebase$type --abort after --continue" ' # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase$type main && - test_path_is_dir "$dotest" && + test_path_is_dir "$state_dir" && echo c > a && echo d >> a && git add a &&
@@ -57,7 +57,7 @@ testrebase() { test_cmp_rev ! HEAD main && git rebase --abort && test_cmp_rev to-rebase pre-rebase && - test ! -d "$dotest" + test ! -d "$state_dir" ' test_expect_success "rebase$type --abort does not update reflog" '
Signature · 1 line
gitgitgadget
PATCH v2 5/11 t3407: use test_path_is_missing
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
At the end of the test we expect the state directory to be missing, but the tests only check it is not a directory.
--- t/t3407-rebase-abort.sh | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
t/t3407-rebase-abort.sh +5 -5
@@ -30,7 +30,7 @@ testrebase() { test_path_is_dir "$state_dir" && git rebase --abort && test_cmp_rev to-rebase pre-rebase && - test ! -d "$state_dir" + test_path_is_missing "$state_dir" ' test_expect_success "rebase$type --abort after --skip" '
@@ -42,7 +42,7 @@ testrebase() { test_cmp_rev HEAD main && git rebase --abort && test_cmp_rev to-rebase pre-rebase && - test ! -d "$state_dir" + test_path_is_missing "$state_dir" ' test_expect_success "rebase$type --abort after --continue" '
@@ -57,7 +57,7 @@ testrebase() { test_cmp_rev ! HEAD main && git rebase --abort && test_cmp_rev to-rebase pre-rebase && - test ! -d "$state_dir" + test_path_is_missing "$state_dir" ' test_expect_success "rebase$type --abort does not update reflog" '
@@ -92,7 +92,7 @@ test_expect_success 'rebase --apply --quit' ' head_before=$(git rev-parse HEAD) && git rebase --quit && test_cmp_rev HEAD $head_before && - test ! -d .git/rebase-apply + test_path_is_missing .git/rebase-apply ' test_expect_success 'rebase --merge --quit' '
@@ -103,7 +103,7 @@ test_expect_success 'rebase --merge --quit' ' head_before=$(git rev-parse HEAD) && git rebase --quit && test_cmp_rev HEAD $head_before && - test ! -d .git/rebase-merge + test_path_is_missing .git/rebase-merge ' test_done
Signature · 1 line
gitgitgadget
PATCH v2 6/11 t3407: strengthen rebase --abort tests
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
The existing tests only check that HEAD points to the correct commit after aborting, they do not check that the original branch is checked out.
--- t/t3407-rebase-abort.sh | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
t/t3407-rebase-abort.sh +10 -3
@@ -19,6 +19,13 @@ test_expect_success setup ' test_commit "merge should fail on this, too" a e pre-rebase ' +# Check that HEAD is equal to "pre-rebase" and the current branch is +# "to-rebase" +check_head() { + test_cmp_rev HEAD pre-rebase && + test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase +} + testrebase() { type=$1 state_dir=$2
@@ -29,7 +36,7 @@ testrebase() { test_must_fail git rebase$type main && test_path_is_dir "$state_dir" && git rebase --abort && - test_cmp_rev to-rebase pre-rebase && + check_head && test_path_is_missing "$state_dir" '
@@ -41,7 +48,7 @@ testrebase() { test_must_fail git rebase --skip && test_cmp_rev HEAD main && git rebase --abort && - test_cmp_rev to-rebase pre-rebase && + check_head && test_path_is_missing "$state_dir" '
@@ -56,7 +63,7 @@ testrebase() { test_must_fail git rebase --continue && test_cmp_rev ! HEAD main && git rebase --abort && - test_cmp_rev to-rebase pre-rebase && + check_head && test_path_is_missing "$state_dir" '
Signature · 1 line
gitgitgadget
PATCH v2 7/11 t3407: rework rebase --quit tests
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
9512177b68 ("rebase: add --quit to cleanup rebase, leave everything else untouched", 2016-11-12) seems to have copied the --abort tests but added two separate tests for the two rebase backends rather than adding a single test into the existing testrebase() function.
--- t/t3407-rebase-abort.sh | 34 ++++++++++++---------------------- 1 file changed, 12 insertions(+), 22 deletions(-)
t/t3407-rebase-abort.sh +12 -22
@@ -86,31 +86,21 @@ testrebase() { test_must_fail git rebase --abort -v && git rebase --abort ' + + test_expect_success "rebase$type --quit" ' + test_when_finished "git symbolic-ref HEAD refs/heads/to-rebase" && + # Clean up the state from the previous one + git reset --hard pre-rebase && + test_must_fail git rebase$type main && + test_path_is_dir $state_dir && + head_before=$(git rev-parse HEAD) && + git rebase --quit && + test_cmp_rev HEAD $head_before && + test_path_is_missing .git/rebase-apply + ' } testrebase " --apply" .git/rebase-apply testrebase " --merge" .git/rebase-merge -test_expect_success 'rebase --apply --quit' ' - # Clean up the state from the previous one - git reset --hard pre-rebase && - test_must_fail git rebase --apply main && - test_path_is_dir .git/rebase-apply && - head_before=$(git rev-parse HEAD) && - git rebase --quit && - test_cmp_rev HEAD $head_before && - test_path_is_missing .git/rebase-apply -' - -test_expect_success 'rebase --merge --quit' ' - # Clean up the state from the previous one - git reset --hard pre-rebase && - test_must_fail git rebase --merge main && - test_path_is_dir .git/rebase-merge && - head_before=$(git rev-parse HEAD) && - git rebase --quit && - test_cmp_rev HEAD $head_before && - test_path_is_missing .git/rebase-merge -' - test_done
Signature · 1 line
gitgitgadget
PATCH v2 8/11 rebase: remove redundant strbuf
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
There is already an strbuf that can be reused for creating messages. msg is not freed if there is an error and there is a logic error where we call strbuf_release(&msg) followed by strbuf_reset(&msg) and strbuf_addf(&msg).
--- builtin/rebase.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
builtin/rebase.c +6 -8
@@ -1299,7 +1299,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)int ret, flags, total_argc, in_progress =0 ;int keep_base =0 ;int ok_to_skip_pre_rebase =0 ; -struct strbuf msg =STRBUF_INIT ;struct strbuf revisions =STRBUF_INIT ;struct strbuf buf =STRBUF_INIT ;struct object_id merge_base;
@@ -2063,30 +2062,29 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)printf (_ ("First, rewinding head to replay your work on top of " "it...\n" )); -strbuf_addf (&msg,"%s: checkout %s" , +strbuf_reset (&buf); +strbuf_addf (&buf,"%s: checkout %s" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), options.onto_name);if (reset_head (the_repository, &options.onto->object.oid,"checkout" ,NULL ,RESET_HEAD_DETACH |RESET_ORIG_HEAD |RESET_HEAD_RUN_POST_CHECKOUT_HOOK , -NULL , msg.buf,DEFAULT_REFLOG_ACTION )) +NULL , buf.buf,DEFAULT_REFLOG_ACTION ))die (_ ("Could not detach HEAD" )); -strbuf_release (&msg);/* * If the onto is a proper descendant of the tip of the branch, then * we just fast-forwarded. */ -strbuf_reset (&msg); +strbuf_reset (&buf);if (oideq (&merge_base, &options.orig_head)) {printf (_ ("Fast-forwarded %s to %s.\n" ), branch_name, options.onto_name); -strbuf_addf (&msg,"rebase finished: %s onto %s" , +strbuf_addf (&buf,"rebase finished: %s onto %s" , options.head_name ? options.head_name :"detached HEAD" ,oid_to_hex (&options.onto->object.oid));reset_head (the_repository,NULL ,"Fast-forwarded" , options.head_name, -RESET_HEAD_REFS_ONLY ,"HEAD" , msg.buf, +RESET_HEAD_REFS_ONLY ,"HEAD" , buf.buf,DEFAULT_REFLOG_ACTION ); -strbuf_release (&msg); ret = !!finish_rebase (&options); goto cleanup; }
Signature · 1 line
gitgitgadget
PATCH v2 9/11 rebase: use our standard error return value
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
Git uses −1 to signal an error. The builtin rebase converts these to +1 all over the place using !! (presumably because the in the scripted version an error was signalled by +1). This is confusing and clutters the code, we only need to convert the value when the function returns.
--- builtin/rebase.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
builtin/rebase.c +9 -9
@@ -1573,7 +1573,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)die (_ ("could not move back to %s" ),oid_to_hex (&options.orig_head));remove_branch_state (the_repository,0 ); - ret = !!finish_rebase (&options); + ret =finish_rebase (&options); goto cleanup; }case ACTION_QUIT : {
@@ -1582,11 +1582,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)struct replay_opts replay =REPLAY_OPTS_INIT ; replay.action =REPLAY_INTERACTIVE_REBASE ; - ret = !!sequencer_remove_state (&replay); + ret =sequencer_remove_state (&replay); }else {strbuf_reset (&buf);strbuf_addstr (&buf, options.state_dir); - ret = !!remove_dir_recursively (&buf,0 ); + ret =remove_dir_recursively (&buf,0 );if (ret)error (_ ("could not remove '%s'" ), options.state_dir);
@@ -1958,7 +1958,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)if (require_clean_work_tree (the_repository,"rebase" ,_ ("Please commit or stash them." ),1 ,1 )) { - ret =1 ; + ret =-1 ; goto cleanup; }
@@ -1993,7 +1993,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) RESET_HEAD_RUN_POST_CHECKOUT_HOOK, NULL, buf.buf, DEFAULT_REFLOG_ACTION) < 0) { - ret = !!error (_ ("could not switch to " + ret =error (_ ("could not switch to " "%s" ), options.switch_to); goto cleanup;
@@ -2008,7 +2008,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)else printf (_ ("Current branch %s is up to date.\n" ), branch_name); - ret = !!finish_rebase (&options); + ret =finish_rebase (&options); goto cleanup; }else if (!(options.flags &REBASE_NO_QUIET )) ;/* be quiet */
@@ -2085,7 +2085,7 @@ 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,DEFAULT_REFLOG_ACTION ); - ret = !!finish_rebase (&options); + ret =finish_rebase (&options); goto cleanup; }
@@ -2099,7 +2099,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) options.revisions = revisions.buf; run_rebase: - ret = !!run_specific_rebase (&options, action); + ret =run_specific_rebase (&options, action); cleanup:strbuf_release (&buf);
@@ -2110,5 +2110,5 @@ cleanup:free (options.strategy);strbuf_release (&options.git_format_patch_opt);free (squash_onto_name); -return ret; +return !!ret; }
Signature · 1 line
gitgitgadget
PATCH v2 11/11 rebase: dereference tags
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
Aborting a rebase stated with 'git rebase <upstream> <tag-object>' should checkout the commit pointed to by <tag-object>. Instead it gives
error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD':
trying to write non-commit object 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'
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.
---
--- builtin/rebase.c | 14 ++++++++------ t/t3407-rebase-abort.sh | 18 ++++++++++++++---- 2 files changed, 22 insertions(+), 10 deletions(-)
builtin/rebase.c +8 -6
@@ -1903,13 +1903,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)die_if_checked_out (buf.buf,1 ); options.head_name =xstrdup (buf.buf);/* If not is it a valid ref (branch or commit)? */ - }else if (!get_oid (branch_name, &options.orig_head) && -lookup_commit_reference (the_repository, - &options.orig_head)) + }else { +struct commit *commit = +lookup_commit_reference_by_name (branch_name); +if (!commit) +die (_ ("no such branch/commit '%s'" ), + branch_name); +oidcpy (&options.orig_head, &commit->object.oid); options.head_name =NULL ; -else -die (_ ("no such branch/commit '%s'" ), - branch_name); + } }else if (argc ==0 ) {/* Do not need to switch branches, we are already on it. */ options.head_name =
t/t3407-rebase-abort.sh +14 -4
@@ -11,18 +11,18 @@ test_expect_success setup ' test_commit a a a && git branch to-rebase && - test_commit b a b && - test_commit c a c && + test_commit --annotate b a b && + test_commit --annotate c a c && git checkout to-rebase && test_commit "merge should fail on this" a d d && - test_commit "merge should fail on this, too" a e pre-rebase + test_commit --annotate "merge should fail on this, too" a e pre-rebase ' # Check that HEAD is equal to "pre-rebase" and the current branch is # "to-rebase" check_head() { - test_cmp_rev HEAD pre-rebase && + test_cmp_rev HEAD pre-rebase^{commit} && test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase }
@@ -67,6 +67,16 @@ testrebase() { test_path_is_missing "$state_dir" ' + test_expect_success "rebase$type --abort when checking out a tag" ' + test_when_finished "git symbolic-ref HEAD refs/heads/to-rebase" && + git reset --hard a -- && + test_must_fail git rebase$type --onto b c pre-rebase && + test_cmp_rev HEAD b^{commit} && + git rebase --abort && + test_cmp_rev HEAD pre-rebase^{commit} && + ! git symbolic-ref HEAD + ' + test_expect_success "rebase$type --abort does not update reflog" ' # Clean up the state from the previous one git reset --hard pre-rebase &&
Signature · 1 line
gitgitgadget
PATCH v2 10/11 rebase: use lookup_commit_reference_by_name()
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
peel_committish() appears to have been copied from the scripted rebase but it duplicates the functionality of lookup_commit_reference_by_name() so lets use that instead.
--- builtin/rebase.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-)
builtin/rebase.c +4 -13
@@ -762,17 +762,6 @@ static int finish_rebase(struct rebase_options *opts)return ret; } -static struct commit *peel_committish (const char *name) -{ -struct object *obj; -struct object_id oid; - -if (get_oid (name, &oid)) -return NULL ; - obj =parse_object (the_repository, &oid); -return (struct commit *)peel_to_type (name,0 , obj,OBJ_COMMIT ); -} -static void add_var (struct strbuf *buf,const char *name,const char *value) {if (!value)
@@ -1844,7 +1833,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)if (!strcmp (options.upstream_name,"-" )) options.upstream_name ="@{-1}" ; } - options.upstream =peel_committish (options.upstream_name); + options.upstream = +lookup_commit_reference_by_name (options.upstream_name);if (!options.upstream)die (_ ("invalid upstream '%s'" ), options.upstream_name); options.upstream_arg = options.upstream_name;
@@ -1887,7 +1877,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) options.onto =lookup_commit_or_die (&merge_base, options.onto_name); }else { - options.onto =peel_committish (options.onto_name); + options.onto = +lookup_commit_reference_by_name (options.onto_name);if (!options.onto)die (_ ("Does not point to a valid commit '%s'" ), options.onto_name);
Signature · 1 line
gitgitgadget
Re: [PATCH v2 08/11] rebase: remove redundant strbuf
In reply to Phillip Wood via GitGitGadgetAm 13.09.21 um 17:19 schrieb Phillip Wood via GitGitGadget:
Quoted 3 lines
From: Phillip Wood <[email protected]>
There is already an strbuf that can be reused for creating messages.
Reminds me of a terrible joke from elementary school: In Peter's class everybody is called Klaus, except Franz -- his name is Michael.
Why would we want to use the same variable for multiple purposes? This makes the code harder to read. And the allocation overhead for these few cases should be negligible.
The most important question: Is this patch really needed to support tags (the purpose of this series)?
Quoted 3 lines
msg is not freed if there is an error and there is a logic error where we call
strbuf_release(&msg)followed bystrbuf_reset(&msg)andstrbuf_addf(&msg).
strbuf_reset() after strbuf_release() is redundant but legal.
Quoted 46 lines
Signed-off-by Phillip Wood <[email protected]>--- builtin/rebase.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)builtin/rebase.c +5
-6@@ -1299,7 +1299,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)int ret, flags, total_argc, in_progress =0 ;int keep_base =0 ;int ok_to_skip_pre_rebase =0 ; -struct strbuf msg =STRBUF_INIT ;struct strbuf revisions =STRBUF_INIT ;struct strbuf buf =STRBUF_INIT ;struct object_id merge_base;@@ -2063,30 +2062,29 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)printf (_ ("First, rewinding head to replay your work on top of " "it...\n" )); -strbuf_addf (&msg,"%s: checkout %s" , +strbuf_reset (&buf); +strbuf_addf (&buf,"%s: checkout %s" ,getenv (GIT_REFLOG_ACTION_ENVIRONMENT ), options.onto_name);if (reset_head (the_repository, &options.onto->object.oid,"checkout" ,NULL ,RESET_HEAD_DETACH |RESET_ORIG_HEAD |RESET_HEAD_RUN_POST_CHECKOUT_HOOK , -NULL , msg.buf,DEFAULT_REFLOG_ACTION )) +NULL , buf.buf,DEFAULT_REFLOG_ACTION ))die (_ ("Could not detach HEAD" )); -strbuf_release (&msg);/* * If the onto is a proper descendant of the tip of the branch, then * we just fast-forwarded. */ -strbuf_reset (&msg); +strbuf_reset (&buf);if (oideq (&merge_base, &options.orig_head)) {printf (_ ("Fast-forwarded %s to %s.\n" ), branch_name, options.onto_name); - strbuf_addf(&msg, "rebase finished: %s onto %s", + strbuf_addf(&buf, "rebase finished: %s onto %s",
builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 2090
Quoted 10 lines
builtin/rebase.c +1
-2options.head_name ? options.head_name :"detached HEAD" , oid_to_hex(&options.onto->object.oid));reset_head (the_repository,NULL ,"Fast-forwarded" , options.head_name, -RESET_HEAD_REFS_ONLY ,"HEAD" , msg.buf, +RESET_HEAD_REFS_ONLY ,"HEAD" , buf.buf,DEFAULT_REFLOG_ACTION ); -strbuf_release (&msg); ret = !!finish_rebase (&options); goto cleanup; }
msg is not released if die() is called, but that's OK; in all other cases it _is_ released in the old code.
I'd rather see the use of that multi-purpose "buf" reduced, e.g. we could simplify path-building like this in a few cases:
- strbuf_reset(&buf);
- strbuf_addf(&buf, "%s/applying", apply_dir());
- if(file_exists(buf.buf))
+ if (file_exists(mkpath("%s/applying", apply_dir())))Sure, this looks a bit lispy, but still better than the old code because there is no state to carry around and reset when "buf" is repurposed.
René
Re: [PATCH v2 08/11] rebase: remove redundant strbuf
Quoted 12 lines · René Scharfe
Am 13.09.21 um 17:19 schrieb Phillip Wood via GitGitGadget:
Quoted 3 lines
From: Phillip Wood <[email protected]>
There is already an strbuf that can be reused for creating messages.
Reminds me of a terrible joke from elementary school: In Peter's class everybody is called Klaus, except Franz -- his name is Michael.
Why would we want to use the same variable for multiple purposes? This makes the code harder to read. And the allocation overhead for these few cases should be negligible.
Quoted 8 lines
The most important question: Is this patch really needed to support tags (the purpose of this series)?
Quoted 3 lines
msg is not freed if there is an error and there is a logic error where we call
strbuf_release(&msg)followed bystrbuf_reset(&msg)andstrbuf_addf(&msg).
strbuf_reset()afterstrbuf_release()is redundant but legal.
All good points.
I do not care too deeply either way, in the sense that it probably is better for this function to have two variables (with at least one of them having a meaningful name "msg" that tells readers what it is about), if the original submission to rewrite "rebase" in C used a single strbuf for both of these and given it a name (like "tmp") that makes it clear that the buffer is merely a temporary area without any longer term significance, I probably wouldn't have told the submitter to rewrite it to use separate strbuf variables.
But if existing code already uses two variables, with at least one of them having a meaningful name that tells what it is used for, I see no reason why we want to rewrite it to use a single one.
Thanks.
Quoted 4 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
Aborting a rebase stated with 'git rebase <upstream> <tag-object>' should checkout the commit pointed to by <tag-object>. Instead it gives
I am not sure if "should checkout the commit pointed to by." is a good description. It does not seem to be sufficiently justified.
Did we auto-peel in scripted version of "git rebase" and is this a regression when the command was rewritten in C?
If that is not the case, this topic is perhaps slightly below borderline "meh" to me. The optional "first switch to this <branch> before doing anything" command-line argument in
git rebase [--onto <there>] <upstream> [<branch>]
was meant to give a branch, and because we treat detached HEAD as almost first-class citizen when dealing with branch-ish things, we allowed
git rebase master my-topic^0
to try rebasing my-topic on detached HEAD without losing the original. In other words, you had to be explicit that you meant the commit object, not a ref that points at it, to trigger this "rebase detached" feature. The same thing for tags.
git rebase master v12.3^0
would be a proper request to rebase the history leading to that commit. Without the peeling, it appears the user is asking to update the ref that can be uniquely identified with "v12.3", but we do not want to rebase a tag.
It would have been a different story if we had a problem when a tag is given to "--onto <there>", but I do not think this topic is about that case.
Having said that, even if we decide that we shouldn't accept the tag object and require peeled form to avoid mistakes (instead of silently peeling the tag ourselves), I do agree that
Quoted 2 lines
error:
update_reffailed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object710d743b2b9892457fdcc3970f397e6ec07447e0to branch 'HEAD'
is a bad error message for this. It should be something like
error: cannot rebase a tag
perhaps.
But if we auto-peeled in an old version, I do not mind this series (but let's drop pointless "clean-up" that is not, like what was pointed out by Réne). In such a case, the first paragraph should say, instead of "should checkout", that "we used to do X, but commit Y broke us and now we die with an error message".
Thanks.
Quoted 4 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
Aborting a rebase stated with 'git rebase <upstream> <tag-object>'
s/stated/started/
Quoted 91 lines
should checkout the commit pointed to by <tag-object>. Instead it gives
error:
update_reffailed for ref 'HEAD': cannot update ref 'HEAD':
trying to write non-commit object710d743b2b9892457fdcc3970f397e6ec07447e0to branch 'HEAD'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.Signed-off-by Phillip Wood <[email protected]>---
--- builtin/rebase.c | 14 ++++++++------ t/t3407-rebase-abort.sh | 18 ++++++++++++++---- 2 files changed, 22 insertions(+), 10 deletions(-)builtin/rebase.c +8
-6@@ -1903,13 +1903,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)die_if_checked_out (buf.buf,1 ); options.head_name =xstrdup (buf.buf);/* If not is it a valid ref (branch or commit)? */ - }else if (!get_oid (branch_name, &options.orig_head) && -lookup_commit_reference (the_repository, - &options.orig_head)) + }else { +struct commit *commit = +lookup_commit_reference_by_name (branch_name); +if (!commit) +die (_ ("no such branch/commit '%s'" ), + branch_name); +oidcpy (&options.orig_head, &commit->object.oid); options.head_name =NULL ; -else -die (_ ("no such branch/commit '%s'" ), - branch_name); + } }else if (argc ==0 ) {/* Do not need to switch branches, we are already on it. */ options.head_name =t/t3407-rebase-abort.sh +14
-5@@ -11,18 +11,18 @@ test_expect_success setup ' test_commit a a a && git branch to-rebase && - test_commit b a b && - test_commit c a c && + test_commit --annotate b a b && + test_commit --annotate c a c && git checkout to-rebase && test_commit "merge should fail on this" a d d && - test_commit "merge should fail on this, too" a e pre-rebase + test_commit --annotate "merge should fail on this, too" a e pre-rebase ' # Check that HEAD is equal to "pre-rebase" and the current branch is # "to-rebase" check_head() { - test_cmp_rev HEAD pre-rebase && + test_cmp_rev HEAD pre-rebase^{commit} && test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase }@@ -67,6 +67,16 @@ testrebase() { test_path_is_missing "$state_dir" ' + test_expect_success "rebase$type --abort when checking out a tag" ' + test_when_finished "git symbolic-ref HEAD refs/heads/to-rebase" && + git reset --hard a -- && + test_must_fail git rebase$type --onto b c pre-rebase && + test_cmp_rev HEAD b^{commit} && + git rebase --abort && + test_cmp_rev HEAD pre-rebase^{commit} && + ! git symbolic-ref HEAD + ' + test_expect_success "rebase$type --abort does not update reflog" ' # Clean up the state from the previous one git reset --hard pre-rebase && --gitgitgadget
Quoted 13 lines · Phillip Wood via GitGitGadget
Thanks to Ævar and Johannes for their comments.
* 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 onto31e4a0db03("Merge branch 'ab/rebase-fatal-fatal-fix'", 2021-09-08) to avoid a merge conflict that upset gitgitgadgetCover letter for V1:
Quoted 8 lines
Aborting a rebase stated with git rebase <upstream> <tag-object> should checkout the commit pointed to by . Instead it gives
error:
update_reffailed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object710d743b2b9892457fdcc3970f397e6ec07447e0to branch 'HEAD'The fix for that is in the last patch, the rest of the patches are cleanups to t3407 and
builtin/rebase.c
Might make sense to split this into 3 separate series (t3407 cleanups, builtin/rebase.c cleanups, and the handling of tag objects). But anyway, reading over the 11 patches, the only issue I noticed was what appears to be a simple typo.
Quoted 16 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
Aborting a rebase stated with 'git rebase <upstream> <tag-object>' should checkout the commit pointed to by <tag-object>. Instead it gives
error:
update_reffailed for ref 'HEAD': cannot update ref 'HEAD':
trying to write non-commit object710d743b2b9892457fdcc3970f397e6ec07447e0to branch 'HEAD'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.Signed-off-by Phillip Wood <[email protected]>
[...]
Quoted 8 lines
t/t3407-rebase-abort.sh +0
-0@@ -11,18 +11,18 @@ test_expect_success setup ' test_commit a a a && git branch to-rebase &&
t/t3407-rebase-abort.sh test_expect_success setup ' · line 20
Quoted 10 lines
t/t3407-rebase-abort.sh +3
-3- test_commit b a b && - test_commit c a c && + test_commit --annotate b a b && + test_commit --annotate c a c && git checkout to-rebase && test_commit "merge should fail on this" a d d && - test_commit "merge should fail on this, too" a e pre-rebase + test_commit --annotate "merge should fail on this, too" a e pre-rebase '
These two do not seem to belong to this particular commit?
t/t3407-rebase-abort.sh test_expect_success setup ' · line 27
Quoted 8 lines
t/t3407-rebase-abort.sh +1
-1# Check that HEAD is equal to "pre-rebase" and the current branch is # "to-rebase" check_head() { - test_cmp_rev HEAD pre-rebase && + test_cmp_rev HEAD pre-rebase^{commit} && test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase }
-- Sergey Organov
Hi Sergey
Quoted 34 lines · Sergey Organov
Quoted 16 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
Aborting a rebase stated with 'git rebase <upstream> <tag-object>' should checkout the commit pointed to by <tag-object>. Instead it gives
error:
update_reffailed for ref 'HEAD': cannot update ref 'HEAD':
trying to write non-commit object710d743b2b9892457fdcc3970f397e6ec07447e0to branch 'HEAD'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.Signed-off-by Phillip Wood <[email protected]>[...]
Quoted 13 lines
t/t3407-rebase-abort.sh +2
-2@@ -11,18 +11,18 @@ test_expect_success setup ' test_commit a a a && git branch to-rebase && - test_commit b a b && - test_commit c a c && + test_commit --annotate b a b && + test_commit --annotate c a c &&
Quoted 7 lines
t/t3407-rebase-abort.sh test_expect_success setup ' · line 20
Quoted 5 lines
t/t3407-rebase-abort.sh +1
-1git checkout to-rebase && test_commit "merge should fail on this" a d d && - test_commit "merge should fail on this, too" a e pre-rebase + test_commit --annotate "merge should fail on this, too" a e pre-rebase 'These two do not seem to belong to this particular commit?
They do, the annotated tag is used in the new test added in this commit which tests that we peel tags.
Best Wishes
Phillip
Quoted 11 lines
t/t3407-rebase-abort.sh test_expect_success setup ' · line 27
Quoted 7 lines
t/t3407-rebase-abort.sh +1
-1# Check that HEAD is equal to "pre-rebase" and the current branch is # "to-rebase" check_head() { - test_cmp_rev HEAD pre-rebase && + test_cmp_rev HEAD pre-rebase^{commit} && test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase }-- Sergey Organov
Hi Junio
Quoted 9 lines · Junio C Hamano
Quoted 4 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
Aborting a rebase stated with 'git rebase <upstream> <tag-object>' should checkout the commit pointed to by <tag-object>. Instead it gives
I am not sure if "should checkout the commit pointed to by." is a good description. It does not seem to be sufficiently justified.
My logic was that as we handle commits here it would make sense to handle tags as well - I discovered that this did not work when I happened to use an annotated tag as the <branch> argument to rebase the commits pointed to by the tag and was surprised it did not work when we happily accept tags for <upstream> and --onto.
Quoted 2 lines
Did we auto-peel in scripted version of "git rebase" and is this a regression when the command was rewritten in C?
As far as I can tell we have never peeled tags here
Quoted 17 lines
If that is not the case, this topic is perhaps slightly below borderline "meh" to me. The optional "first switch to this <branch> before doing anything" command-line argument in
git rebase [--onto <there>] <upstream> [<branch>]
was meant to give a branch, and because we treat detached HEAD as almost first-class citizen when dealing with branch-ish things, we allowed
git rebase master my-topic^0
to try rebasing my-topic on detached HEAD without losing the original. In other words, you had to be explicit that you meant the commit object, not a ref that points at it, to trigger this "rebase detached" feature. The same thing for tags.
Quoted 6 lines
git rebase master v12.3^0
would be a proper request to rebase the history leading to that commit. Without the peeling, it appears the user is asking to update the ref that can be uniquely identified with "v12.3", but we do not want to rebase a tag.
I wrote this patch as I felt it was an artificial distinction to require that <branch> is a branch-ish thing rather than a commit-ish thing. Rebase already peels <upstream> and --onto so it feels inconsistent not to do it for <branch>. I guess the counter argument to that is users may be confused and start complaining that the tag itself is not rebased.
Quoted 3 lines
It would have been a different story if we had a problem when a tag is given to "--onto <there>", but I do not think this topic is about that case.
No "--onto <tag>" works fine. We also accept a tag object for upstream without requiring the user to peel it for us.
Quoted 4 lines
Having said that, even if we decide that we shouldn't accept the tag object and require peeled form to avoid mistakes (instead of silently peeling the tag ourselves), I do agree that
Quoted 8 lines
Quoted 2 lines
error:
update_reffailed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object710d743b2b9892457fdcc3970f397e6ec07447e0to branch 'HEAD'is a bad error message for this. It should be something like
error: cannot rebase a tag
perhaps.
We could do that if we're worried that users would be confused by the tag not being rebased if we started automatically peeling <branch>. (I'm kind of leaning in that direction at the moment having read your email)
Best Wishes
Phillip
Quoted 8 lines
But if we auto-peeled in an old version, I do not mind this series (but let's drop pointless "clean-up" that is not, like what was pointed out by Réne). In such a case, the first paragraph should say, instead of "should checkout", that "we used to do X, but commit Y broke us and now we die with an error message".
Thanks.
Re: [PATCH v2 08/11] rebase: remove redundant strbuf
In reply to Junio C HamanoQuoted 5 lines · Junio C Hamano
Quoted 3 lines · René Scharfe
Am 13.09.21 um 17:19 schrieb Phillip Wood via GitGitGadget:
Quoted 2 lines
From: Phillip Wood <[email protected]>
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
There is already an strbuf that can be reused for creating messages.
Reminds me of a terrible joke from elementary school: In Peter's class everybody is called Klaus, except Franz -- his name is Michael.
Why would we want to use the same variable for multiple purposes? This makes the code harder to read. And the allocation overhead for these few cases should be negligible.
For better or worse reusing the same strbuf is a common pattern and when I saw the code switch to using a different variable I wondered if it was because the value of buf was being used later. It is also confusing to free msg in a different place to all the other variables. rebase_cmd() being so long does not help (msg is defined 763 lines before its first use) as it makes it harder to see what is going on.
Quoted 8 lines
Quoted 8 lines
The most important question: Is this patch really needed to support tags (the purpose of this series)?
Quoted 3 lines
msg is not freed if there is an error and there is a logic error where we call
strbuf_release(&msg)followed bystrbuf_reset(&msg)andstrbuf_addf(&msg).
strbuf_reset()afterstrbuf_release()is redundant but legal.
It is confusing to the reader though, I spent time checking why the strbuf_release() call was there before concluding it was a mistake.
Quoted 11 lines
All good points.
I do not care too deeply either way, in the sense that it probably is better for this function to have two variables (with at least one of them having a meaningful name "msg" that tells readers what it is about), if the original submission to rewrite "rebase" in C used a single strbuf for both of these and given it a name (like "tmp") that makes it clear that the buffer is merely a temporary area without any longer term significance, I probably wouldn't have told the submitter to rewrite it to use separate strbuf variables.
Quoted 3 lines
But if existing code already uses two variables, with at least one of them having a meaningful name that tells what it is used for, I see no reason why we want to rewrite it to use a single one.
In a short function where it is easy to see what happens between the variable declaration and its use I'd agree but here everything is so spread out I actually found the switch to a second variable confusing.
Best Wishes
Phillip
Quoted 2 lines
Thanks.
Re: [PATCH v2 08/11] rebase: remove redundant strbuf
In reply to René ScharfeHi René
Thanks for looking at this series
Quoted 10 lines · René Scharfe
Am 13.09.21 um 17:19 schrieb Phillip Wood via GitGitGadget:
Quoted 1 line
From: Phillip Wood <[email protected]>
I'd rather see the use of that multi-purpose "buf" reduced, e.g. we could simplify path-building like this in a few cases:
(quoted diff) +1
-3- strbuf_reset(&buf); - strbuf_addf(&buf, "%s/applying", apply_dir()); - if(file_exists(buf.buf)) + if (file_exists(mkpath("%s/applying", apply_dir())))
Quoted 3 lines
Sure, this looks a bit lispy, but still better than the old code because there is no state to carry around and reset when "buf" is repurposed.
That's a nice suggestion, I think it's much clearer which file we're checking for.
Best Wishes
Phillip
Quoted 13 lines · Phillip Wood
Hi Junio
Quoted 9 lines · Junio C Hamano
Quoted 4 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
Aborting a rebase stated with 'git rebase <upstream> <tag-object>' should checkout the commit pointed to by <tag-object>. Instead it gives
I am not sure if "should checkout the commit pointed to by." is a good description. It does not seem to be sufficiently justified.
Quoted 6 lines
My logic was that as we handle commits here it would make sense to handle tags as well - I discovered that this did not work when I happened to use an annotated tag as the <branch> argument to rebase the commits pointed to by the tag and was surprised it did not work when we happily accept tags for <upstream> and --onto.
Quoted 2 lines
Did we auto-peel in scripted version of "git rebase" and is this a regression when the command was rewritten in C?
As far as I can tell we have never peeled tags here
That's a bit misleading. We have never peeled a tag given as <branch> when we parse it. In the scripted version we just passed the tag oid along to rev-list, checkout and reset and they peeled it. So I think this is actually a regression in the builtin rebase. I'll update the commit message to reflect that unless we feel that allowing a tag for <branch> is a mistake and we should be erroring out to avoid the possible confusion of the tag not being rebased, only the commits it points to.
Sorry for the confusion
Phillip
Quoted 59 lines
Quoted 17 lines
If that is not the case, this topic is perhaps slightly below borderline "meh" to me. The optional "first switch to this <branch> before doing anything" command-line argument in
git rebase [--onto <there>] <upstream> [<branch>]
was meant to give a branch, and because we treat detached HEAD as almost first-class citizen when dealing with branch-ish things, we allowed
git rebase master my-topic^0
to try rebasing my-topic on detached HEAD without losing the original. In other words, you had to be explicit that you meant the commit object, not a ref that points at it, to trigger this "rebase detached" feature. The same thing for tags.
Quoted 6 lines
git rebase master v12.3^0
would be a proper request to rebase the history leading to that commit. Without the peeling, it appears the user is asking to update the ref that can be uniquely identified with "v12.3", but we do not want to rebase a tag.
I wrote this patch as I felt it was an artificial distinction to require that <branch> is a branch-ish thing rather than a commit-ish thing. Rebase already peels <upstream> and --onto so it feels inconsistent not to do it for <branch>. I guess the counter argument to that is users may be confused and start complaining that the tag itself is not rebased.
Quoted 3 lines
It would have been a different story if we had a problem when a tag is given to "--onto <there>", but I do not think this topic is about that case.
No "--onto <tag>" works fine. We also accept a tag object for upstream without requiring the user to peel it for us.
Quoted 4 lines
Having said that, even if we decide that we shouldn't accept the tag object and require peeled form to avoid mistakes (instead of silently peeling the tag ourselves), I do agree that
Quoted 8 lines
Quoted 2 lines
error:
update_reffailed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object710d743b2b9892457fdcc3970f397e6ec07447e0to branch 'HEAD'is a bad error message for this. It should be something like
error: cannot rebase a tag
perhaps.
We could do that if we're worried that users would be confused by the tag not being rebased if we started automatically peeling <branch>. (I'm kind of leaning in that direction at the moment having read your email)
Best Wishes
Phillip
Quoted 8 lines
But if we auto-peeled in an old version, I do not mind this series (but let's drop pointless "clean-up" that is not, like what was pointed out by Réne). In such a case, the first paragraph should say, instead of "should checkout", that "we used to do X, but commit Y broke us and now we die with an error message".
Thanks.
Quoted 4 lines · Phillip Wood
Quoted 3 lines
Quoted 2 lines
Did we auto-peel in scripted version of "git rebase" and is this a regression when the command was rewritten in C?
As far as I can tell we have never peeled tags here
Quoted 8 lines
That's a bit misleading. We have never peeled a tag given as <branch> when we parse it. In the scripted version we just passed the tag oid along to rev-list, checkout and reset and they peeled it. So I think this is actually a regression in the builtin rebase. I'll update the commit message to reflect that unless we feel that allowing a tag for <branch> is a mistake and we should be erroring out to avoid the possible confusion of the tag not being rebased, only the commits it points to.
OK, so this is a regression fix. That makes the change much simpler to sell. I'd expect that the description would be along the lines of something like this, perhaps.
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>.
This used to work, even when <B> is a tag that points at a commit, until Git X.Y.Z 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.
Fix this old regression by doing ....
Thanks for digging (and fixing, of course).
v3 cover rebase: dereference tags
In reply to Phillip Wood via GitGitGadgetThanks 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.
* 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.
---
Signature · 1 line
gitgitgadget
PATCH v3 1/10 t3407: run tests in $TEST_DIRECTORY
From: Phillip Wood <[email protected]>
Commit 97b88dd58c ("git-rebase.sh: Fix --merge --abort failures when path contains whitespace", 2008-05-04) started running these tests in a subdirectory with a space in its name. At that time $TEST_DIRECTORY did not contain a space but shortly after in 4a7aaccd83 ("Rename the test trash directory to contain spaces.", 2008-05-04) $TEST_DIRECTORY was changed to contain a space so we no longer need to run these tests in a subdirectory.
--- t/t3407-rebase-abort.sh | 13 ------------- 1 file changed, 13 deletions(-)
t/t3407-rebase-abort.sh +0 -13
@@ -7,13 +7,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . ./test-lib.sh -### Test that we handle space characters properly -work_dir="$(pwd)/test dir" - test_expect_success setup ' - mkdir -p "$work_dir" && - cd "$work_dir" && - git init && echo a > a && git add a && git commit -m a &&
@@ -37,7 +31,6 @@ testrebase() { dotest=$2 test_expect_success "rebase$type --abort" ' - cd "$work_dir" && # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase$type main &&
@@ -48,7 +41,6 @@ testrebase() { ' test_expect_success "rebase$type --abort after --skip" ' - cd "$work_dir" && # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase$type main &&
@@ -61,7 +53,6 @@ testrebase() { ' test_expect_success "rebase$type --abort after --continue" ' - cd "$work_dir" && # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase$type main &&
@@ -77,7 +68,6 @@ testrebase() { ' test_expect_success "rebase$type --abort does not update reflog" ' - cd "$work_dir" && # Clean up the state from the previous one git reset --hard pre-rebase && git reflog show to-rebase > reflog_before &&
@@ -89,7 +79,6 @@ testrebase() { ' test_expect_success 'rebase --abort can not be used with other options' ' - cd "$work_dir" && # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase$type main &&
@@ -103,7 +92,6 @@ testrebase " --apply" .git/rebase-apply testrebase " --merge" .git/rebase-merge test_expect_success 'rebase --apply --quit' ' - cd "$work_dir" && # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase --apply main &&
@@ -115,7 +103,6 @@ test_expect_success 'rebase --apply --quit' ' ' test_expect_success 'rebase --merge --quit' ' - cd "$work_dir" && # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase --merge main &&
Signature · 1 line
gitgitgadget
PATCH v3 2/10 t3407: use test_commit
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
Simplify the setup by using test_commit. Note that this replaces the branch "pre-rebase" with a tag of the same name. "pre-rebase" is only used as a fixed reference point so it makes sense to use a tag rather than a branch.
--- t/t3407-rebase-abort.sh | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)
t/t3407-rebase-abort.sh +5 -12
@@ -8,22 +8,15 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . ./test-lib.sh test_expect_success setup ' - echo a > a && - git add a && - git commit -m a && + test_commit a a a && git branch to-rebase && - echo b > a && - git commit -a -m b && - echo c > a && - git commit -a -m c && + test_commit b a b && + test_commit c a c && git checkout to-rebase && - echo d > a && - git commit -a -m "merge should fail on this" && - echo e > a && - git commit -a -m "merge should fail on this, too" && - git branch pre-rebase + test_commit "merge should fail on this" a d d && + test_commit "merge should fail on this, too" a e pre-rebase ' testrebase() {
Signature · 1 line
gitgitgadget
PATCH v3 3/10 t3407: use test_cmp_rev
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
This provides better diagnostics if a test fails
--- t/t3407-rebase-abort.sh | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
t/t3407-rebase-abort.sh +7 -7
@@ -29,7 +29,7 @@ testrebase() { test_must_fail git rebase$type main && test_path_is_dir "$dotest" && git rebase --abort && - test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) && + test_cmp_rev to-rebase pre-rebase && test ! -d "$dotest" '
@@ -39,9 +39,9 @@ testrebase() { test_must_fail git rebase$type main && test_path_is_dir "$dotest" && test_must_fail git rebase --skip && - test $(git rev-parse HEAD) = $(git rev-parse main) && + test_cmp_rev HEAD main && git rebase --abort && - test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) && + test_cmp_rev to-rebase pre-rebase && test ! -d "$dotest" '
@@ -54,9 +54,9 @@ testrebase() { echo d >> a && git add a && test_must_fail git rebase --continue && - test $(git rev-parse HEAD) != $(git rev-parse main) && + test_cmp_rev ! HEAD main && git rebase --abort && - test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) && + test_cmp_rev to-rebase pre-rebase && test ! -d "$dotest" '
@@ -91,7 +91,7 @@ test_expect_success 'rebase --apply --quit' ' test_path_is_dir .git/rebase-apply && head_before=$(git rev-parse HEAD) && git rebase --quit && - test $(git rev-parse HEAD) = $head_before && + test_cmp_rev HEAD $head_before && test ! -d .git/rebase-apply '
@@ -102,7 +102,7 @@ test_expect_success 'rebase --merge --quit' ' test_path_is_dir .git/rebase-merge && head_before=$(git rev-parse HEAD) && git rebase --quit && - test $(git rev-parse HEAD) = $head_before && + test_cmp_rev HEAD $head_before && test ! -d .git/rebase-merge '
Signature · 1 line
gitgitgadget
PATCH v3 4/10 t3407: rename a variable
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
$dotest holds the name of the rebase state directory and was named after the state directory used at the time the test was added. As we no longer use that name rename the variable to reflect its purpose.
--- t/t3407-rebase-abort.sh | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
t/t3407-rebase-abort.sh +7 -7
@@ -21,35 +21,35 @@ test_expect_success setup ' testrebase() { type=$1 - dotest=$2 + state_dir=$2 test_expect_success "rebase$type --abort" ' # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase$type main && - test_path_is_dir "$dotest" && + test_path_is_dir "$state_dir" && git rebase --abort && test_cmp_rev to-rebase pre-rebase && - test ! -d "$dotest" + test ! -d "$state_dir" ' test_expect_success "rebase$type --abort after --skip" ' # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase$type main && - test_path_is_dir "$dotest" && + test_path_is_dir "$state_dir" && test_must_fail git rebase --skip && test_cmp_rev HEAD main && git rebase --abort && test_cmp_rev to-rebase pre-rebase && - test ! -d "$dotest" + test ! -d "$state_dir" ' test_expect_success "rebase$type --abort after --continue" ' # Clean up the state from the previous one git reset --hard pre-rebase && test_must_fail git rebase$type main && - test_path_is_dir "$dotest" && + test_path_is_dir "$state_dir" && echo c > a && echo d >> a && git add a &&
@@ -57,7 +57,7 @@ testrebase() { test_cmp_rev ! HEAD main && git rebase --abort && test_cmp_rev to-rebase pre-rebase && - test ! -d "$dotest" + test ! -d "$state_dir" ' test_expect_success "rebase$type --abort does not update reflog" '
Signature · 1 line
gitgitgadget
PATCH v3 5/10 t3407: use test_path_is_missing
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
At the end of the test we expect the state directory to be missing, but the tests only check it is not a directory.
--- t/t3407-rebase-abort.sh | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
t/t3407-rebase-abort.sh +5 -5
@@ -30,7 +30,7 @@ testrebase() { test_path_is_dir "$state_dir" && git rebase --abort && test_cmp_rev to-rebase pre-rebase && - test ! -d "$state_dir" + test_path_is_missing "$state_dir" ' test_expect_success "rebase$type --abort after --skip" '
@@ -42,7 +42,7 @@ testrebase() { test_cmp_rev HEAD main && git rebase --abort && test_cmp_rev to-rebase pre-rebase && - test ! -d "$state_dir" + test_path_is_missing "$state_dir" ' test_expect_success "rebase$type --abort after --continue" '
@@ -57,7 +57,7 @@ testrebase() { test_cmp_rev ! HEAD main && git rebase --abort && test_cmp_rev to-rebase pre-rebase && - test ! -d "$state_dir" + test_path_is_missing "$state_dir" ' test_expect_success "rebase$type --abort does not update reflog" '
@@ -92,7 +92,7 @@ test_expect_success 'rebase --apply --quit' ' head_before=$(git rev-parse HEAD) && git rebase --quit && test_cmp_rev HEAD $head_before && - test ! -d .git/rebase-apply + test_path_is_missing .git/rebase-apply ' test_expect_success 'rebase --merge --quit' '
@@ -103,7 +103,7 @@ test_expect_success 'rebase --merge --quit' ' head_before=$(git rev-parse HEAD) && git rebase --quit && test_cmp_rev HEAD $head_before && - test ! -d .git/rebase-merge + test_path_is_missing .git/rebase-merge ' test_done
Signature · 1 line
gitgitgadget
PATCH v3 6/10 t3407: strengthen rebase --abort tests
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
The existing tests only check that HEAD points to the correct commit after aborting, they do not check that the original branch is checked out.
--- t/t3407-rebase-abort.sh | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
t/t3407-rebase-abort.sh +10 -3
@@ -19,6 +19,13 @@ test_expect_success setup ' test_commit "merge should fail on this, too" a e pre-rebase ' +# Check that HEAD is equal to "pre-rebase" and the current branch is +# "to-rebase" +check_head() { + test_cmp_rev HEAD pre-rebase && + test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase +} + testrebase() { type=$1 state_dir=$2
@@ -29,7 +36,7 @@ testrebase() { test_must_fail git rebase$type main && test_path_is_dir "$state_dir" && git rebase --abort && - test_cmp_rev to-rebase pre-rebase && + check_head && test_path_is_missing "$state_dir" '
@@ -41,7 +48,7 @@ testrebase() { test_must_fail git rebase --skip && test_cmp_rev HEAD main && git rebase --abort && - test_cmp_rev to-rebase pre-rebase && + check_head && test_path_is_missing "$state_dir" '
@@ -56,7 +63,7 @@ testrebase() { test_must_fail git rebase --continue && test_cmp_rev ! HEAD main && git rebase --abort && - test_cmp_rev to-rebase pre-rebase && + check_head && test_path_is_missing "$state_dir" '
Signature · 1 line
gitgitgadget
PATCH v3 7/10 t3407: rework rebase --quit tests
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
9512177b68 ("rebase: add --quit to cleanup rebase, leave everything else untouched", 2016-11-12) seems to have copied the --abort tests but added two separate tests for the two rebase backends rather than adding a single test into the existing testrebase() function.
--- t/t3407-rebase-abort.sh | 34 ++++++++++++---------------------- 1 file changed, 12 insertions(+), 22 deletions(-)
t/t3407-rebase-abort.sh +12 -22
@@ -86,31 +86,21 @@ testrebase() { test_must_fail git rebase --abort -v && git rebase --abort ' + + test_expect_success "rebase$type --quit" ' + test_when_finished "git symbolic-ref HEAD refs/heads/to-rebase" && + # Clean up the state from the previous one + git reset --hard pre-rebase && + test_must_fail git rebase$type main && + test_path_is_dir $state_dir && + head_before=$(git rev-parse HEAD) && + git rebase --quit && + test_cmp_rev HEAD $head_before && + test_path_is_missing .git/rebase-apply + ' } testrebase " --apply" .git/rebase-apply testrebase " --merge" .git/rebase-merge -test_expect_success 'rebase --apply --quit' ' - # Clean up the state from the previous one - git reset --hard pre-rebase && - test_must_fail git rebase --apply main && - test_path_is_dir .git/rebase-apply && - head_before=$(git rev-parse HEAD) && - git rebase --quit && - test_cmp_rev HEAD $head_before && - test_path_is_missing .git/rebase-apply -' - -test_expect_success 'rebase --merge --quit' ' - # Clean up the state from the previous one - git reset --hard pre-rebase && - test_must_fail git rebase --merge main && - test_path_is_dir .git/rebase-merge && - head_before=$(git rev-parse HEAD) && - git rebase --quit && - test_cmp_rev HEAD $head_before && - test_path_is_missing .git/rebase-merge -' - test_done
Signature · 1 line
gitgitgadget
PATCH v3 8/10 rebase: use our standard error return value
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
Git uses −1 to signal an error. The builtin rebase converts these to +1 all over the place using !! (presumably because the in the scripted version an error was signalled by +1). This is confusing and clutters the code, we only need to convert the value when the function returns.
--- builtin/rebase.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
builtin/rebase.c +9 -9
@@ -1574,7 +1574,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)die (_ ("could not move back to %s" ),oid_to_hex (&options.orig_head));remove_branch_state (the_repository,0 ); - ret = !!finish_rebase (&options); + ret =finish_rebase (&options); goto cleanup; }case ACTION_QUIT : {
@@ -1583,11 +1583,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)struct replay_opts replay =REPLAY_OPTS_INIT ; replay.action =REPLAY_INTERACTIVE_REBASE ; - ret = !!sequencer_remove_state (&replay); + ret =sequencer_remove_state (&replay); }else {strbuf_reset (&buf);strbuf_addstr (&buf, options.state_dir); - ret = !!remove_dir_recursively (&buf,0 ); + ret =remove_dir_recursively (&buf,0 );if (ret)error (_ ("could not remove '%s'" ), options.state_dir);
@@ -1959,7 +1959,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)if (require_clean_work_tree (the_repository,"rebase" ,_ ("Please commit or stash them." ),1 ,1 )) { - ret =1 ; + ret =-1 ; goto cleanup; }
@@ -1994,7 +1994,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) RESET_HEAD_RUN_POST_CHECKOUT_HOOK, NULL, buf.buf, DEFAULT_REFLOG_ACTION) < 0) { - ret = !!error (_ ("could not switch to " + ret =error (_ ("could not switch to " "%s" ), options.switch_to); goto cleanup;
@@ -2009,7 +2009,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)else printf (_ ("Current branch %s is up to date.\n" ), branch_name); - ret = !!finish_rebase (&options); + ret =finish_rebase (&options); goto cleanup; }else if (!(options.flags &REBASE_NO_QUIET )) ;/* be quiet */
@@ -2087,7 +2087,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)RESET_HEAD_REFS_ONLY ,"HEAD" , msg.buf, DEFAULT_REFLOG_ACTION);strbuf_release (&msg); - ret = !!finish_rebase (&options); + ret =finish_rebase (&options); goto cleanup; }
@@ -2101,7 +2101,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) options.revisions = revisions.buf; run_rebase: - ret = !!run_specific_rebase (&options, action); + ret =run_specific_rebase (&options, action); cleanup:strbuf_release (&buf);
@@ -2112,5 +2112,5 @@ cleanup:free (options.strategy);strbuf_release (&options.git_format_patch_opt);free (squash_onto_name); -return ret; +return !!ret; }
Signature · 1 line
gitgitgadget
PATCH v3 9/10 rebase: use lookup_commit_reference_by_name()
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
peel_committish() appears to have been copied from the scripted rebase but it duplicates the functionality of lookup_commit_reference_by_name() so lets use that instead.
--- builtin/rebase.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-)
builtin/rebase.c +4 -13
@@ -762,17 +762,6 @@ static int finish_rebase(struct rebase_options *opts)return ret; } -static struct commit *peel_committish (const char *name) -{ -struct object *obj; -struct object_id oid; - -if (get_oid (name, &oid)) -return NULL ; - obj =parse_object (the_repository, &oid); -return (struct commit *)peel_to_type (name,0 , obj,OBJ_COMMIT ); -} -static void add_var (struct strbuf *buf,const char *name,const char *value) {if (!value)
@@ -1845,7 +1834,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)if (!strcmp (options.upstream_name,"-" )) options.upstream_name ="@{-1}" ; } - options.upstream =peel_committish (options.upstream_name); + options.upstream = +lookup_commit_reference_by_name (options.upstream_name);if (!options.upstream)die (_ ("invalid upstream '%s'" ), options.upstream_name); options.upstream_arg = options.upstream_name;
@@ -1888,7 +1878,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) options.onto =lookup_commit_or_die (&merge_base, options.onto_name); }else { - options.onto =peel_committish (options.onto_name); + options.onto = +lookup_commit_reference_by_name (options.onto_name);if (!options.onto)die (_ ("Does not point to a valid commit '%s'" ), options.onto_name);
Signature · 1 line
gitgitgadget
PATCH v3 10/10 rebase: dereference tags
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
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>.
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.
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.
---
--- builtin/rebase.c | 14 ++++++++------ t/t3407-rebase-abort.sh | 18 ++++++++++++++---- 2 files changed, 22 insertions(+), 10 deletions(-)
builtin/rebase.c +8 -6
@@ -1904,13 +1904,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)die_if_checked_out (buf.buf,1 ); options.head_name =xstrdup (buf.buf);/* If not is it a valid ref (branch or commit)? */ - }else if (!get_oid (branch_name, &options.orig_head) && -lookup_commit_reference (the_repository, - &options.orig_head)) + }else { +struct commit *commit = +lookup_commit_reference_by_name (branch_name); +if (!commit) +die (_ ("no such branch/commit '%s'" ), + branch_name); +oidcpy (&options.orig_head, &commit->object.oid); options.head_name =NULL ; -else -die (_ ("no such branch/commit '%s'" ), - branch_name); + } }else if (argc ==0 ) {/* Do not need to switch branches, we are already on it. */ options.head_name =
t/t3407-rebase-abort.sh +14 -4
@@ -11,18 +11,18 @@ test_expect_success setup ' test_commit a a a && git branch to-rebase && - test_commit b a b && - test_commit c a c && + test_commit --annotate b a b && + test_commit --annotate c a c && git checkout to-rebase && test_commit "merge should fail on this" a d d && - test_commit "merge should fail on this, too" a e pre-rebase + test_commit --annotate "merge should fail on this, too" a e pre-rebase ' # Check that HEAD is equal to "pre-rebase" and the current branch is # "to-rebase" check_head() { - test_cmp_rev HEAD pre-rebase && + test_cmp_rev HEAD pre-rebase^{commit} && test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase }
@@ -67,6 +67,16 @@ testrebase() { test_path_is_missing "$state_dir" ' + test_expect_success "rebase$type --abort when checking out a tag" ' + test_when_finished "git symbolic-ref HEAD refs/heads/to-rebase" && + git reset --hard a -- && + test_must_fail git rebase$type --onto b c pre-rebase && + test_cmp_rev HEAD b^{commit} && + git rebase --abort && + test_cmp_rev HEAD pre-rebase^{commit} && + ! git symbolic-ref HEAD + ' + test_expect_success "rebase$type --abort does not update reflog" ' # Clean up the state from the previous one git reset --hard pre-rebase &&
Signature · 1 line
gitgitgadget
Quoted 7 lines · Phillip Wood via GitGitGadget
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 JunioCover letter for v2: Thanks to Ævar and Johannes for their comments.
This version looks good to me.
Quoted 99 lines
* 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 onto31e4a0db03("Merge branch 'ab/rebase-fatal-fatal-fix'", 2021-09-08) to avoid a merge conflict that upset gitgitgadgetCover 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_reffailed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object710d743b2b9892457fdcc3970f397e6ec07447e0to branch 'HEAD'The fix for that is in the last patch, the rest of the patches are cleanups to t3407 and
builtin/rebase.cPhillip Wood (10):
t3407: run tests in $TEST_DIRECTORY
t3407: usetest_commit
t3407: usetest_cmp_rev
t3407: rename a variable
t3407: usetest_path_is_missing
t3407: strengthen rebase --abort tests
t3407: rework rebase --quit tests
rebase: use our standard error return value
rebase: uselookup_commit_reference_by_name()
rebase: dereference tagsbuiltin/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/1033Range-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_reffailed for ref 'HEAD': cannot update ref 'HEAD':
- trying to write non-commit object
-710d743b2b9892457fdcc3970f397e6ec07447e0to 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 usinglookup_commit_reference_by_name()when parsing
- the command line.
+ Fix this old regression by usinglookup_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 <[email protected]>---
--
gitgitgadget