cherry-pick: better error message when the parameter is a non-commit
PATCH 1/1 cherry-pick: better error message when the parameter is a non-commit
When copy&paste goes wrong, and the user e.g. tries to cherry-pick a blob, the error message used to be:
fatal: BUG: expected exactly one commit from walk
Instead, now it is:
fatal: Can't cherry-pick a blob
--- sequencer.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
sequencer.c +8 -1
@@ -1082,8 +1082,15 @@ int sequencer_pick_revisions(struct replay_opts *opts)if (prepare_revision_walk (opts->revs))die (_ ("revision walk setup failed" )); cmit =get_revision (opts->revs); -if (!cmit ||get_revision (opts->revs)) +if (!cmit ||get_revision (opts->revs)) { +unsigned char sha1[20 ]; +if (!get_sha1 (opts->revs->cmdline.rev->name, sha1)) { +enum object_type type =sha1_object_info (sha1,NULL ); +if (type >0 && type !=OBJ_COMMIT ) +die (_ ("Can't cherry-pick a %s" ),typename (type)); + }die ("BUG: expected exactly one commit from walk" ); + }return single_pick (cmit, opts); }
Signature · 1 line
1.8.1.4
Hi,
Quoted 5 lines · Miklos Vajna
When copy&paste goes wrong, and the user e.g. tries to cherry-pick a blob, the error message used to be:
fatal: BUG: expected exactly one commit from walk
Quoted 8 lines
Instead, now it is:
fatal: Can't cherry-pick a blob
Signed-off-by Miklos Vajna <[email protected]>--- sequencer.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
Ping, any comment on this patch?
Thanks,
Miklos
Quoted 8 lines · Miklos Vajna
Quoted 7 lines · Miklos Vajna
When copy&paste goes wrong, and the user e.g. tries to cherry-pick a blob, the error message used to be:
fatal: BUG: expected exactly one commit from walk
Instead, now it is:
Quoted 8 lines
Quoted 6 lines
fatal: Can't cherry-pick a blob
Signed-off-by Miklos Vajna <[email protected]>--- sequencer.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)Ping, any comment on this patch?
Nothing in particular from me.
Quoted 2 lines · Miklos Vajna
When copy&paste goes wrong, and the user e.g. tries to cherry-pick a blob, the error message used to be:
It is the other way around. When the user tries to cherry-pick a non-commit we say a correct but nonspecific "expected one commit", and it does not matter how the user threw a non-commit at us. One possibility could be copy&paste going wrong.
Quoted 5 lines
fatal: BUG: expected exactly one commit from walk
Instead, now it is:
fatal: Can't cherry-pick a blob
I wonder what we would do when "git cherry-pick master: next" is given. That is not "single commit input" case and not covered by this patch, but perhaps something we may want to diagnose?
In other words, perhaps we would want to inspect pending objects before running prepare_revision_walk and make sure everybody is commit-ish or something?
Quoted 26 lines
Signed-off-by Miklos Vajna <[email protected]>--- sequencer.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)sequencer.c +8
-1@@ -1082,8 +1082,15 @@ int sequencer_pick_revisions(struct replay_opts *opts)if (prepare_revision_walk (opts->revs))die (_ ("revision walk setup failed" )); cmit =get_revision (opts->revs); -if (!cmit ||get_revision (opts->revs)) +if (!cmit ||get_revision (opts->revs)) { +unsigned char sha1[20 ]; +if (!get_sha1 (opts->revs->cmdline.rev->name, sha1)) { +enum object_type type =sha1_object_info (sha1,NULL ); +if (type >0 && type !=OBJ_COMMIT ) +die (_ ("Can't cherry-pick a %s" ),typename (type)); + }die ("BUG: expected exactly one commit from walk" ); + }return single_pick (cmit, opts); }
PATCH v2 1/1 cherry-pick: make sure all input objects are commits
When a single argument was a non-commit, the error message used to be:
fatal: BUG: expected exactly one commit from walk
For multiple arguments, when none of the arguments was a commit, the error was:
fatal: empty commit set passed
Finally, when some of the arguments were non-commits, we ignored those arguments. Instead, now make sure all arguments are commits, and for the first non-commit, error out with:
fatal: <name>: Can't cherry-pick a <type>
---
Quoted 3 lines · Junio C Hamano
In other words, perhaps we would want to inspect pending objects before running
prepare_revision_walkand make sure everybody is commit-ish or something?
Sure, that makes sense to me.
sequencer.c | 13 +++++++++++++ t/t3508-cherry-pick-many-commits.sh | 6 ++++++ 2 files changed, 19 insertions(+)
sequencer.c +13 -0
@@ -1047,6 +1047,7 @@ int sequencer_pick_revisions(struct replay_opts *opts) {struct commit_list *todo_list =NULL ;unsigned char sha1[20 ]; +int i;if (opts->subcommand ==REPLAY_NONE )assert (opts->revs);
@@ -1067,6 +1068,18 @@ int sequencer_pick_revisions(struct replay_opts *opts)if (opts->subcommand ==REPLAY_CONTINUE )return sequencer_continue (opts); +for (i =0 ; i < opts->revs->pending.nr; i++) { +unsigned char sha1[20 ]; +const char *name = opts->revs->pending.objects[i].name; + +if (!get_sha1 (name, sha1)) { +enum object_type type =sha1_object_info (sha1,NULL ); + +if (type >0 && type !=OBJ_COMMIT ) +die (_ ("%s: can't cherry-pick a %s" ), name,typename (type)); + } + } +/* * If we were called as "git cherry-pick <commit>", just * cherry-pick/revert it, set CHERRY_PICK_HEAD /
t/t3508-cherry-pick-many-commits.sh +6 -0
@@ -55,6 +55,12 @@ one two" ' +test_expect_success 'cherry-pick three one two: fails' ' + git checkout -f master && + git reset --hard first && + test_must_fail git cherry-pick three one two: +' + test_expect_success 'output to keep user entertained during multi-pick' ' cat <<-\EOF >expected && [master OBJID] second
Signature · 1 line
1.8.1.4
Re: [PATCH v2] cherry-pick: make sure all input objects are commits
Quoted 6 lines · Miklos Vajna
When a single argument was a non-commit, the error message used to be:
fatal: BUG: expected exactly one commit from walk
For multiple arguments, when none of the arguments was a commit, the error was:
Quoted 7 lines
fatal: empty commit set passed
Finally, when some of the arguments were non-commits, we ignored those arguments. Instead, now make sure all arguments are commits, and for the first non-commit, error out with:
fatal: <name>: Can't cherry-pick a <type>
Thanks. This is worth fixing.
Quoted 8 lines
sequencer.c +0
-0@@ -1067,6 +1068,18 @@ int sequencer_pick_revisions(struct replay_opts *opts)if (opts->subcommand ==REPLAY_CONTINUE )return sequencer_continue (opts);
sequencer.c int sequencer_pick_revisions(struct replay_opts *opts) · line 1080
Quoted 10 lines
sequencer.c +10
-0+for (i =0 ; i < opts->revs->pending.nr; i++) { +unsigned char sha1[20 ]; +const char *name = opts->revs->pending.objects[i].name; + +if (!get_sha1 (name, sha1)) { +enum object_type type =sha1_object_info (sha1,NULL ); + +if (type >0 && type !=OBJ_COMMIT ) +die (_ ("%s: can't cherry-pick a %s" ), name,typename (type)); + }
else? What happens if get_sha1() fails?
t/t3508-cherry-pick-many-commits.sh one · line 62
Quoted 13 lines
t/t3508-cherry-pick-many-commits.sh +5
-0@@ -55,6 +55,12 @@ one two" ' +test_expect_success 'cherry-pick three one two: fails' ' + git checkout -f master && + git reset --hard first && + test_must_fail git cherry-pick three one two: +'
So you're testing just the third case (where commit objects are mixed with non-commit objects), which is arguably a bug. Okay.
Re: [PATCH v2] cherry-pick: make sure all input objects are commits
Quoted 4 lines · Ramkumar Ramachandra
Quoted 4 lines
sequencer.c +4
-0+for (i =0 ; i < opts->revs->pending.nr; i++) { +unsigned char sha1[20 ]; +const char *name = opts->revs->pending.objects[i].name; +
Quoted 8 lines
sequencer.c int sequencer_pick_revisions(struct replay_opts *opts) · line 1080
Quoted 6 lines
sequencer.c +6
-0+if (!get_sha1 (name, sha1)) { +enum object_type type =sha1_object_info (sha1,NULL ); + +if (type >0 && type !=OBJ_COMMIT ) +die (_ ("%s: can't cherry-pick a %s" ), name,typename (type)); + }else? What happens if
get_sha1()fails?
I guess that is a should-not-happen category. parse_args() calls setup_revisions(), and that will already die() if the argument is not a valid object at all.
Quoted 8 lines
Quoted 8 lines
t/t3508-cherry-pick-many-commits.sh +0
-0@@ -55,6 +55,12 @@ one two" '
Quoted 8 lines
t/t3508-cherry-pick-many-commits.sh one · line 62
Quoted 5 lines
t/t3508-cherry-pick-many-commits.sh +5
-0+test_expect_success 'cherry-pick three one two: fails' ' + git checkout -f master && + git reset --hard first && + test_must_fail git cherry-pick three one two: +'So you're testing just the third case (where commit objects are mixed with non-commit objects), which is arguably a bug. Okay.
Yes. If you would want, I could of course add test cases for two other cases when we already errored out and now the error message is just changed, but I don't think duplicating the error message strings from the code to the testsuite is really wanted. :-)
Re: [PATCH v2] cherry-pick: make sure all input objects are commits
Quoted 3 lines · Miklos Vajna
I guess that is a should-not-happen category.
parse_args()callssetup_revisions(), and that will alreadydie()if the argument is not a valid object at all.
Then why do you have an if() guarding the code? In my opinion, you should have an else-clause that die()s with an appropriate message.
Quoted 4 lines
Yes. If you would want, I could of course add test cases for two other cases when we already errored out and now the error message is just changed, but I don't think duplicating the error message strings from the code to the testsuite is really wanted. :-)
Nope, I'd never suggest that: this is fine. What I meant is: you should clarify that you're fixing a bug and adding a test to guard it, in the commit message.
PATCH v3 1/1 cherry-pick: make sure all input objects are commits
When a single argument was a non-commit, the error message used to be:
fatal: BUG: expected exactly one commit from walk
For multiple arguments, when none of the arguments was a commit, the error was:
fatal: empty commit set passed
Finally, when some of the arguments were non-commits, we ignored those arguments. Fix this bug and make sure all arguments are commits, and for the first non-commit, error out with:
fatal: <name>: Can't cherry-pick a <type>
---
Quoted 2 lines · Ramkumar Ramachandra
Then why do you have an
if()guarding the code? In my opinion, you should have an else-clause that die()s with an appropriate message.
And you were right -- I actually forgot about --stdin, where the else-clause is hit. Added that for now, excluding --stdin.
Quoted 3 lines
Nope, I'd never suggest that: this is fine. What I meant is: you should clarify that you're fixing a bug and adding a test to guard it, in the commit message.
Done.
sequencer.c | 18 ++++++++++++++++++ t/t3508-cherry-pick-many-commits.sh | 6 ++++++ 2 files changed, 24 insertions(+)
sequencer.c +18 -0
@@ -1047,6 +1047,7 @@ int sequencer_pick_revisions(struct replay_opts *opts) {struct commit_list *todo_list =NULL ;unsigned char sha1[20 ]; +int i;if (opts->subcommand ==REPLAY_NONE )assert (opts->revs);
@@ -1067,6 +1068,23 @@ int sequencer_pick_revisions(struct replay_opts *opts)if (opts->subcommand ==REPLAY_CONTINUE )return sequencer_continue (opts); +for (i =0 ; i < opts->revs->pending.nr; i++) { +unsigned char sha1[20 ]; +const char *name = opts->revs->pending.objects[i].name; + +/* This happens when using --stdin. */ +if (!strlen (name)) +continue ; + +if (!get_sha1 (name, sha1)) { +enum object_type type =sha1_object_info (sha1,NULL ); + +if (type >0 && type !=OBJ_COMMIT ) +die (_ ("%s: can't cherry-pick a %s" ), name,typename (type)); + }else +die (_ ("%s: bad revision" ), name); + } +/* * If we were called as "git cherry-pick <commit>", just * cherry-pick/revert it, set CHERRY_PICK_HEAD /
t/t3508-cherry-pick-many-commits.sh +6 -0
@@ -55,6 +55,12 @@ one two" ' +test_expect_success 'cherry-pick three one two: fails' ' + git checkout -f master && + git reset --hard first && + test_must_fail git cherry-pick three one two: +' + test_expect_success 'output to keep user entertained during multi-pick' ' cat <<-\EOF >expected && [master OBJID] second
Signature · 1 line
1.8.1.4
Re: [PATCH v3] cherry-pick: make sure all input objects are commits
Quoted 1 line · Miklos Vajna
Signed-off-by Miklos Vajna <[email protected]>
This one looks good. FWIW,
Re: [PATCH v3] cherry-pick: make sure all input objects are commits
In reply to Miklos VajnaQuoted 6 lines · Miklos Vajna
Fix this bug and make sure all arguments are commits, and for the first non-commit, error out with:
fatal: <name>: Can't cherry-pick a <type>
Review int sequencer_pick_revisions(struct replay_opts *opts) · line 1077
Quoted 10 lines
(quoted diff) +7
-0@@ -1067,6 +1068,23 @@ int sequencer_pick_revisions(struct replay_opts *opts) if (opts->subcommand == REPLAY_CONTINUE) return sequencer_continue(opts); + for (i = 0; i < opts->revs->pending.nr; i++) { + unsigned char sha1[20]; + const char *name = opts->revs->pending.objects[i].name; + + /* This happens when using --stdin. */ + if (!strlen(name)) + continue;
This is undefined behavior; the pending.objects[i].name has been freed already. Luckily valgrind points you right at it:
==9178== Invalid read of size 1
==9178== at 0x4CEFB4: sequencer_pick_revisions (sequencer.c:1077) ==9178== by 0x45E7F2: cmd_cherry_pick (revert.c:236) ==9178== by 0x40523C: handle_internal_command (git.c:292) ==9178== by 0x405467: main (git.c:500) ==9178== Address 0x5bedbd0 is 0 bytes inside a block of size 1,001 free'd ==9178== at 0x4C2ACDA: free (vg_replace_malloc.c:468) ==9178== by 0x4D96C7: strbuf_release (strbuf.c:40) ==9178== by 0x4C9AAE: setup_revisions (revision.c:1285) ==9178== by 0x45E6FA: parse_args (revert.c:203) ==9178== by 0x45E7EA: cmd_cherry_pick (revert.c:235) ==9178== by 0x40523C: handle_internal_command (git.c:292) ==9178== by 0x405467: main (git.c:500)
Quoted 1 line
From a cursory glance it looks like it's actually an existing bug in
read_revisions_from_stdin or handle_revision_arg, depending on which way you look at it. read_revisions_from_stdin passes its temporary buffer down to handle_revision_arg:
struct strbuf sb;
[...]
strbuf_init(&sb, 1000);
while (strbuf_getwholeline(&sb, stdin, '\n') != EOF) {
[...]
if (handle_revision_arg(sb.buf, revs, 0, REVARG_CANNOT_BE_FILENAME))
die("bad revision '%s'", sb.buf);
}But handle_revision_arg ends up just stuffing that parameter into the revision-walker options via some helpers:
add_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags); add_pending_object_with_mode(revs, object, arg, oc.mode);
This seems to have been lurking since 281eee4 (revision: keep track of the end-user input from the command line, 2011-08-25).
Junio, at which level should we fix it? We could of course have read_revisions_from_stdin make a copy of the buffers it passes down, but perhaps it would be less surprising to instead have handle_revision_arg make sure it makes a copy of everything it "keeps"?
The easy fix of course is just this:
revision.c +2 -1
@@ -1277,7 +1277,8 @@ static void read_revisions_from_stdin(struct rev_info *revs, }die ("options not supported in --stdin mode" ); } -if (handle_revision_arg (sb.buf, revs,0 ,REVARG_CANNOT_BE_FILENAME )) +if (handle_revision_arg (xstrdup (sb.buf), revs,0 , +REVARG_CANNOT_BE_FILENAME ))die ("bad revision '%s'" , sb.buf); }if (seen_dashdash)
Signature · 2 lines
Thomas Rast
trast@{inf,student}.ethz.chRe: [PATCH v3] cherry-pick: make sure all input objects are commits
Quoted 20 lines · Thomas Rast
From a cursory glance it looks like it's actually an existing bug in
read_revisions_from_stdinorhandle_revision_arg, depending on which way you look at it.read_revisions_from_stdinpasses its temporary buffer down tohandle_revision_arg:struct strbuf sb; [...] strbuf_init(&sb, 1000); while (strbuf_getwholeline(&sb, stdin, '\n') != EOF) { [...] if (handle_revision_arg(sb.buf, revs, 0, REVARG_CANNOT_BE_FILENAME)) die("bad revision '%s'", sb.buf); }But
handle_revision_argends up just stuffing that parameter into the revision-walker options via some helpers:add_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags); add_pending_object_with_mode(revs, object, arg, oc.mode);
Quoted 7 lines
This seems to have been lurking since 281eee4 (revision: keep track of the end-user input from the command line, 2011-08-25).
Junio, at which level should we fix it? We could of course have
read_revisions_from_stdinmake a copy of the buffers it passes down, but perhaps it would be less surprising to instead havehandle_revision_argmake sure it makes a copy of everything it "keeps"?
Looking at it again, it seems that the issue is much older than the introduction of cmdline interface.
Everything we throw at add_pending_object() is assumed to be stable, because historically they were argv[] strings, and --stdin is what breaks that assumption. Making copies unconditionally at the lower layer only because some minority callers give it unstable strings does not sound like a good trade-off.
So I changed my mind. Your "easy fix" looks to me the right thing to do.
The paths given to handle_refs() may also have to be copied before saved, depending on how ref iteration is implemented, details of which may change as Michael seems to be updating the area again. I think we let the callback peek ref_entry->name[] which is stable, so I suspect we are OK.
Quoted 16 lines
The easy fix of course is just this:
revision.c +2
-1@@ -1277,7 +1277,8 @@ static void read_revisions_from_stdin(struct rev_info *revs, }die ("options not supported in --stdin mode" ); } -if (handle_revision_arg (sb.buf, revs,0 ,REVARG_CANNOT_BE_FILENAME )) +if (handle_revision_arg (xstrdup (sb.buf), revs,0 , +REVARG_CANNOT_BE_FILENAME ))die ("bad revision '%s'" , sb.buf); }if (seen_dashdash)
Re: [PATCH v3] cherry-pick: make sure all input objects are commits
Quoted 5 lines · Junio C Hamano
The paths given to
handle_refs()may also have to be copied before saved, depending on how ref iteration is implemented, details of which may change as Michael seems to be updating the area again. I think we let the callback peek ref_entry->name[] which is stable, so I suspect we are OK.
ref_entry->name is stable as long as invalidate_ref_cache() is not called, and I am not even thinking of changing that (partly because I don't have the energy to audit and adjust all of the callers).
Michael
Signature · 3 lines
Michael Haggerty [email protected] http://softwareswirl.blogspot.com/
Re: [PATCH v3] cherry-pick: make sure all input objects are commits
In reply to Thomas RastQuoted 20 lines · Thomas Rast
From a cursory glance it looks like it's actually an existing bug in
read_revisions_from_stdinorhandle_revision_arg, depending on which way you look at it.read_revisions_from_stdinpasses its temporary buffer down tohandle_revision_arg:struct strbuf sb; [...] strbuf_init(&sb, 1000); while (strbuf_getwholeline(&sb, stdin, '\n') != EOF) { [...] if (handle_revision_arg(sb.buf, revs, 0, REVARG_CANNOT_BE_FILENAME)) die("bad revision '%s'", sb.buf); }But
handle_revision_argends up just stuffing that parameter into the revision-walker options via some helpers:add_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags); add_pending_object_with_mode(revs, object, arg, oc.mode);
Quoted 8 lines
This seems to have been lurking since 281eee4 (revision: keep track of the end-user input from the command line, 2011-08-25).
Junio, at which level should we fix it? We could of course have
read_revisions_from_stdinmake a copy of the buffers it passes down, but perhaps it would be less surprising to instead havehandle_revision_argmake sure it makes a copy of everything it "keeps"?
That sounds like the right way to go to me.
Quoted 16 lines
The easy fix of course is just this:
revision.c +2
-1@@ -1277,7 +1277,8 @@ static void read_revisions_from_stdin(struct rev_info *revs, }die ("options not supported in --stdin mode" ); } -if (handle_revision_arg (sb.buf, revs,0 ,REVARG_CANNOT_BE_FILENAME )) +if (handle_revision_arg (xstrdup (sb.buf), revs,0 , +REVARG_CANNOT_BE_FILENAME ))die ("bad revision '%s'" , sb.buf); }if (seen_dashdash)
Re: [PATCH v3] cherry-pick: make sure all input objects are commits
In reply to Miklos VajnaQuoted 8 lines · Miklos Vajna
When a single argument was a non-commit, the error message used to be:
fatal: BUG: expected exactly one commit from walk
For multiple arguments, when none of the arguments was a commit, the error was:
fatal: empty commit set passed
Quoted 7 lines
Finally, when some of the arguments were non-commits, we ignored those arguments. Fix this bug and make sure all arguments are commits, and for the first non-commit, error out with:
fatal: <name>: Can't cherry-pick a <type>
Signed-off-by Miklos Vajna <[email protected]>
This turns out to be an irritatingly stupid change. While I am rebuilding a privately tagged tip of 'maint', I am seeing:
fatal: v1.8.2.3: Can't cherry-pick a tag
You would want to reject non committish, not non commit.
Quoted 57 lines
sequencer.c | 18 ++++++++++++++++++ t/t3508-cherry-pick-many-commits.sh | 6 ++++++ 2 files changed, 24 insertions(+)sequencer.c +18
-0@@ -1047,6 +1047,7 @@ int sequencer_pick_revisions(struct replay_opts *opts) {struct commit_list *todo_list =NULL ;unsigned char sha1[20 ]; +int i;if (opts->subcommand ==REPLAY_NONE )assert (opts->revs);@@ -1067,6 +1068,23 @@ int sequencer_pick_revisions(struct replay_opts *opts)if (opts->subcommand ==REPLAY_CONTINUE )return sequencer_continue (opts); +for (i =0 ; i < opts->revs->pending.nr; i++) { +unsigned char sha1[20 ]; +const char *name = opts->revs->pending.objects[i].name; + +/* This happens when using --stdin. */ +if (!strlen (name)) +continue ; + +if (!get_sha1 (name, sha1)) { +enum object_type type =sha1_object_info (sha1,NULL ); + +if (type >0 && type !=OBJ_COMMIT ) +die (_ ("%s: can't cherry-pick a %s" ), name,typename (type)); + }else +die (_ ("%s: bad revision" ), name); + } +/* * If we were called as "git cherry-pick <commit>", just * cherry-pick/revert it, set CHERRY_PICK_HEAD / t/t3508-cherry-pick-many-commits.sh +6
-0@@ -55,6 +55,12 @@ one two" ' +test_expect_success 'cherry-pick three one two: fails' ' + git checkout -f master && + git reset --hard first && + test_must_fail git cherry-pick three one two: +' + test_expect_success 'output to keep user entertained during multi-pick' ' cat <<-\EOF >expected && [master OBJID] second
Re: [PATCH v3] cherry-pick: make sure all input objects are commits
Quoted 16 lines · Junio C Hamano
Quoted 14 lines · Miklos Vajna
When a single argument was a non-commit, the error message used to be:
fatal: BUG: expected exactly one commit from walk
For multiple arguments, when none of the arguments was a commit, the error was:
fatal: empty commit set passed
Finally, when some of the arguments were non-commits, we ignored those arguments. Fix this bug and make sure all arguments are commits, and for the first non-commit, error out with:
fatal: <name>: Can't cherry-pick a <type>
Quoted 8 lines
Quoted 1 line
Signed-off-by Miklos Vajna <[email protected]>This turns out to be an irritatingly stupid change. While I am rebuilding a privately tagged tip of 'maint', I am seeing:
fatal: v1.8.2.3: Can't cherry-pick a tagYou would want to reject non committish, not non commit.
I'd apply this before -rc2. I _think_ it is also OK to just let lookup_commit_reference_gently() barf with its standard message
error: Object %s is a %s, not a commit
without an extra sha1_object_info() call in the error codepath, but I did not bother, as this is meant to be an emergency fix.
-- >8 --
Subject: cherry-pick: picking a tag that resolves to a commit is OK
Earlier, 21246dbb9e0a (cherry-pick: make sure all input objects are commits, 2013-04-11) tried to catch an unlikely "git cherry-pick $blob" as an error, but broke a more important use case to cherry-pick a tag that points at a commit.
--- sequencer.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
sequencer.c +3 -3
@@ -1077,10 +1077,10 @@ int sequencer_pick_revisions(struct replay_opts *opts)continue ;if (!get_sha1 (name, sha1)) { -enum object_type type =sha1_object_info (sha1,NULL ); - -if (type >0 && type !=OBJ_COMMIT ) +if (!lookup_commit_reference_gently (sha1,1 )) { +enum object_type type =sha1_object_info (sha1,NULL );die (_ ("%s: can't cherry-pick a %s" ), name,typename (type)); + } }else die (_ ("%s: bad revision" ), name); }
Re: [PATCH v3] cherry-pick: make sure all input objects are commits
Quoted 7 lines · Junio C Hamano
I'd apply this before -rc2. I _think_ it is also OK to just let
lookup_commit_reference_gently()barf with its standard messageerror: Object %s is a %s, not a commit
without an extra
sha1_object_info()call in the error codepath, but I did not bother, as this is meant to be an emergency fix.
Yes, that makes a lot of sense. I myself never cherry-pick tags, but I understand that is part of some workflow.