rebase: dereference tags #1033
Closed
rebase: dereference tags #1033
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/submit |
|
Submitted as pull.1033.git.1631094563.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -7,37 +7,23 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME | |||
|
|
|||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Wed, Sep 08 2021, Phillip Wood via GitGitGadget wrote:
> 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.
FWIW you could use --no-tag to test_commit to keep the behavior, but
changing it here seems fine.
|
User |
| @@ -7,61 +7,45 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME | |||
|
|
|||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Wed, Sep 08 2021, Phillip Wood via GitGitGadget wrote:
> @@ -54,9 +54,9 @@ testrebase() {
> echo d >> a &&
> git add a &&
> test_must_fail git rebase --continue &&
> - test $(git rev-parse HEAD) != $(git rev-parse main) &&
> + ! test_cmp_rev HEAD main &&
Use "test_cmp_rev !", making the shell do the negation is troublesome
for the same reason we don't do it with git command, it potentially
hides segfaults (and that test helper invokes "git", hence the optional
"!" first rgument).
There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
On 08/09/2021 11:40, Ævar Arnfjörð Bjarmason wrote:
>
> On Wed, Sep 08 2021, Phillip Wood via GitGitGadget wrote:
>
>> @@ -54,9 +54,9 @@ testrebase() {
>> echo d >> a &&
>> git add a &&
>> test_must_fail git rebase --continue &&
>> - test $(git rev-parse HEAD) != $(git rev-parse main) &&
>> + ! test_cmp_rev HEAD main &&
>
> Use "test_cmp_rev !", making the shell do the negation is troublesome
> for the same reason we don't do it with git command, it potentially
> hides segfaults (and that test helper invokes "git", hence the optional
> "!" first argument).
Thanks, I didn't realize one could pass "!" to test_cmp_rev - it gives a
better message on failure as well as avoiding hidden segfaults
Best Wishes
Phillip
| @@ -7,13 +7,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME | |||
|
|
|||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Wed, Sep 08 2021, Phillip Wood via GitGitGadget wrote:
> 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.
Well spotted & a nice cleanup!
| @@ -7,77 +7,67 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME | |||
|
|
|||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Wed, Sep 08 2021, Phillip Wood via GitGitGadget wrote:
> 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.
> [...]
> +# Check that HEAD is equal to "pre-rebase" and the current branch is
> +# "to-rebase"
> +check_head() {
> + test_cmp_rev HEAD pre-rebase &&
> + test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase
> +}
I reflexively thought "use test_cmp here!", but in this case that's
pointless as any difference is noted by test_cmp_rev, we're really
seeing what HEAD is checked out to here. I see we use the same pattern
in various other tests...
| @@ -762,17 +762,6 @@ static int finish_rebase(struct rebase_options *opts) | |||
| return ret; | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Wed, Sep 08 2021, Phillip Wood via GitGitGadget wrote:
> 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 | 18 +++++++++++-------
> t/t3407-rebase-abort.sh | 18 ++++++++++++++----
> 2 files changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 93fcc0df2ad..8bf7660a24b 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1903,13 +1903,17 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> die_if_checked_out(buf.buf, 1);
> options.head_name = xstrdup(buf.buf);
> /* If not is it a valid ref (branch or commit)? */
> - } else if (!get_oid(branch_name, &options.orig_head) &&
> - lookup_commit_reference(the_repository,
> - &options.orig_head))
> - options.head_name = NULL;
> - else
> - die(_("fatal: no such branch/commit '%s'"),
> - branch_name);
> + } else {
> + struct commit *commit =
> + lookup_commit_reference_by_name(branch_name);
> + if (commit) {
> + oidcpy(&options.orig_head, &commit->object.oid);
> + options.head_name = NULL;
> + } else {
> + die(_("fatal: no such branch/commit '%s'"),
> + branch_name);
> + }
> + }
Suggested style nit:
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 8bf7660a24b..c751ef866fd 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1906,13 +1906,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
} else {
struct commit *commit =
lookup_commit_reference_by_name(branch_name);
- if (commit) {
- oidcpy(&options.orig_head, &commit->object.oid);
- options.head_name = NULL;
- } else {
+ if (!commit)
die(_("fatal: no such branch/commit '%s'"),
branch_name);
- }
+
+ oidcpy(&options.orig_head, &commit->object.oid);
+ options.head_name = NULL;
}
} else if (argc == 0) {
/* Do not need to switch branches, we are already on it. */
I.e. handle the "die" case right away & skip the indenting of the
non-assert code.
(Also grepping around we could really use a fatal non-gentle version of
lookup_commit_reference_by_name(), but that's another more general
cleanup...)
> } 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 2f41b06e028..310cd0c736c 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 &&
This whole series looks good to me, left some comments on other
patches. Consider the above suggested squash highly optional :)
|
User |
builtin/rebase.c
Outdated
| @@ -1299,7 +1299,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) | |||
| int ret, flags, total_argc, in_progress = 0; | |||
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Phillip,
On Wed, 8 Sep 2021, Phillip Wood via GitGitGadget wrote:
> 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).
In some instances, we use a `buf` variable to construct a string that is
then assigned to a `const char *` and is used elsewhere throughout the
function. If this was the case in the code you modified, that would be a
problem.
I did verify manually that this is not the case, though, so this patch is
good to go.
Thanks,
Dscho
|
User |
phillipwood
force-pushed
the
wip/rebase-handle-tags
branch
2 times, most recently
from
51e6025
to
f5adb55
Compare
Commit 97b88dd ("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 4a7aacc ("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>
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>
This provides better diagnostics if a test fails Signed-off-by: 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>
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>
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>
9512177 ("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>
phillipwood
force-pushed
the
wip/rebase-handle-tags
branch
from
f5adb55
to
951de6b
Compare
|
/submit |
|
Submitted as pull.1033.v2.git.1631546362.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
builtin/rebase.c
Outdated
| @@ -1299,7 +1299,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) | |||
| int ret, flags, total_argc, in_progress = 0; | |||
There was a problem hiding this comment.
On the Git mailing list, René Scharfe wrote (reply to this):
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é
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
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.
There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
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.
>
There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
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
|
User |
| @@ -762,17 +762,6 @@ static int finish_rebase(struct rebase_options *opts) | |||
| return ret; | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"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.
There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
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.
>
There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
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.
>>
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
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).
|
This branch is now known as |
|
This patch series was integrated into seen via git@89b09b5. |
|
There was a status update in the "New Topics" section about the branch "git rebase <upstream> <tag>" failed when aborted in the middle, as it mistakenly tried to write the tag object instead of peeling it to HEAD. Expecting a reroll. |
|
Found multiple candidates in gitster/git: Using the first one. |
|
This patch series was integrated into seen via git@04ab3f0. |
|
Found multiple candidates in gitster/git: Using the first one. |
1 similar comment
|
Found multiple candidates in gitster/git: Using the first one. |
|
There was a status update in the "Cooking" section about the branch "git rebase <upstream> <tag>" failed when aborted in the middle, as it mistakenly tried to write the tag object instead of peeling it to HEAD. Expecting a reroll. |
phillipwood
force-pushed
the
wip/rebase-handle-tags
branch
from
951de6b
to
55a6250
Compare
|
/submit |
|
Submitted as pull.1033.v3.git.1632219848.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via git@1b036d0. |
|
This patch series was integrated into seen via git@4938d68. |
|
There was a status update in the "Cooking" section about the branch "git rebase <upstream> <tag>" failed when aborted in the middle, as it mistakenly tried to write the tag object instead of peeling it to HEAD. Will merge to 'next'? |
|
On the Git mailing list, Elijah Newren wrote (reply to this): |
|
There was a status update in the "Cooking" section about the branch "git rebase <upstream> <tag>" failed when aborted in the middle, as it mistakenly tried to write the tag object instead of peeling it to HEAD. Will merge to 'next'. |
|
This patch series was integrated into seen via git@dd3416c. |
|
This patch series was integrated into next via git@980add2. |
|
This patch series was integrated into seen via git@40af0a3. |
|
This patch series was integrated into seen via git@7bccd4c. |
|
There was a status update in the "Cooking" section about the branch "git rebase <upstream> <tag>" failed when aborted in the middle, as it mistakenly tried to write the tag object instead of peeling it to HEAD. Will merge to 'master'. |
|
This patch series was integrated into seen via git@22ddd0e. |
|
This patch series was integrated into seen via git@7bccd4c. |
|
This patch series was integrated into seen via git@22ddd0e. |
|
This patch series was integrated into seen via git@a6f8664. |
|
There was a status update in the "Cooking" section about the branch "git rebase <upstream> <tag>" failed when aborted in the middle, as it mistakenly tried to write the tag object instead of peeling it to HEAD. Will merge to 'master'. |
|
This patch series was integrated into seen via git@7cebe73. |
|
This patch series was integrated into next via git@7cebe73. |
|
This patch series was integrated into master via git@7cebe73. |
|
Closed via 7cebe73. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Thanks for the comments on V2. Here are the changes from that version:
Cover letter for v2:
Thanks to Ævar and Johannes for their comments.
Cover letter for V1:
Aborting a rebase stated with
git rebase <upstream> <tag-object>should checkout the commit pointed to by . Instead it givesThe fix for that is in the last patch, the rest of the patches are cleanups to t3407 and builtin/rebase.c
cc: Ævar Arnfjörð Bjarmason avarab@gmail.com
cc: Phillip Wood phillip.wood123@gmail.com
cc: Johannes Schindelin Johannes.Schindelin@gmx.de
cc: René Scharfe l.s.r@web.de
cc: Elijah Newren newren@gmail.com
cc: Sergey Organov sorganov@gmail.com