{"thread":{"id":"33363","subject":"[PATCH] cherry-pick: better error message when the parameter is a non-commit","startedAt":"2013-04-03T09:27:04Z","lastAt":"2013-05-10T07:07:13Z","messageCount":17,"participants":["Miklos Vajna","Junio C Hamano","Ramkumar Ramachandra","Thomas Rast","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"213028","messageId":"20130403092704.GC21520@suse.cz","threadId":"33363","inReplyTo":null,"subject":"[PATCH] cherry-pick: better error message when the parameter is a non-commit","fromName":"Miklos Vajna","fromEmail":"vmiklos@suse.cz","sentAt":"2013-04-03T09:27:04Z","receivedAt":"2013-04-03T09:27:04Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"When copy&paste goes wrong, and the user e.g. tries to cherry-pick a\nblob, the error message used to be:\n\n\tfatal: BUG: expected exactly one commit from walk\n\nInstead, now it is:\n\n\tfatal: Can't cherry-pick a blob\n\nSigned-off-by: Miklos Vajna \n---\n sequencer.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex baa0310..0ac00d4 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1082,8 +1082,15 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \t\tif (prepare_revision_walk(opts->revs))\n \t\t\tdie(_(\"revision walk setup failed\"));\n \t\tcmit = get_revision(opts->revs);\n-\t\tif (!cmit || get_revision(opts->revs))\n+\t\tif (!cmit || get_revision(opts->revs)) {\n+\t\t\tunsigned char sha1[20];\n+\t\t\tif (!get_sha1(opts->revs->cmdline.rev->name, sha1)) {\n+\t\t\t\tenum object_type type = sha1_object_info(sha1, NULL);\n+\t\t\t\tif (type > 0 && type != OBJ_COMMIT)\n+\t\t\t\t\tdie(_(\"Can't cherry-pick a %s\"), typename(type));\n+\t\t\t}\n \t\t\tdie(\"BUG: expected exactly one commit from walk\");\n+\t\t}\n \t\treturn single_pick(cmit, opts);\n \t}\n \n-- \n1.8.1.4\n"},{"id":"213515","messageId":"20130408122750.GB5132@suse.cz","threadId":"33363","inReplyTo":"20130403092704.GC21520@suse.cz","subject":"Re: [PATCH] cherry-pick: better error message when the parameter is a non-commit","fromName":"Miklos Vajna","fromEmail":"vmiklos@suse.cz","sentAt":"2013-04-08T12:27:50Z","receivedAt":"2013-04-08T12:27:50Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"Hi,\n\nOn Wed, Apr 03, 2013 at 11:27:04AM +0200, Miklos Vajna wrote:\n> When copy&paste goes wrong, and the user e.g. tries to cherry-pick a\n> blob, the error message used to be:\n> \n> \tfatal: BUG: expected exactly one commit from walk\n> \n> Instead, now it is:\n> \n> \tfatal: Can't cherry-pick a blob\n> \n> Signed-off-by: Miklos Vajna \n> ---\n> sequencer.c | 9 ++++++++-\n> 1 file changed, 8 insertions(+), 1 deletion(-)\n\nPing, any comment on this patch?\n\nThanks,\n\nMiklos\n"},{"id":"213530","messageId":"7v7gkdynqx.fsf@alter.siamese.dyndns.org","threadId":"33363","inReplyTo":"20130408122750.GB5132@suse.cz","subject":"Re: [PATCH] cherry-pick: better error message when the parameter is a non-commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-08T16:45:58Z","receivedAt":"2013-04-08T16:45:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miklos Vajna writes:\n\n> On Wed, Apr 03, 2013 at 11:27:04AM +0200, Miklos Vajna wrote:\n>> When copy&paste goes wrong, and the user e.g. tries to cherry-pick a\n>> blob, the error message used to be:\n>> \n>> \tfatal: BUG: expected exactly one commit from walk\n>> \n>> Instead, now it is:\n>> \n>> \tfatal: Can't cherry-pick a blob\n>> \n>> Signed-off-by: Miklos Vajna \n>> ---\n>> sequencer.c | 9 ++++++++-\n>> 1 file changed, 8 insertions(+), 1 deletion(-)\n>\n> Ping, any comment on this patch?\n\nNothing in particular from me.\n"},{"id":"213531","messageId":"7v38v1yn8o.fsf@alter.siamese.dyndns.org","threadId":"33363","inReplyTo":"20130403092704.GC21520@suse.cz","subject":"Re: [PATCH] cherry-pick: better error message when the parameter is a non-commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-08T16:56:55Z","receivedAt":"2013-04-08T16:56:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miklos Vajna writes:\n\n> When copy&paste goes wrong, and the user e.g. tries to cherry-pick a\n> blob, the error message used to be:\n\nIt is the other way around. When the user tries to cherry-pick a\nnon-commit we say a correct but nonspecific \"expected one commit\",\nand it does not matter how the user threw a non-commit at us. One\npossibility could be copy&paste going wrong.\n\n> \tfatal: BUG: expected exactly one commit from walk\n>\n> Instead, now it is:\n>\n> \tfatal: Can't cherry-pick a blob\n\nI wonder what we would do when \"git cherry-pick master: next\"\nis given. That is not \"single commit input\" case and not covered by\nthis patch, but perhaps something we may want to diagnose?\n\nIn other words, perhaps we would want to inspect pending objects\nbefore running prepare_revision_walk and make sure everybody is\ncommit-ish or something?\n\n>\n> Signed-off-by: Miklos Vajna \n> ---\n> sequencer.c | 9 ++++++++-\n> 1 file changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index baa0310..0ac00d4 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -1082,8 +1082,15 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n> \t\tif (prepare_revision_walk(opts->revs))\n> \t\t\tdie(_(\"revision walk setup failed\"));\n> \t\tcmit = get_revision(opts->revs);\n> -\t\tif (!cmit || get_revision(opts->revs))\n> +\t\tif (!cmit || get_revision(opts->revs)) {\n> +\t\t\tunsigned char sha1[20];\n> +\t\t\tif (!get_sha1(opts->revs->cmdline.rev->name, sha1)) {\n> +\t\t\t\tenum object_type type = sha1_object_info(sha1, NULL);\n> +\t\t\t\tif (type > 0 && type != OBJ_COMMIT)\n> +\t\t\t\t\tdie(_(\"Can't cherry-pick a %s\"), typename(type));\n> +\t\t\t}\n> \t\t\tdie(\"BUG: expected exactly one commit from walk\");\n> +\t\t}\n> \t\treturn single_pick(cmit, opts);\n> \t}\n"},{"id":"213915","messageId":"20130411092638.GA12770@suse.cz","threadId":"33363","inReplyTo":"7v38v1yn8o.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] cherry-pick: make sure all input objects are commits","fromName":"Miklos Vajna","fromEmail":"vmiklos@suse.cz","sentAt":"2013-04-11T09:26:38Z","receivedAt":"2013-04-11T09:26:38Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"When a single argument was a non-commit, the error message used to be:\n\n\tfatal: BUG: expected exactly one commit from walk\n\nFor multiple arguments, when none of the arguments was a commit, the error was:\n\n\tfatal: empty commit set passed\n\nFinally, when some of the arguments were non-commits, we ignored those\narguments. Instead, now make sure all arguments are commits, and for\nthe first non-commit, error out with:\n\n\tfatal: : Can't cherry-pick a \n\nSigned-off-by: Miklos Vajna \n---\n\nOn Mon, Apr 08, 2013 at 09:56:55AM -0700, Junio C Hamano wrote:\n> In other words, perhaps we would want to inspect pending objects\n> before running prepare_revision_walk and make sure everybody is\n> commit-ish or something?\n\nSure, that makes sense to me.\n\n sequencer.c | 13 +++++++++++++\n t/t3508-cherry-pick-many-commits.sh | 6 ++++++\n 2 files changed, 19 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex baa0310..eb25101 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1047,6 +1047,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n {\n \tstruct commit_list *todo_list = NULL;\n \tunsigned char sha1[20];\n+\tint i;\n \n \tif (opts->subcommand == REPLAY_NONE)\n \t\tassert(opts->revs);\n@@ -1067,6 +1068,18 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \tif (opts->subcommand == REPLAY_CONTINUE)\n \t\treturn sequencer_continue(opts);\n \n+\tfor (i = 0; i < opts->revs->pending.nr; i++) {\n+\t\tunsigned char sha1[20];\n+\t\tconst char *name = opts->revs->pending.objects[i].name;\n+\n+\t\tif (!get_sha1(name, sha1)) {\n+\t\t\tenum object_type type = sha1_object_info(sha1, NULL);\n+\n+\t\t\tif (type > 0 && type != OBJ_COMMIT)\n+\t\t\t\tdie(_(\"%s: can't cherry-pick a %s\"), name, typename(type));\n+\t\t}\n+\t}\n+\n \t/*\n \t * If we were called as \"git cherry-pick \", just\n \t * cherry-pick/revert it, set CHERRY_PICK_HEAD /\ndiff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh\nindex 4e7136b..19c99d7 100755\n--- a/t/t3508-cherry-pick-many-commits.sh\n+++ b/t/t3508-cherry-pick-many-commits.sh\n@@ -55,6 +55,12 @@ one\n two\"\n '\n \n+test_expect_success 'cherry-pick three one two: fails' '\n+\tgit checkout -f master &&\n+\tgit reset --hard first &&\n+\ttest_must_fail git cherry-pick three one two:\n+'\n+\n test_expect_success 'output to keep user entertained during multi-pick' '\n \tcat <<-\\EOF >expected &&\n \t[master OBJID] second\n-- \n1.8.1.4\n"},{"id":"213920","messageId":"CALkWK0n6FjGbXTqiOT_O6NbB5h0DLaNWKCCTQAFSO_BL-pPdBA@mail.gmail.com","threadId":"33363","inReplyTo":"20130411092638.GA12770@suse.cz","subject":"Re: [PATCH v2] cherry-pick: make sure all input objects are commits","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-11T10:22:44Z","receivedAt":"2013-04-11T10:22:44Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Miklos Vajna wrote:\n> When a single argument was a non-commit, the error message used to be:\n>\n> fatal: BUG: expected exactly one commit from walk\n>\n> For multiple arguments, when none of the arguments was a commit, the error was:\n>\n> fatal: empty commit set passed\n>\n> Finally, when some of the arguments were non-commits, we ignored those\n> arguments. Instead, now make sure all arguments are commits, and for\n> the first non-commit, error out with:\n>\n> fatal: : Can't cherry-pick a \n\nThanks. This is worth fixing.\n\n> diff --git a/sequencer.c b/sequencer.c\n> index baa0310..eb25101 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -1067,6 +1068,18 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n> if (opts->subcommand == REPLAY_CONTINUE)\n> return sequencer_continue(opts);\n>\n> + for (i = 0; i < opts->revs->pending.nr; i++) {\n> + unsigned char sha1[20];\n> + const char *name = opts->revs->pending.objects[i].name;\n> +\n> + if (!get_sha1(name, sha1)) {\n> + enum object_type type = sha1_object_info(sha1, NULL);\n> +\n> + if (type > 0 && type != OBJ_COMMIT)\n> + die(_(\"%s: can't cherry-pick a %s\"), name, typename(type));\n> + }\n\nelse? What happens if get_sha1() fails?\n\n> diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh\n> index 4e7136b..19c99d7 100755\n> --- a/t/t3508-cherry-pick-many-commits.sh\n> +++ b/t/t3508-cherry-pick-many-commits.sh\n> @@ -55,6 +55,12 @@ one\n> two\"\n> '\n>\n> +test_expect_success 'cherry-pick three one two: fails' '\n> + git checkout -f master &&\n> + git reset --hard first &&\n> + test_must_fail git cherry-pick three one two:\n> +'\n\nSo you're testing just the third case (where commit objects are mixed\nwith non-commit objects), which is arguably a bug. Okay.\n"},{"id":"213923","messageId":"20130411110324.GD12770@suse.cz","threadId":"33363","inReplyTo":"CALkWK0n6FjGbXTqiOT_O6NbB5h0DLaNWKCCTQAFSO_BL-pPdBA@mail.gmail.com","subject":"Re: [PATCH v2] cherry-pick: make sure all input objects are commits","fromName":"Miklos Vajna","fromEmail":"vmiklos@suse.cz","sentAt":"2013-04-11T11:03:25Z","receivedAt":"2013-04-11T11:03:25Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Thu, Apr 11, 2013 at 03:52:44PM +0530, Ramkumar Ramachandra wrote:\n> > + for (i = 0; i < opts->revs->pending.nr; i++) {\n> > + unsigned char sha1[20];\n> > + const char *name = opts->revs->pending.objects[i].name;\n> > +\n> > + if (!get_sha1(name, sha1)) {\n> > + enum object_type type = sha1_object_info(sha1, NULL);\n> > +\n> > + if (type > 0 && type != OBJ_COMMIT)\n> > + die(_(\"%s: can't cherry-pick a %s\"), name, typename(type));\n> > + }\n> \n> else? What happens if get_sha1() fails?\n\nI guess that is a should-not-happen category. parse_args() calls\nsetup_revisions(), and that will already die() if the argument is not a\nvalid object at all.\n\n> > diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh\n> > index 4e7136b..19c99d7 100755\n> > --- a/t/t3508-cherry-pick-many-commits.sh\n> > +++ b/t/t3508-cherry-pick-many-commits.sh\n> > @@ -55,6 +55,12 @@ one\n> > two\"\n> > '\n> >\n> > +test_expect_success 'cherry-pick three one two: fails' '\n> > + git checkout -f master &&\n> > + git reset --hard first &&\n> > + test_must_fail git cherry-pick three one two:\n> > +'\n> \n> So you're testing just the third case (where commit objects are mixed\n> with non-commit objects), which is arguably a bug. Okay.\n\nYes. If you would want, I could of course add test cases for two other\ncases when we already errored out and now the error message is just\nchanged, but I don't think duplicating the error message strings from\nthe code to the testsuite is really wanted. :-)\n"},{"id":"213926","messageId":"CALkWK0kb+2KZLvRJDJb_VrNNs1k4grsfyFv0HfYv0Kr9v4sChQ@mail.gmail.com","threadId":"33363","inReplyTo":"20130411110324.GD12770@suse.cz","subject":"Re: [PATCH v2] cherry-pick: make sure all input objects are commits","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-11T11:42:06Z","receivedAt":"2013-04-11T11:42:06Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Miklos Vajna wrote:\n> I guess that is a should-not-happen category. parse_args() calls\n> setup_revisions(), and that will already die() if the argument is not a\n> valid object at all.\n\nThen why do you have an if() guarding the code? In my opinion, you\nshould have an else-clause that die()s with an appropriate message.\n\n> Yes. If you would want, I could of course add test cases for two other\n> cases when we already errored out and now the error message is just\n> changed, but I don't think duplicating the error message strings from\n> the code to the testsuite is really wanted. :-)\n\nNope, I'd never suggest that: this is fine. What I meant is: you\nshould clarify that you're fixing a bug and adding a test to guard it,\nin the commit message.\n"},{"id":"213954","messageId":"20130411130652.GG12770@suse.cz","threadId":"33363","inReplyTo":"CALkWK0kb+2KZLvRJDJb_VrNNs1k4grsfyFv0HfYv0Kr9v4sChQ@mail.gmail.com","subject":"[PATCH v3] cherry-pick: make sure all input objects are commits","fromName":"Miklos Vajna","fromEmail":"vmiklos@suse.cz","sentAt":"2013-04-11T13:06:52Z","receivedAt":"2013-04-11T13:06:52Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"When a single argument was a non-commit, the error message used to be:\n\n\tfatal: BUG: expected exactly one commit from walk\n\nFor multiple arguments, when none of the arguments was a commit, the error was:\n\n\tfatal: empty commit set passed\n\nFinally, when some of the arguments were non-commits, we ignored those\narguments. Fix this bug and make sure all arguments are commits, and\nfor the first non-commit, error out with:\n\n\tfatal: : Can't cherry-pick a \n\nSigned-off-by: Miklos Vajna \n---\n\nOn Thu, Apr 11, 2013 at 05:12:06PM +0530, Ramkumar Ramachandra wrote:\n> Then why do you have an if() guarding the code? In my opinion, you\n> should have an else-clause that die()s with an appropriate message.\n\nAnd you were right -- I actually forgot about --stdin, where the \nelse-clause is hit. Added that for now, excluding --stdin.\n\n> Nope, I'd never suggest that: this is fine. What I meant is: you\n> should clarify that you're fixing a bug and adding a test to guard it,\n> in the commit message.\n\nDone.\n\n sequencer.c | 18 ++++++++++++++++++\n t/t3508-cherry-pick-many-commits.sh | 6 ++++++\n 2 files changed, 24 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex baa0310..61fdb68 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1047,6 +1047,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n {\n \tstruct commit_list *todo_list = NULL;\n \tunsigned char sha1[20];\n+\tint i;\n \n \tif (opts->subcommand == REPLAY_NONE)\n \t\tassert(opts->revs);\n@@ -1067,6 +1068,23 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \tif (opts->subcommand == REPLAY_CONTINUE)\n \t\treturn sequencer_continue(opts);\n \n+\tfor (i = 0; i < opts->revs->pending.nr; i++) {\n+\t\tunsigned char sha1[20];\n+\t\tconst char *name = opts->revs->pending.objects[i].name;\n+\n+\t\t/* This happens when using --stdin. */\n+\t\tif (!strlen(name))\n+\t\t\tcontinue;\n+\n+\t\tif (!get_sha1(name, sha1)) {\n+\t\t\tenum object_type type = sha1_object_info(sha1, NULL);\n+\n+\t\t\tif (type > 0 && type != OBJ_COMMIT)\n+\t\t\t\tdie(_(\"%s: can't cherry-pick a %s\"), name, typename(type));\n+\t\t} else\n+\t\t\tdie(_(\"%s: bad revision\"), name);\n+\t}\n+\n \t/*\n \t * If we were called as \"git cherry-pick \", just\n \t * cherry-pick/revert it, set CHERRY_PICK_HEAD /\ndiff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh\nindex 4e7136b..19c99d7 100755\n--- a/t/t3508-cherry-pick-many-commits.sh\n+++ b/t/t3508-cherry-pick-many-commits.sh\n@@ -55,6 +55,12 @@ one\n two\"\n '\n \n+test_expect_success 'cherry-pick three one two: fails' '\n+\tgit checkout -f master &&\n+\tgit reset --hard first &&\n+\ttest_must_fail git cherry-pick three one two:\n+'\n+\n test_expect_success 'output to keep user entertained during multi-pick' '\n \tcat <<-\\EOF >expected &&\n \t[master OBJID] second\n-- \n1.8.1.4\n"},{"id":"213958","messageId":"CALkWK0kj+At9-7=t=Vjh=WoAu_Xv2SOWM6+=eL5ySPYB3+ZEQQ@mail.gmail.com","threadId":"33363","inReplyTo":"20130411130652.GG12770@suse.cz","subject":"Re: [PATCH v3] cherry-pick: make sure all input objects are commits","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-11T13:27:40Z","receivedAt":"2013-04-11T13:27:40Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Miklos Vajna wrote:\n> Signed-off-by: Miklos Vajna \n\nThis one looks good. FWIW,\n\nReviewed-by: Ramkumar Ramachandra \n"},{"id":"214276","messageId":"87vc7odvzi.fsf@linux-k42r.v.cablecom.net","threadId":"33363","inReplyTo":"20130411130652.GG12770@suse.cz","subject":"Re: [PATCH v3] cherry-pick: make sure all input objects are commits","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-04-15T08:44:01Z","receivedAt":"2013-04-15T08:44:01Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Miklos Vajna writes:\n\n> Fix this bug and make sure all arguments are commits, and\n> for the first non-commit, error out with:\n>\n> \tfatal: : Can't cherry-pick a \n\n> @@ -1067,6 +1068,23 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n> \tif (opts->subcommand == REPLAY_CONTINUE)\n> \t\treturn sequencer_continue(opts);\n> \n> +\tfor (i = 0; i < opts->revs->pending.nr; i++) {\n> +\t\tunsigned char sha1[20];\n> +\t\tconst char *name = opts->revs->pending.objects[i].name;\n> +\n> +\t\t/* This happens when using --stdin. */\n> +\t\tif (!strlen(name))\n> +\t\t\tcontinue;\n\nThis is undefined behavior; the pending.objects[i].name has been freed\nalready. Luckily valgrind points you right at it:\n\n ==9178== Invalid read of size 1\n ==9178== at 0x4CEFB4: sequencer_pick_revisions (sequencer.c:1077)\n ==9178== by 0x45E7F2: cmd_cherry_pick (revert.c:236)\n ==9178== by 0x40523C: handle_internal_command (git.c:292)\n ==9178== by 0x405467: main (git.c:500)\n ==9178== Address 0x5bedbd0 is 0 bytes inside a block of size 1,001 free'd\n ==9178== at 0x4C2ACDA: free (vg_replace_malloc.c:468)\n ==9178== by 0x4D96C7: strbuf_release (strbuf.c:40)\n ==9178== by 0x4C9AAE: setup_revisions (revision.c:1285)\n ==9178== by 0x45E6FA: parse_args (revert.c:203)\n ==9178== by 0x45E7EA: cmd_cherry_pick (revert.c:235)\n ==9178== by 0x40523C: handle_internal_command (git.c:292)\n ==9178== by 0x405467: main (git.c:500)\n\n>From a cursory glance it looks like it's actually an existing bug in\nread_revisions_from_stdin or handle_revision_arg, depending on which way\nyou look at it. read_revisions_from_stdin passes its temporary buffer\ndown to handle_revision_arg:\n\n struct strbuf sb;\n [...]\n strbuf_init(&sb, 1000);\n while (strbuf_getwholeline(&sb, stdin, '\\n') != EOF) {\n [...]\n if (handle_revision_arg(sb.buf, revs, 0, REVARG_CANNOT_BE_FILENAME))\n die(\"bad revision '%s'\", sb.buf);\n }\n\nBut handle_revision_arg ends up just stuffing that parameter into the\nrevision-walker options via some helpers:\n\n\tadd_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);\n\tadd_pending_object_with_mode(revs, object, arg, oc.mode);\n\nThis seems to have been lurking since 281eee4 (revision: keep track of\nthe end-user input from the command line, 2011-08-25).\n\nJunio, at which level should we fix it? We could of course have\nread_revisions_from_stdin make a copy of the buffers it passes down, but\nperhaps it would be less surprising to instead have handle_revision_arg\nmake sure it makes a copy of everything it \"keeps\"?\n\nThe easy fix of course is just this:\n\ndiff --git i/revision.c w/revision.c\nindex 3a20c96..181a8db 100644\n--- i/revision.c\n+++ w/revision.c\n@@ -1277,7 +1277,8 @@ static void read_revisions_from_stdin(struct rev_info *revs,\n \t\t\t}\n \t\t\tdie(\"options not supported in --stdin mode\");\n \t\t}\n-\t\tif (handle_revision_arg(sb.buf, revs, 0, REVARG_CANNOT_BE_FILENAME))\n+\t\tif (handle_revision_arg(xstrdup(sb.buf), revs, 0,\n+\t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n \t\t\tdie(\"bad revision '%s'\", sb.buf);\n \t}\n \tif (seen_dashdash)\n\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"214355","messageId":"7vehebsl4h.fsf@alter.siamese.dyndns.org","threadId":"33363","inReplyTo":"87vc7odvzi.fsf@linux-k42r.v.cablecom.net","subject":"Re: [PATCH v3] cherry-pick: make sure all input objects are commits","fromName":"Junio C Hamano","fromEmail":"junio@pobox.com","sentAt":"2013-04-15T18:29:34Z","receivedAt":"2013-04-15T18:29:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> From a cursory glance it looks like it's actually an existing bug in\n> read_revisions_from_stdin or handle_revision_arg, depending on which way\n> you look at it. read_revisions_from_stdin passes its temporary buffer\n> down to handle_revision_arg:\n>\n> struct strbuf sb;\n> [...]\n> strbuf_init(&sb, 1000);\n> while (strbuf_getwholeline(&sb, stdin, '\\n') != EOF) {\n> [...]\n> if (handle_revision_arg(sb.buf, revs, 0, REVARG_CANNOT_BE_FILENAME))\n> die(\"bad revision '%s'\", sb.buf);\n> }\n>\n> But handle_revision_arg ends up just stuffing that parameter into the\n> revision-walker options via some helpers:\n>\n> \tadd_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);\n> \tadd_pending_object_with_mode(revs, object, arg, oc.mode);\n>\n> This seems to have been lurking since 281eee4 (revision: keep track of\n> the end-user input from the command line, 2011-08-25).\n>\n> Junio, at which level should we fix it? We could of course have\n> read_revisions_from_stdin make a copy of the buffers it passes\n> down, but perhaps it would be less surprising to instead have\n> handle_revision_arg make sure it makes a copy of everything it\n> \"keeps\"?\n\nThat sounds like the right way to go to me.\n\n> The easy fix of course is just this:\n>\n> diff --git i/revision.c w/revision.c\n> index 3a20c96..181a8db 100644\n> --- i/revision.c\n> +++ w/revision.c\n> @@ -1277,7 +1277,8 @@ static void read_revisions_from_stdin(struct rev_info *revs,\n> \t\t\t}\n> \t\t\tdie(\"options not supported in --stdin mode\");\n> \t\t}\n> -\t\tif (handle_revision_arg(sb.buf, revs, 0, REVARG_CANNOT_BE_FILENAME))\n> +\t\tif (handle_revision_arg(xstrdup(sb.buf), revs, 0,\n> +\t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n> \t\t\tdie(\"bad revision '%s'\", sb.buf);\n> \t}\n> \tif (seen_dashdash)\n"},{"id":"214373","messageId":"7v61znsj49.fsf@alter.siamese.dyndns.org","threadId":"33363","inReplyTo":"87vc7odvzi.fsf@linux-k42r.v.cablecom.net","subject":"Re: [PATCH v3] cherry-pick: make sure all input objects are commits","fromName":"Junio C Hamano","fromEmail":"junio@pobox.com","sentAt":"2013-04-15T19:12:54Z","receivedAt":"2013-04-15T19:12:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> From a cursory glance it looks like it's actually an existing bug in\n> read_revisions_from_stdin or handle_revision_arg, depending on which way\n> you look at it. read_revisions_from_stdin passes its temporary buffer\n> down to handle_revision_arg:\n>\n> struct strbuf sb;\n> [...]\n> strbuf_init(&sb, 1000);\n> while (strbuf_getwholeline(&sb, stdin, '\\n') != EOF) {\n> [...]\n> if (handle_revision_arg(sb.buf, revs, 0, REVARG_CANNOT_BE_FILENAME))\n> die(\"bad revision '%s'\", sb.buf);\n> }\n>\n> But handle_revision_arg ends up just stuffing that parameter into the\n> revision-walker options via some helpers:\n>\n> \tadd_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);\n> \tadd_pending_object_with_mode(revs, object, arg, oc.mode);\n>\n> This seems to have been lurking since 281eee4 (revision: keep track of\n> the end-user input from the command line, 2011-08-25).\n>\n> Junio, at which level should we fix it? We could of course have\n> read_revisions_from_stdin make a copy of the buffers it passes down, but\n> perhaps it would be less surprising to instead have handle_revision_arg\n> make sure it makes a copy of everything it \"keeps\"?\n\nLooking at it again, it seems that the issue is much older than the\nintroduction of cmdline interface.\n\nEverything we throw at add_pending_object() is assumed to be stable,\nbecause historically they were argv[] strings, and --stdin is what\nbreaks that assumption. Making copies unconditionally at the lower\nlayer only because some minority callers give it unstable strings\ndoes not sound like a good trade-off.\n\nSo I changed my mind. Your \"easy fix\" looks to me the right thing\nto do.\n\nThe paths given to handle_refs() may also have to be copied before\nsaved, depending on how ref iteration is implemented, details of\nwhich may change as Michael seems to be updating the area again.\nI think we let the callback peek ref_entry->name[] which is stable,\nso I suspect we are OK.\n\n> The easy fix of course is just this:\n>\n> diff --git i/revision.c w/revision.c\n> index 3a20c96..181a8db 100644\n> --- i/revision.c\n> +++ w/revision.c\n> @@ -1277,7 +1277,8 @@ static void read_revisions_from_stdin(struct rev_info *revs,\n> \t\t\t}\n> \t\t\tdie(\"options not supported in --stdin mode\");\n> \t\t}\n> -\t\tif (handle_revision_arg(sb.buf, revs, 0, REVARG_CANNOT_BE_FILENAME))\n> +\t\tif (handle_revision_arg(xstrdup(sb.buf), revs, 0,\n> +\t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n> \t\t\tdie(\"bad revision '%s'\", sb.buf);\n> \t}\n> \tif (seen_dashdash)\n"},{"id":"214488","messageId":"516D5EAD.4070503@alum.mit.edu","threadId":"33363","inReplyTo":"7v61znsj49.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] cherry-pick: make sure all input objects are commits","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-16T14:22:37Z","receivedAt":"2013-04-16T14:22:37Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/15/2013 09:12 PM, Junio C Hamano wrote:\n> The paths given to handle_refs() may also have to be copied before\n> saved, depending on how ref iteration is implemented, details of\n> which may change as Michael seems to be updating the area again.\n> I think we let the callback peek ref_entry->name[] which is stable,\n> so I suspect we are OK.\n\nref_entry->name is stable as long as invalidate_ref_cache() is not\ncalled, and I am not even thinking of changing that (partly because I\ndon't have the energy to audit and adjust all of the callers).\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"216810","messageId":"7vy5bo7x62.fsf@alter.siamese.dyndns.org","threadId":"33363","inReplyTo":"20130411130652.GG12770@suse.cz","subject":"Re: [PATCH v3] cherry-pick: make sure all input objects are commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-09T19:47:33Z","receivedAt":"2013-05-09T19:47:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miklos Vajna writes:\n\n> When a single argument was a non-commit, the error message used to be:\n>\n> \tfatal: BUG: expected exactly one commit from walk\n>\n> For multiple arguments, when none of the arguments was a commit, the error was:\n>\n> \tfatal: empty commit set passed\n>\n> Finally, when some of the arguments were non-commits, we ignored those\n> arguments. Fix this bug and make sure all arguments are commits, and\n> for the first non-commit, error out with:\n>\n> \tfatal: : Can't cherry-pick a \n>\n> Signed-off-by: Miklos Vajna \n\nThis turns out to be an irritatingly stupid change. While I am\nrebuilding a privately tagged tip of 'maint', I am seeing:\n\n\tfatal: v1.8.2.3: Can't cherry-pick a tag\n\nYou would want to reject non committish, not non commit.\n\n> sequencer.c | 18 ++++++++++++++++++\n> t/t3508-cherry-pick-many-commits.sh | 6 ++++++\n> 2 files changed, 24 insertions(+)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index baa0310..61fdb68 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -1047,6 +1047,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n> {\n> \tstruct commit_list *todo_list = NULL;\n> \tunsigned char sha1[20];\n> +\tint i;\n> \n> \tif (opts->subcommand == REPLAY_NONE)\n> \t\tassert(opts->revs);\n> @@ -1067,6 +1068,23 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n> \tif (opts->subcommand == REPLAY_CONTINUE)\n> \t\treturn sequencer_continue(opts);\n> \n> +\tfor (i = 0; i < opts->revs->pending.nr; i++) {\n> +\t\tunsigned char sha1[20];\n> +\t\tconst char *name = opts->revs->pending.objects[i].name;\n> +\n> +\t\t/* This happens when using --stdin. */\n> +\t\tif (!strlen(name))\n> +\t\t\tcontinue;\n> +\n> +\t\tif (!get_sha1(name, sha1)) {\n> +\t\t\tenum object_type type = sha1_object_info(sha1, NULL);\n> +\n> +\t\t\tif (type > 0 && type != OBJ_COMMIT)\n> +\t\t\t\tdie(_(\"%s: can't cherry-pick a %s\"), name, typename(type));\n> +\t\t} else\n> +\t\t\tdie(_(\"%s: bad revision\"), name);\n> +\t}\n> +\n> \t/*\n> \t * If we were called as \"git cherry-pick \", just\n> \t * cherry-pick/revert it, set CHERRY_PICK_HEAD /\n> diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh\n> index 4e7136b..19c99d7 100755\n> --- a/t/t3508-cherry-pick-many-commits.sh\n> +++ b/t/t3508-cherry-pick-many-commits.sh\n> @@ -55,6 +55,12 @@ one\n> two\"\n> '\n> \n> +test_expect_success 'cherry-pick three one two: fails' '\n> +\tgit checkout -f master &&\n> +\tgit reset --hard first &&\n> +\ttest_must_fail git cherry-pick three one two:\n> +'\n> +\n> test_expect_success 'output to keep user entertained during multi-pick' '\n> \tcat <<-\\EOF >expected &&\n> \t[master OBJID] second\n"},{"id":"216813","messageId":"7vsj1v99ve.fsf@alter.siamese.dyndns.org","threadId":"33363","inReplyTo":"7vy5bo7x62.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] cherry-pick: make sure all input objects are commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-09T20:27:49Z","receivedAt":"2013-05-09T20:27:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Miklos Vajna writes:\n>\n>> When a single argument was a non-commit, the error message used to be:\n>>\n>> \tfatal: BUG: expected exactly one commit from walk\n>>\n>> For multiple arguments, when none of the arguments was a commit, the error was:\n>>\n>> \tfatal: empty commit set passed\n>>\n>> Finally, when some of the arguments were non-commits, we ignored those\n>> arguments. Fix this bug and make sure all arguments are commits, and\n>> for the first non-commit, error out with:\n>>\n>> \tfatal: : Can't cherry-pick a \n>>\n>> Signed-off-by: Miklos Vajna \n>\n> This turns out to be an irritatingly stupid change. While I am\n> rebuilding a privately tagged tip of 'maint', I am seeing:\n>\n> \tfatal: v1.8.2.3: Can't cherry-pick a tag\n>\n> You would want to reject non committish, not non commit.\n\nI'd apply this before -rc2. I _think_ it is also OK to just let\nlookup_commit_reference_gently() barf with its standard message\n\n\terror: Object %s is a %s, not a commit\n\nwithout an extra sha1_object_info() call in the error codepath, but\nI did not bother, as this is meant to be an emergency fix.\n\n-- >8 --\nSubject: cherry-pick: picking a tag that resolves to a commit is OK\n\nEarlier, 21246dbb9e0a (cherry-pick: make sure all input objects are\ncommits, 2013-04-11) tried to catch an unlikely \"git cherry-pick $blob\"\nas an error, but broke a more important use case to cherry-pick a\ntag that points at a commit.\n\nSigned-off-by: Junio C Hamano \n---\n sequencer.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 61fdb68..f2c9d98 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1077,10 +1077,10 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \t\t\tcontinue;\n \n \t\tif (!get_sha1(name, sha1)) {\n-\t\t\tenum object_type type = sha1_object_info(sha1, NULL);\n-\n-\t\t\tif (type > 0 && type != OBJ_COMMIT)\n+\t\t\tif (!lookup_commit_reference_gently(sha1, 1)) {\n+\t\t\t\tenum object_type type = sha1_object_info(sha1, NULL);\n \t\t\t\tdie(_(\"%s: can't cherry-pick a %s\"), name, typename(type));\n+\t\t\t}\n \t\t} else\n \t\t\tdie(_(\"%s: bad revision\"), name);\n \t}\n"},{"id":"216855","messageId":"20130510070712.GA24415@suse.cz","threadId":"33363","inReplyTo":"7vsj1v99ve.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] cherry-pick: make sure all input objects are commits","fromName":"Miklos Vajna","fromEmail":"vmiklos@suse.cz","sentAt":"2013-05-10T07:07:13Z","receivedAt":"2013-05-10T07:07:13Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Thu, May 09, 2013 at 01:27:49PM -0700, Junio C Hamano wrote:\n> I'd apply this before -rc2. I _think_ it is also OK to just let\n> lookup_commit_reference_gently() barf with its standard message\n> \n> \terror: Object %s is a %s, not a commit\n> \n> without an extra sha1_object_info() call in the error codepath, but\n> I did not bother, as this is meant to be an emergency fix.\n\nYes, that makes a lot of sense. I myself never cherry-pick tags, but I\nunderstand that is part of some workflow.\n"}]}