From f20c1fb296be2a18426541573146159fdbcc668c Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Mon, 13 Sep 2021 15:19:12 +0000 Subject: t3407: run tests in $TEST_DIRECTORY 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. Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh index 7c381fb..c747bd3 100755 --- a/t/t3407-rebase-abort.sh +++ b/t/t3407-rebase-abort.sh @@ -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 && -- cgit v0.10.2-6-g49f6 From e505f452d4e7d0d4b603b1332ad742e74a5d5374 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Mon, 13 Sep 2021 15:19:13 +0000 Subject: t3407: use test_commit 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. Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh index c747bd3..0ad2196 100755 --- a/t/t3407-rebase-abort.sh +++ b/t/t3407-rebase-abort.sh @@ -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() { -- cgit v0.10.2-6-g49f6 From 7390c65df4fa997563ee2d1f61acd6ebb9697eac Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Mon, 13 Sep 2021 15:19:14 +0000 Subject: t3407: use test_cmp_rev This provides better diagnostics if a test fails Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh index 0ad2196..39076ac 100755 --- a/t/t3407-rebase-abort.sh +++ b/t/t3407-rebase-abort.sh @@ -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 ' -- cgit v0.10.2-6-g49f6 From 54627db03a60baec8fee7390b25b6ac806a9ecff Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Mon, 13 Sep 2021 15:19:15 +0000 Subject: t3407: rename a variable $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. Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh index 39076ac..2c70230 100755 --- a/t/t3407-rebase-abort.sh +++ b/t/t3407-rebase-abort.sh @@ -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" ' -- cgit v0.10.2-6-g49f6 From 0b7ae738a6a166d3b60af68c9b0984971baa7718 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Mon, 13 Sep 2021 15:19:16 +0000 Subject: t3407: use test_path_is_missing At the end of the test we expect the state directory to be missing, but the tests only check it is not a directory. Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh index 2c70230..7eba9ec 100755 --- a/t/t3407-rebase-abort.sh +++ b/t/t3407-rebase-abort.sh @@ -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 -- cgit v0.10.2-6-g49f6 From 1e14bc11ed01e876809e62d2b13db71d6c0d265c Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Mon, 13 Sep 2021 15:19:17 +0000 Subject: t3407: strengthen rebase --abort tests 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. Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh index 7eba9ec..f826444 100755 --- a/t/t3407-rebase-abort.sh +++ b/t/t3407-rebase-abort.sh @@ -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" ' -- cgit v0.10.2-6-g49f6 From d045719ac8ab2b2c6f4d7ce61ad3d35843556bbf Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Mon, 13 Sep 2021 15:19:18 +0000 Subject: t3407: rework rebase --quit tests 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. Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh index f826444..162112b 100755 --- a/t/t3407-rebase-abort.sh +++ b/t/t3407-rebase-abort.sh @@ -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 -- cgit v0.10.2-6-g49f6 From 35f070b4de8fa01c11d9478297ea074d633bd7d5 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Tue, 21 Sep 2021 10:24:05 +0000 Subject: rebase: use our standard error return value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano diff --git a/builtin/rebase.c b/builtin/rebase.c index eb01f4d..8c810d4 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1575,7 +1575,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: { @@ -1584,11 +1584,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); @@ -1960,7 +1960,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; } @@ -1995,7 +1995,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; @@ -2010,7 +2010,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 */ @@ -2088,7 +2088,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; } @@ -2102,7 +2102,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); @@ -2113,5 +2113,5 @@ cleanup: free(options.strategy); strbuf_release(&options.git_format_patch_opt); free(squash_onto_name); - return ret; + return !!ret; } -- cgit v0.10.2-6-g49f6 From 1d188263e0847fd356d7ff245c32e8ebb29115b3 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Tue, 21 Sep 2021 10:24:06 +0000 Subject: rebase: use lookup_commit_reference_by_name() 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. Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano diff --git a/builtin/rebase.c b/builtin/rebase.c index 8c810d4..e89e21d 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -763,17 +763,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) @@ -1846,7 +1835,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; @@ -1889,7 +1879,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); -- cgit v0.10.2-6-g49f6 From 7740ac691d8e7f1bed67bcbdb1ee5c5c618f7373 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Tue, 21 Sep 2021 10:24:07 +0000 Subject: rebase: dereference tags A rebase started with 'git rebase ' is conceptually to first checkout and run 'git rebase ' starting from that state. 'git rebase --abort' in the middle of such a rebase should take us back to the state we checked out . This used to work, even when 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 . 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 Signed-off-by: Junio C Hamano diff --git a/builtin/rebase.c b/builtin/rebase.c index e89e21d..f82bfae 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1905,13 +1905,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 = diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh index 162112b..ebbaed1 100755 --- a/t/t3407-rebase-abort.sh +++ b/t/t3407-rebase-abort.sh @@ -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 && -- cgit v0.10.2-6-g49f6