* [PATCH v2 01/11] t3407: run tests in $TEST_DIRECTORY
2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
@ 2021-09-13 15:19 ` Phillip Wood via GitGitGadget
2021-09-13 15:19 ` [PATCH v2 02/11] t3407: use test_commit Phillip Wood via GitGitGadget
` (11 subsequent siblings)
12 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-13 15:19 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, Phillip Wood, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
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 <phillip.wood@dunelm.org.uk>
---
t/t3407-rebase-abort.sh | 13 -------------
1 file changed, 13 deletions(-)
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 7c381fbc89a..c747bd31d76 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 &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v2 02/11] t3407: use test_commit
2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
2021-09-13 15:19 ` [PATCH v2 01/11] t3407: run tests in $TEST_DIRECTORY Phillip Wood via GitGitGadget
@ 2021-09-13 15:19 ` Phillip Wood via GitGitGadget
2021-09-13 15:19 ` [PATCH v2 03/11] t3407: use test_cmp_rev Phillip Wood via GitGitGadget
` (10 subsequent siblings)
12 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-13 15:19 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, Phillip Wood, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
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 <phillip.wood@dunelm.org.uk>
---
t/t3407-rebase-abort.sh | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index c747bd31d76..0ad21966bc5 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() {
--
gitgitgadget
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v2 03/11] t3407: use test_cmp_rev
2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
2021-09-13 15:19 ` [PATCH v2 01/11] t3407: run tests in $TEST_DIRECTORY Phillip Wood via GitGitGadget
2021-09-13 15:19 ` [PATCH v2 02/11] t3407: use test_commit Phillip Wood via GitGitGadget
@ 2021-09-13 15:19 ` Phillip Wood via GitGitGadget
2021-09-13 15:19 ` [PATCH v2 04/11] t3407: rename a variable Phillip Wood via GitGitGadget
` (9 subsequent siblings)
12 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-13 15:19 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, Phillip Wood, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
This provides better diagnostics if a test fails
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
t/t3407-rebase-abort.sh | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 0ad21966bc5..39076ac9462 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
'
--
gitgitgadget
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v2 04/11] t3407: rename a variable
2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
` (2 preceding siblings ...)
2021-09-13 15:19 ` [PATCH v2 03/11] t3407: use test_cmp_rev Phillip Wood via GitGitGadget
@ 2021-09-13 15:19 ` Phillip Wood via GitGitGadget
2021-09-13 15:19 ` [PATCH v2 05/11] t3407: use test_path_is_missing Phillip Wood via GitGitGadget
` (8 subsequent siblings)
12 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-13 15:19 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, Phillip Wood, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
$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 <phillip.wood@dunelm.org.uk>
---
t/t3407-rebase-abort.sh | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 39076ac9462..2c70230a4eb 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" '
--
gitgitgadget
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v2 05/11] t3407: use test_path_is_missing
2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
` (3 preceding siblings ...)
2021-09-13 15:19 ` [PATCH v2 04/11] t3407: rename a variable Phillip Wood via GitGitGadget
@ 2021-09-13 15:19 ` Phillip Wood via GitGitGadget
2021-09-13 15:19 ` [PATCH v2 06/11] t3407: strengthen rebase --abort tests Phillip Wood via GitGitGadget
` (7 subsequent siblings)
12 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-13 15:19 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, Phillip Wood, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
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 <phillip.wood@dunelm.org.uk>
---
t/t3407-rebase-abort.sh | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 2c70230a4eb..7eba9ec1619 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
--
gitgitgadget
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v2 06/11] t3407: strengthen rebase --abort tests
2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
` (4 preceding siblings ...)
2021-09-13 15:19 ` [PATCH v2 05/11] t3407: use test_path_is_missing Phillip Wood via GitGitGadget
@ 2021-09-13 15:19 ` Phillip Wood via GitGitGadget
2021-09-13 15:19 ` [PATCH v2 07/11] t3407: rework rebase --quit tests Phillip Wood via GitGitGadget
` (6 subsequent siblings)
12 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-13 15:19 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, Phillip Wood, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
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 <phillip.wood@dunelm.org.uk>
---
t/t3407-rebase-abort.sh | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 7eba9ec1619..f8264449b6c 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"
'
--
gitgitgadget
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v2 07/11] t3407: rework rebase --quit tests
2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
` (5 preceding siblings ...)
2021-09-13 15:19 ` [PATCH v2 06/11] t3407: strengthen rebase --abort tests Phillip Wood via GitGitGadget
@ 2021-09-13 15:19 ` Phillip Wood via GitGitGadget
2021-09-13 15:19 ` [PATCH v2 08/11] rebase: remove redundant strbuf Phillip Wood via GitGitGadget
` (5 subsequent siblings)
12 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-13 15:19 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, Phillip Wood, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
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 <phillip.wood@dunelm.org.uk>
---
t/t3407-rebase-abort.sh | 34 ++++++++++++----------------------
1 file changed, 12 insertions(+), 22 deletions(-)
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index f8264449b6c..162112ba5ea 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
--
gitgitgadget
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v2 08/11] rebase: remove redundant strbuf
2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
` (6 preceding siblings ...)
2021-09-13 15:19 ` [PATCH v2 07/11] t3407: rework rebase --quit tests Phillip Wood via GitGitGadget
@ 2021-09-13 15:19 ` Phillip Wood via GitGitGadget
2021-09-13 18:34 ` René Scharfe
2021-09-13 15:19 ` [PATCH v2 09/11] rebase: use our standard error return value Phillip Wood via GitGitGadget
` (4 subsequent siblings)
12 siblings, 1 reply; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-13 15:19 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, Phillip Wood, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
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).
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
builtin/rebase.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 6138009d6e4..69a67ab1252 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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;
}
--
gitgitgadget
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v2 08/11] rebase: remove redundant strbuf
2021-09-13 15:19 ` [PATCH v2 08/11] rebase: remove redundant strbuf Phillip Wood via GitGitGadget
@ 2021-09-13 18:34 ` René Scharfe
2021-09-13 22:40 ` Junio C Hamano
2021-09-14 10:33 ` Phillip Wood
0 siblings, 2 replies; 55+ messages in thread
From: René Scharfe @ 2021-09-13 18:34 UTC (permalink / raw)
To: Phillip Wood via GitGitGadget, git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, Phillip Wood
Am 13.09.21 um 17:19 schrieb Phillip Wood via GitGitGadget:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 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)?
> 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).
strbuf_reset() after strbuf_release() is redundant but legal.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> builtin/rebase.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 6138009d6e4..69a67ab1252 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -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;
> }
>
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é
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [PATCH v2 08/11] rebase: remove redundant strbuf
2021-09-13 18:34 ` René Scharfe
@ 2021-09-13 22:40 ` Junio C Hamano
2021-09-14 10:31 ` Phillip Wood
2021-09-14 10:33 ` Phillip Wood
1 sibling, 1 reply; 55+ messages in thread
From: Junio C Hamano @ 2021-09-13 22:40 UTC (permalink / raw)
To: René Scharfe
Cc: Phillip Wood via GitGitGadget, git,
Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, Phillip Wood
René Scharfe <l.s.r@web.de> writes:
> Am 13.09.21 um 17:19 schrieb Phillip Wood via GitGitGadget:
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> 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)?
>
>> 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).
>
> strbuf_reset() after strbuf_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.
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 08/11] rebase: remove redundant strbuf
2021-09-13 22:40 ` Junio C Hamano
@ 2021-09-14 10:31 ` Phillip Wood
0 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood @ 2021-09-14 10:31 UTC (permalink / raw)
To: Junio C Hamano, René Scharfe
Cc: Phillip Wood via GitGitGadget, git,
Ævar Arnfjörð Bjarmason, Johannes Schindelin,
Phillip Wood
On 13/09/2021 23:40, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
>
>> Am 13.09.21 um 17:19 schrieb Phillip Wood via GitGitGadget:
>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>
>>> 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.
>> The most important question: Is this patch really needed to support
>> tags (the purpose of this series)?
>>
>>> 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).
>>
>> strbuf_reset() after strbuf_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.
> 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.
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
> Thanks.
>
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 08/11] rebase: remove redundant strbuf
2021-09-13 18:34 ` René Scharfe
2021-09-13 22:40 ` Junio C Hamano
@ 2021-09-14 10:33 ` Phillip Wood
1 sibling, 0 replies; 55+ messages in thread
From: Phillip Wood @ 2021-09-14 10:33 UTC (permalink / raw)
To: René Scharfe, Phillip Wood via GitGitGadget, git
Cc: Ævar Arnfjörð Bjarmason, Johannes Schindelin,
Phillip Wood
Hi René
Thanks for looking at this series
On 13/09/2021 19:34, René Scharfe wrote:
> Am 13.09.21 um 17:19 schrieb Phillip Wood via GitGitGadget:
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 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.
That's a nice suggestion, I think it's much clearer which file we're
checking for.
Best Wishes
Phillip
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v2 09/11] rebase: use our standard error return value
2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
` (7 preceding siblings ...)
2021-09-13 15:19 ` [PATCH v2 08/11] rebase: remove redundant strbuf Phillip Wood via GitGitGadget
@ 2021-09-13 15:19 ` Phillip Wood via GitGitGadget
2021-09-13 15:19 ` [PATCH v2 10/11] rebase: use lookup_commit_reference_by_name() Phillip Wood via GitGitGadget
` (3 subsequent siblings)
12 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-13 15:19 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, Phillip Wood, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
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 <phillip.wood@dunelm.org.uk>
---
builtin/rebase.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 69a67ab1252..7905672d8de 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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;
}
--
gitgitgadget
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v2 10/11] rebase: use lookup_commit_reference_by_name()
2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
` (8 preceding siblings ...)
2021-09-13 15:19 ` [PATCH v2 09/11] rebase: use our standard error return value Phillip Wood via GitGitGadget
@ 2021-09-13 15:19 ` Phillip Wood via GitGitGadget
2021-09-13 15:19 ` [PATCH v2 11/11] rebase: dereference tags Phillip Wood via GitGitGadget
` (2 subsequent siblings)
12 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-13 15:19 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, Phillip Wood, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
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 <phillip.wood@dunelm.org.uk>
---
builtin/rebase.c | 17 ++++-------------
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 7905672d8de..74663208468 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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);
--
gitgitgadget
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v2 11/11] rebase: dereference tags
2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
` (9 preceding siblings ...)
2021-09-13 15:19 ` [PATCH v2 10/11] rebase: use lookup_commit_reference_by_name() Phillip Wood via GitGitGadget
@ 2021-09-13 15:19 ` Phillip Wood via GitGitGadget
2021-09-13 22:58 ` Junio C Hamano
` (2 more replies)
2021-09-14 4:02 ` [PATCH v2 00/11] " Elijah Newren
2021-09-21 10:23 ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
12 siblings, 3 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-13 15:19 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, Phillip Wood, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
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.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
---
builtin/rebase.c | 14 ++++++++------
t/t3407-rebase-abort.sh | 18 ++++++++++++++----
2 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 74663208468..2b70a196f9a 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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 =
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 162112ba5ea..ebbaed147a6 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 &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v2 11/11] rebase: dereference tags
2021-09-13 15:19 ` [PATCH v2 11/11] rebase: dereference tags Phillip Wood via GitGitGadget
@ 2021-09-13 22:58 ` Junio C Hamano
2021-09-14 10:17 ` Phillip Wood
2021-09-14 3:42 ` Elijah Newren
2021-09-14 9:48 ` Sergey Organov
2 siblings, 1 reply; 55+ messages in thread
From: Junio C Hamano @ 2021-09-13 22:58 UTC (permalink / raw)
To: Phillip Wood via GitGitGadget
Cc: git, Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, Phillip Wood
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 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
> error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object 710d743b2b9892457fdcc3970f397e6ec07447e0 to 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.
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [PATCH v2 11/11] rebase: dereference tags
2021-09-13 22:58 ` Junio C Hamano
@ 2021-09-14 10:17 ` Phillip Wood
2021-09-14 13:27 ` Phillip Wood
0 siblings, 1 reply; 55+ messages in thread
From: Phillip Wood @ 2021-09-14 10:17 UTC (permalink / raw)
To: Junio C Hamano, Phillip Wood via GitGitGadget
Cc: git, Ævar Arnfjörð Bjarmason, Johannes Schindelin,
Phillip Wood
Hi Junio
On 13/09/2021 23:58, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> 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.
> 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
> 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.
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.
> 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.
> 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
>
>> error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object 710d743b2b9892457fdcc3970f397e6ec07447e0 to 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
> 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.
>
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 11/11] rebase: dereference tags
2021-09-14 10:17 ` Phillip Wood
@ 2021-09-14 13:27 ` Phillip Wood
2021-09-14 16:29 ` Junio C Hamano
0 siblings, 1 reply; 55+ messages in thread
From: Phillip Wood @ 2021-09-14 13:27 UTC (permalink / raw)
To: Junio C Hamano, Phillip Wood via GitGitGadget
Cc: git, Ævar Arnfjörð Bjarmason, Johannes Schindelin,
Phillip Wood
On 14/09/2021 11:17, Phillip Wood wrote:
> Hi Junio
>
> On 13/09/2021 23:58, Junio C Hamano wrote:
>> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>
>>> 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.
>
>> 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
>> 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.
>
> 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.
>
>> 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.
>
>> 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
>>
>>> error: update_ref failed for ref 'HEAD': cannot update ref
>>> 'HEAD': trying to write non-commit object
>>> 710d743b2b9892457fdcc3970f397e6ec07447e0 to 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
>
>> 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.
>>
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 11/11] rebase: dereference tags
2021-09-14 13:27 ` Phillip Wood
@ 2021-09-14 16:29 ` Junio C Hamano
0 siblings, 0 replies; 55+ messages in thread
From: Junio C Hamano @ 2021-09-14 16:29 UTC (permalink / raw)
To: Phillip Wood
Cc: Phillip Wood via GitGitGadget, git,
Ævar Arnfjörð Bjarmason, Johannes Schindelin,
Phillip Wood
Phillip Wood <phillip.wood123@gmail.com> writes:
>>> 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.
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).
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 11/11] rebase: dereference tags
2021-09-13 15:19 ` [PATCH v2 11/11] rebase: dereference tags Phillip Wood via GitGitGadget
2021-09-13 22:58 ` Junio C Hamano
@ 2021-09-14 3:42 ` Elijah Newren
2021-09-14 9:48 ` Sergey Organov
2 siblings, 0 replies; 55+ messages in thread
From: Elijah Newren @ 2021-09-14 3:42 UTC (permalink / raw)
To: Phillip Wood via GitGitGadget
Cc: Git Mailing List, Ævar Arnfjörð Bjarmason,
Phillip Wood, Johannes Schindelin, Phillip Wood
On Mon, Sep 13, 2021 at 8:47 AM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Aborting a rebase stated with 'git rebase <upstream> <tag-object>'
s/stated/started/
> 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.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> ---
> builtin/rebase.c | 14 ++++++++------
> t/t3407-rebase-abort.sh | 18 ++++++++++++++----
> 2 files changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 74663208468..2b70a196f9a 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -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 =
> diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
> index 162112ba5ea..ebbaed147a6 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 &&
> --
> gitgitgadget
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [PATCH v2 11/11] rebase: dereference tags
2021-09-13 15:19 ` [PATCH v2 11/11] rebase: dereference tags Phillip Wood via GitGitGadget
2021-09-13 22:58 ` Junio C Hamano
2021-09-14 3:42 ` Elijah Newren
@ 2021-09-14 9:48 ` Sergey Organov
2021-09-14 9:58 ` Phillip Wood
2 siblings, 1 reply; 55+ messages in thread
From: Sergey Organov @ 2021-09-14 9:48 UTC (permalink / raw)
To: Phillip Wood via GitGitGadget
Cc: git, Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, Phillip Wood
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 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.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
[...]
> diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
> index 162112ba5ea..ebbaed147a6 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
> '
These two do not seem to belong to this particular commit?
>
> # 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
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [PATCH v2 11/11] rebase: dereference tags
2021-09-14 9:48 ` Sergey Organov
@ 2021-09-14 9:58 ` Phillip Wood
0 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood @ 2021-09-14 9:58 UTC (permalink / raw)
To: Sergey Organov, Phillip Wood via GitGitGadget
Cc: git, Ævar Arnfjörð Bjarmason, Johannes Schindelin,
Phillip Wood
Hi Sergey
On 14/09/2021 10:48, Sergey Organov wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> 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.
>>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> [...]
>
>> diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
>> index 162112ba5ea..ebbaed147a6 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
>> '
>
> 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
>> # 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
>
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 00/11] rebase: dereference tags
2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
` (10 preceding siblings ...)
2021-09-13 15:19 ` [PATCH v2 11/11] rebase: dereference tags Phillip Wood via GitGitGadget
@ 2021-09-14 4:02 ` Elijah Newren
2021-09-21 10:23 ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
12 siblings, 0 replies; 55+ messages in thread
From: Elijah Newren @ 2021-09-14 4:02 UTC (permalink / raw)
To: Phillip Wood via GitGitGadget
Cc: Git Mailing List, Ævar Arnfjörð Bjarmason,
Phillip Wood, Johannes Schindelin, Phillip Wood
On Mon, Sep 13, 2021 at 8:46 AM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> 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
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.
^ permalink raw reply [flat|nested] 55+ messages in thread* [PATCH v3 00/10] rebase: dereference tags
2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
` (11 preceding siblings ...)
2021-09-14 4:02 ` [PATCH v2 00/11] " Elijah Newren
@ 2021-09-21 10:23 ` Phillip Wood via GitGitGadget
2021-09-21 10:23 ` [PATCH v3 01/10] t3407: run tests in $TEST_DIRECTORY Phillip Wood via GitGitGadget
` (10 more replies)
12 siblings, 11 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-21 10:23 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, René Scharfe, Elijah Newren,
Sergey Organov, Phillip Wood
Thanks for the comments on V2. Here are the changes from that version:
* dropped the controversial cleanup that was patch 8
* reworded patch 10 as suggested by Junio
Cover letter for v2: Thanks to Ævar and Johannes for their comments.
* Changed "! test_cmp_rev" to "test_cmp_rev !" (suggested by Ævar)
* Fixed the quoting for the title of the "rebase --quit" tests.
* Reworked the last commit to handle the error case first (suggested by
Ævar)
* Tweaked the commit messages for patches 8 & 11
* Rebased onto 31e4a0db03 ("Merge branch 'ab/rebase-fatal-fatal-fix'",
2021-09-08) to avoid a merge conflict that upset gitgitgadget
Cover letter for V1: Aborting a rebase stated with git rebase <upstream>
<tag-object> should checkout the commit pointed to by . Instead it gives
error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'
The fix for that is in the last patch, the rest of the patches are cleanups
to t3407 and builtin/rebase.c
Phillip Wood (10):
t3407: run tests in $TEST_DIRECTORY
t3407: use test_commit
t3407: use test_cmp_rev
t3407: rename a variable
t3407: use test_path_is_missing
t3407: strengthen rebase --abort tests
t3407: rework rebase --quit tests
rebase: use our standard error return value
rebase: use lookup_commit_reference_by_name()
rebase: dereference tags
builtin/rebase.c | 49 ++++++++-----------
t/t3407-rebase-abort.sh | 105 ++++++++++++++++++----------------------
2 files changed, 67 insertions(+), 87 deletions(-)
base-commit: 31e4a0db0337e2aa972d9b9f11a332dff7c4cbcb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1033%2Fphillipwood%2Fwip%2Frebase-handle-tags-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1033/phillipwood/wip/rebase-handle-tags-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1033
Range-diff vs v2:
1: bac009d8543 = 1: bac009d8543 t3407: run tests in $TEST_DIRECTORY
2: abfffb31a56 = 2: abfffb31a56 t3407: use test_commit
3: 7755ce17fef = 3: 7755ce17fef t3407: use test_cmp_rev
4: 38eee11baf5 = 4: 38eee11baf5 t3407: rename a variable
5: 61a37c89f1e = 5: 61a37c89f1e t3407: use test_path_is_missing
6: 6866630528b = 6: 6866630528b t3407: strengthen rebase --abort tests
7: fd55a3196b1 = 7: fd55a3196b1 t3407: rework rebase --quit tests
8: ad3c4efc027 < -: ----------- rebase: remove redundant strbuf
9: ad940b633d0 ! 8: d3af62c746c rebase: use our standard error return value
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
} else if (!(options.flags & REBASE_NO_QUIET))
; /* be quiet */
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
- reset_head(the_repository, NULL, "Fast-forwarded", options.head_name,
- RESET_HEAD_REFS_ONLY, "HEAD", buf.buf,
+ RESET_HEAD_REFS_ONLY, "HEAD", msg.buf,
DEFAULT_REFLOG_ACTION);
+ strbuf_release(&msg);
- ret = !!finish_rebase(&options);
+ ret = finish_rebase(&options);
goto cleanup;
10: bc103e703e8 = 9: 4eedd3ae766 rebase: use lookup_commit_reference_by_name()
11: 951de6bb199 ! 10: 55a6250ab38 rebase: dereference tags
@@ Metadata
## Commit message ##
rebase: dereference tags
- Aborting a rebase stated with 'git rebase <upstream> <tag-object>'
- should checkout the commit pointed to by <tag-object>. Instead it gives
+ A rebase started with 'git rebase <A> <B>' is conceptually to first
+ checkout <B> and run 'git rebase <A>' starting from that state. 'git
+ rebase --abort' in the middle of such a rebase should take us back to
+ the state we checked out <B>.
- error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD':
- trying to write non-commit object
- 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'
+ This used to work, even when <B> is a tag that points at a commit,
+ until Git 2.20.0 when the command was reimplemented in C. The command
+ now complains that the tag object itself cannot be checked out, which
+ may be technically correct but is not what the user asked to do.
- This is because when we parse the command line arguments although we
- check that the tag points to a commit we remember the oid of the tag
- and try and checkout that object rather than the commit it points
- to. Fix this by using lookup_commit_reference_by_name() when parsing
- the command line.
+ Fix this old regression by using lookup_commit_reference_by_name()
+ when parsing <B>. The scripted version did not need to peel the tag
+ because the commands it passed the tag to (e.g 'git reset') peeled the
+ tag themselves.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
--
gitgitgadget
^ permalink raw reply [flat|nested] 55+ messages in thread* [PATCH v3 01/10] t3407: run tests in $TEST_DIRECTORY
2021-09-21 10:23 ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
@ 2021-09-21 10:23 ` Phillip Wood via GitGitGadget
2021-09-21 10:23 ` [PATCH v3 02/10] t3407: use test_commit Phillip Wood via GitGitGadget
` (9 subsequent siblings)
10 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-21 10:23 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, René Scharfe, Elijah Newren,
Sergey Organov, Phillip Wood, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
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 <phillip.wood@dunelm.org.uk>
---
t/t3407-rebase-abort.sh | 13 -------------
1 file changed, 13 deletions(-)
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 7c381fbc89a..c747bd31d76 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 &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v3 02/10] t3407: use test_commit
2021-09-21 10:23 ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
2021-09-21 10:23 ` [PATCH v3 01/10] t3407: run tests in $TEST_DIRECTORY Phillip Wood via GitGitGadget
@ 2021-09-21 10:23 ` Phillip Wood via GitGitGadget
2021-09-21 10:24 ` [PATCH v3 03/10] t3407: use test_cmp_rev Phillip Wood via GitGitGadget
` (8 subsequent siblings)
10 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-21 10:23 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, René Scharfe, Elijah Newren,
Sergey Organov, Phillip Wood, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
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 <phillip.wood@dunelm.org.uk>
---
t/t3407-rebase-abort.sh | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index c747bd31d76..0ad21966bc5 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() {
--
gitgitgadget
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v3 03/10] t3407: use test_cmp_rev
2021-09-21 10:23 ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
2021-09-21 10:23 ` [PATCH v3 01/10] t3407: run tests in $TEST_DIRECTORY Phillip Wood via GitGitGadget
2021-09-21 10:23 ` [PATCH v3 02/10] t3407: use test_commit Phillip Wood via GitGitGadget
@ 2021-09-21 10:24 ` Phillip Wood via GitGitGadget
2021-09-21 10:24 ` [PATCH v3 04/10] t3407: rename a variable Phillip Wood via GitGitGadget
` (7 subsequent siblings)
10 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-21 10:24 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, René Scharfe, Elijah Newren,
Sergey Organov, Phillip Wood, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
This provides better diagnostics if a test fails
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
t/t3407-rebase-abort.sh | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 0ad21966bc5..39076ac9462 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
'
--
gitgitgadget
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v3 04/10] t3407: rename a variable
2021-09-21 10:23 ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
` (2 preceding siblings ...)
2021-09-21 10:24 ` [PATCH v3 03/10] t3407: use test_cmp_rev Phillip Wood via GitGitGadget
@ 2021-09-21 10:24 ` Phillip Wood via GitGitGadget
2021-09-21 10:24 ` [PATCH v3 05/10] t3407: use test_path_is_missing Phillip Wood via GitGitGadget
` (6 subsequent siblings)
10 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-21 10:24 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, René Scharfe, Elijah Newren,
Sergey Organov, Phillip Wood, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
$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 <phillip.wood@dunelm.org.uk>
---
t/t3407-rebase-abort.sh | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 39076ac9462..2c70230a4eb 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" '
--
gitgitgadget
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v3 05/10] t3407: use test_path_is_missing
2021-09-21 10:23 ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
` (3 preceding siblings ...)
2021-09-21 10:24 ` [PATCH v3 04/10] t3407: rename a variable Phillip Wood via GitGitGadget
@ 2021-09-21 10:24 ` Phillip Wood via GitGitGadget
2021-09-21 10:24 ` [PATCH v3 06/10] t3407: strengthen rebase --abort tests Phillip Wood via GitGitGadget
` (5 subsequent siblings)
10 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-21 10:24 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, René Scharfe, Elijah Newren,
Sergey Organov, Phillip Wood, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
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 <phillip.wood@dunelm.org.uk>
---
t/t3407-rebase-abort.sh | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 2c70230a4eb..7eba9ec1619 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
--
gitgitgadget
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v3 06/10] t3407: strengthen rebase --abort tests
2021-09-21 10:23 ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
` (4 preceding siblings ...)
2021-09-21 10:24 ` [PATCH v3 05/10] t3407: use test_path_is_missing Phillip Wood via GitGitGadget
@ 2021-09-21 10:24 ` Phillip Wood via GitGitGadget
2021-09-21 10:24 ` [PATCH v3 07/10] t3407: rework rebase --quit tests Phillip Wood via GitGitGadget
` (4 subsequent siblings)
10 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-21 10:24 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, René Scharfe, Elijah Newren,
Sergey Organov, Phillip Wood, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
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 <phillip.wood@dunelm.org.uk>
---
t/t3407-rebase-abort.sh | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 7eba9ec1619..f8264449b6c 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"
'
--
gitgitgadget
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v3 07/10] t3407: rework rebase --quit tests
2021-09-21 10:23 ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
` (5 preceding siblings ...)
2021-09-21 10:24 ` [PATCH v3 06/10] t3407: strengthen rebase --abort tests Phillip Wood via GitGitGadget
@ 2021-09-21 10:24 ` Phillip Wood via GitGitGadget
2021-09-21 10:24 ` [PATCH v3 08/10] rebase: use our standard error return value Phillip Wood via GitGitGadget
` (3 subsequent siblings)
10 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-21 10:24 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, René Scharfe, Elijah Newren,
Sergey Organov, Phillip Wood, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
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 <phillip.wood@dunelm.org.uk>
---
t/t3407-rebase-abort.sh | 34 ++++++++++++----------------------
1 file changed, 12 insertions(+), 22 deletions(-)
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index f8264449b6c..162112ba5ea 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
--
gitgitgadget
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v3 08/10] rebase: use our standard error return value
2021-09-21 10:23 ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
` (6 preceding siblings ...)
2021-09-21 10:24 ` [PATCH v3 07/10] t3407: rework rebase --quit tests Phillip Wood via GitGitGadget
@ 2021-09-21 10:24 ` Phillip Wood via GitGitGadget
2021-09-21 10:24 ` [PATCH v3 09/10] rebase: use lookup_commit_reference_by_name() Phillip Wood via GitGitGadget
` (2 subsequent siblings)
10 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-21 10:24 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, René Scharfe, Elijah Newren,
Sergey Organov, Phillip Wood, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
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 <phillip.wood@dunelm.org.uk>
---
builtin/rebase.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 6138009d6e4..444e169ef74 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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;
}
--
gitgitgadget
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v3 09/10] rebase: use lookup_commit_reference_by_name()
2021-09-21 10:23 ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
` (7 preceding siblings ...)
2021-09-21 10:24 ` [PATCH v3 08/10] rebase: use our standard error return value Phillip Wood via GitGitGadget
@ 2021-09-21 10:24 ` Phillip Wood via GitGitGadget
2021-09-21 10:24 ` [PATCH v3 10/10] rebase: dereference tags Phillip Wood via GitGitGadget
2021-09-24 1:26 ` [PATCH v3 00/10] " Elijah Newren
10 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-21 10:24 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, René Scharfe, Elijah Newren,
Sergey Organov, Phillip Wood, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
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 <phillip.wood@dunelm.org.uk>
---
builtin/rebase.c | 17 ++++-------------
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 444e169ef74..0ace9e0a8ec 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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);
--
gitgitgadget
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v3 10/10] rebase: dereference tags
2021-09-21 10:23 ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
` (8 preceding siblings ...)
2021-09-21 10:24 ` [PATCH v3 09/10] rebase: use lookup_commit_reference_by_name() Phillip Wood via GitGitGadget
@ 2021-09-21 10:24 ` Phillip Wood via GitGitGadget
2021-09-24 1:26 ` [PATCH v3 00/10] " Elijah Newren
10 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-21 10:24 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, René Scharfe, Elijah Newren,
Sergey Organov, Phillip Wood, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
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.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
---
builtin/rebase.c | 14 ++++++++------
t/t3407-rebase-abort.sh | 18 ++++++++++++++----
2 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0ace9e0a8ec..b4433ee7978 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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 =
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 162112ba5ea..ebbaed147a6 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 &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v3 00/10] rebase: dereference tags
2021-09-21 10:23 ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
` (9 preceding siblings ...)
2021-09-21 10:24 ` [PATCH v3 10/10] rebase: dereference tags Phillip Wood via GitGitGadget
@ 2021-09-24 1:26 ` Elijah Newren
10 siblings, 0 replies; 55+ messages in thread
From: Elijah Newren @ 2021-09-24 1:26 UTC (permalink / raw)
To: Phillip Wood via GitGitGadget
Cc: Git Mailing List, Ævar Arnfjörð Bjarmason,
Phillip Wood, Johannes Schindelin, René Scharfe,
Sergey Organov, Phillip Wood
On Tue, Sep 21, 2021 at 3:24 AM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> Thanks for the comments on V2. Here are the changes from that version:
>
> * dropped the controversial cleanup that was patch 8
> * reworded patch 10 as suggested by Junio
>
> Cover letter for v2: Thanks to Ævar and Johannes for their comments.
This version looks good to me.
>
> * Changed "! test_cmp_rev" to "test_cmp_rev !" (suggested by Ævar)
> * Fixed the quoting for the title of the "rebase --quit" tests.
> * Reworked the last commit to handle the error case first (suggested by
> Ævar)
> * Tweaked the commit messages for patches 8 & 11
> * Rebased onto 31e4a0db03 ("Merge branch 'ab/rebase-fatal-fatal-fix'",
> 2021-09-08) to avoid a merge conflict that upset gitgitgadget
>
> Cover letter for V1: Aborting a rebase stated with git rebase <upstream>
> <tag-object> should checkout the commit pointed to by . Instead it gives
>
> error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'
>
>
> The fix for that is in the last patch, the rest of the patches are cleanups
> to t3407 and builtin/rebase.c
>
> Phillip Wood (10):
> t3407: run tests in $TEST_DIRECTORY
> t3407: use test_commit
> t3407: use test_cmp_rev
> t3407: rename a variable
> t3407: use test_path_is_missing
> t3407: strengthen rebase --abort tests
> t3407: rework rebase --quit tests
> rebase: use our standard error return value
> rebase: use lookup_commit_reference_by_name()
> rebase: dereference tags
>
> builtin/rebase.c | 49 ++++++++-----------
> t/t3407-rebase-abort.sh | 105 ++++++++++++++++++----------------------
> 2 files changed, 67 insertions(+), 87 deletions(-)
>
>
> base-commit: 31e4a0db0337e2aa972d9b9f11a332dff7c4cbcb
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1033%2Fphillipwood%2Fwip%2Frebase-handle-tags-v3
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1033/phillipwood/wip/rebase-handle-tags-v3
> Pull-Request: https://github.com/gitgitgadget/git/pull/1033
>
> Range-diff vs v2:
>
> 1: bac009d8543 = 1: bac009d8543 t3407: run tests in $TEST_DIRECTORY
> 2: abfffb31a56 = 2: abfffb31a56 t3407: use test_commit
> 3: 7755ce17fef = 3: 7755ce17fef t3407: use test_cmp_rev
> 4: 38eee11baf5 = 4: 38eee11baf5 t3407: rename a variable
> 5: 61a37c89f1e = 5: 61a37c89f1e t3407: use test_path_is_missing
> 6: 6866630528b = 6: 6866630528b t3407: strengthen rebase --abort tests
> 7: fd55a3196b1 = 7: fd55a3196b1 t3407: rework rebase --quit tests
> 8: ad3c4efc027 < -: ----------- rebase: remove redundant strbuf
> 9: ad940b633d0 ! 8: d3af62c746c rebase: use our standard error return value
> @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
> } else if (!(options.flags & REBASE_NO_QUIET))
> ; /* be quiet */
> @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
> - reset_head(the_repository, NULL, "Fast-forwarded", options.head_name,
> - RESET_HEAD_REFS_ONLY, "HEAD", buf.buf,
> + RESET_HEAD_REFS_ONLY, "HEAD", msg.buf,
> DEFAULT_REFLOG_ACTION);
> + strbuf_release(&msg);
> - ret = !!finish_rebase(&options);
> + ret = finish_rebase(&options);
> goto cleanup;
> 10: bc103e703e8 = 9: 4eedd3ae766 rebase: use lookup_commit_reference_by_name()
> 11: 951de6bb199 ! 10: 55a6250ab38 rebase: dereference tags
> @@ Metadata
> ## Commit message ##
> rebase: dereference tags
>
> - Aborting a rebase stated with 'git rebase <upstream> <tag-object>'
> - should checkout the commit pointed to by <tag-object>. Instead it gives
> + A rebase started with 'git rebase <A> <B>' is conceptually to first
> + checkout <B> and run 'git rebase <A>' starting from that state. 'git
> + rebase --abort' in the middle of such a rebase should take us back to
> + the state we checked out <B>.
>
> - error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD':
> - trying to write non-commit object
> - 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'
> + This used to work, even when <B> is a tag that points at a commit,
> + until Git 2.20.0 when the command was reimplemented in C. The command
> + now complains that the tag object itself cannot be checked out, which
> + may be technically correct but is not what the user asked to do.
>
> - This is because when we parse the command line arguments although we
> - check that the tag points to a commit we remember the oid of the tag
> - and try and checkout that object rather than the commit it points
> - to. Fix this by using lookup_commit_reference_by_name() when parsing
> - the command line.
> + Fix this old regression by using lookup_commit_reference_by_name()
> + when parsing <B>. The scripted version did not need to peel the tag
> + because the commands it passed the tag to (e.g 'git reset') peeled the
> + tag themselves.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>
> --
> gitgitgadget
^ permalink raw reply [flat|nested] 55+ messages in thread