* [PATCH] cherry-pick: better error message when the parameter is a non-commit @ 2013-04-03 9:27 Miklos Vajna 2013-04-08 12:27 ` Miklos Vajna 2013-04-08 16:56 ` Junio C Hamano 0 siblings, 2 replies; 17+ messages in thread From: Miklos Vajna @ 2013-04-03 9:27 UTC (permalink / raw) To: gitster; +Cc: git 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 Signed-off-by: Miklos Vajna <vmiklos@suse.cz> --- sequencer.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index baa0310..0ac00d4 100644 --- a/sequencer.c +++ b/sequencer.c @@ -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); } -- 1.8.1.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] cherry-pick: better error message when the parameter is a non-commit 2013-04-03 9:27 [PATCH] cherry-pick: better error message when the parameter is a non-commit Miklos Vajna @ 2013-04-08 12:27 ` Miklos Vajna 2013-04-08 16:45 ` Junio C Hamano 2013-04-08 16:56 ` Junio C Hamano 1 sibling, 1 reply; 17+ messages in thread From: Miklos Vajna @ 2013-04-08 12:27 UTC (permalink / raw) To: gitster; +Cc: git Hi, On Wed, Apr 03, 2013 at 11:27:04AM +0200, Miklos Vajna <vmiklos@suse.cz> wrote: > 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 > > Signed-off-by: Miklos Vajna <vmiklos@suse.cz> > --- > sequencer.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) Ping, any comment on this patch? Thanks, Miklos ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] cherry-pick: better error message when the parameter is a non-commit 2013-04-08 12:27 ` Miklos Vajna @ 2013-04-08 16:45 ` Junio C Hamano 0 siblings, 0 replies; 17+ messages in thread From: Junio C Hamano @ 2013-04-08 16:45 UTC (permalink / raw) To: Miklos Vajna; +Cc: git Miklos Vajna <vmiklos@suse.cz> writes: > On Wed, Apr 03, 2013 at 11:27:04AM +0200, Miklos Vajna <vmiklos@suse.cz> wrote: >> 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 >> >> Signed-off-by: Miklos Vajna <vmiklos@suse.cz> >> --- >> sequencer.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) > > Ping, any comment on this patch? Nothing in particular from me. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] cherry-pick: better error message when the parameter is a non-commit 2013-04-03 9:27 [PATCH] cherry-pick: better error message when the parameter is a non-commit Miklos Vajna 2013-04-08 12:27 ` Miklos Vajna @ 2013-04-08 16:56 ` Junio C Hamano 2013-04-11 9:26 ` [PATCH v2] cherry-pick: make sure all input objects are commits Miklos Vajna 1 sibling, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2013-04-08 16:56 UTC (permalink / raw) To: Miklos Vajna; +Cc: git Miklos Vajna <vmiklos@suse.cz> writes: > 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. > 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? > > Signed-off-by: Miklos Vajna <vmiklos@suse.cz> > --- > sequencer.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/sequencer.c b/sequencer.c > index baa0310..0ac00d4 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -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); > } ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2] cherry-pick: make sure all input objects are commits 2013-04-08 16:56 ` Junio C Hamano @ 2013-04-11 9:26 ` Miklos Vajna 2013-04-11 10:22 ` Ramkumar Ramachandra 0 siblings, 1 reply; 17+ messages in thread From: Miklos Vajna @ 2013-04-11 9:26 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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> Signed-off-by: Miklos Vajna <vmiklos@suse.cz> --- On Mon, Apr 08, 2013 at 09:56:55AM -0700, Junio C Hamano <gitster@pobox.com> wrote: > 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? Sure, that makes sense to me. sequencer.c | 13 +++++++++++++ t/t3508-cherry-pick-many-commits.sh | 6 ++++++ 2 files changed, 19 insertions(+) diff --git a/sequencer.c b/sequencer.c index baa0310..eb25101 100644 --- a/sequencer.c +++ b/sequencer.c @@ -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 / diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh index 4e7136b..19c99d7 100755 --- a/t/t3508-cherry-pick-many-commits.sh +++ b/t/t3508-cherry-pick-many-commits.sh @@ -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 -- 1.8.1.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2] cherry-pick: make sure all input objects are commits 2013-04-11 9:26 ` [PATCH v2] cherry-pick: make sure all input objects are commits Miklos Vajna @ 2013-04-11 10:22 ` Ramkumar Ramachandra 2013-04-11 11:03 ` Miklos Vajna 0 siblings, 1 reply; 17+ messages in thread From: Ramkumar Ramachandra @ 2013-04-11 10:22 UTC (permalink / raw) To: Miklos Vajna; +Cc: Junio C Hamano, git Miklos Vajna wrote: > 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> Thanks. This is worth fixing. > diff --git a/sequencer.c b/sequencer.c > index baa0310..eb25101 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -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)); > + } else? What happens if get_sha1() fails? > diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh > index 4e7136b..19c99d7 100755 > --- a/t/t3508-cherry-pick-many-commits.sh > +++ b/t/t3508-cherry-pick-many-commits.sh > @@ -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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] cherry-pick: make sure all input objects are commits 2013-04-11 10:22 ` Ramkumar Ramachandra @ 2013-04-11 11:03 ` Miklos Vajna 2013-04-11 11:42 ` Ramkumar Ramachandra 0 siblings, 1 reply; 17+ messages in thread From: Miklos Vajna @ 2013-04-11 11:03 UTC (permalink / raw) To: Ramkumar Ramachandra; +Cc: Junio C Hamano, git [-- Attachment #1: Type: text/plain, Size: 1701 bytes --] On Thu, Apr 11, 2013 at 03:52:44PM +0530, Ramkumar Ramachandra <artagnon@gmail.com> wrote: > > + 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? 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. > > diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh > > index 4e7136b..19c99d7 100755 > > --- a/t/t3508-cherry-pick-many-commits.sh > > +++ b/t/t3508-cherry-pick-many-commits.sh > > @@ -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. 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. :-) [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] cherry-pick: make sure all input objects are commits 2013-04-11 11:03 ` Miklos Vajna @ 2013-04-11 11:42 ` Ramkumar Ramachandra 2013-04-11 13:06 ` [PATCH v3] " Miklos Vajna 0 siblings, 1 reply; 17+ messages in thread From: Ramkumar Ramachandra @ 2013-04-11 11:42 UTC (permalink / raw) To: Miklos Vajna; +Cc: Junio C Hamano, git Miklos Vajna wrote: > 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. 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. > 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3] cherry-pick: make sure all input objects are commits 2013-04-11 11:42 ` Ramkumar Ramachandra @ 2013-04-11 13:06 ` Miklos Vajna 2013-04-11 13:27 ` Ramkumar Ramachandra ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Miklos Vajna @ 2013-04-11 13:06 UTC (permalink / raw) To: Junio C Hamano; +Cc: Ramkumar Ramachandra, git 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> Signed-off-by: Miklos Vajna <vmiklos@suse.cz> --- On Thu, Apr 11, 2013 at 05:12:06PM +0530, Ramkumar Ramachandra <artagnon@gmail.com> wrote: > 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. > 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(+) diff --git a/sequencer.c b/sequencer.c index baa0310..61fdb68 100644 --- a/sequencer.c +++ b/sequencer.c @@ -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 / diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh index 4e7136b..19c99d7 100755 --- a/t/t3508-cherry-pick-many-commits.sh +++ b/t/t3508-cherry-pick-many-commits.sh @@ -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 -- 1.8.1.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3] cherry-pick: make sure all input objects are commits 2013-04-11 13:06 ` [PATCH v3] " Miklos Vajna @ 2013-04-11 13:27 ` Ramkumar Ramachandra 2013-04-15 8:44 ` Thomas Rast 2013-05-09 19:47 ` Junio C Hamano 2 siblings, 0 replies; 17+ messages in thread From: Ramkumar Ramachandra @ 2013-04-11 13:27 UTC (permalink / raw) To: Miklos Vajna; +Cc: Junio C Hamano, git Miklos Vajna wrote: > Signed-off-by: Miklos Vajna <vmiklos@suse.cz> This one looks good. FWIW, Reviewed-by: Ramkumar Ramachandra <artagnon@gmail.com> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3] cherry-pick: make sure all input objects are commits 2013-04-11 13:06 ` [PATCH v3] " Miklos Vajna 2013-04-11 13:27 ` Ramkumar Ramachandra @ 2013-04-15 8:44 ` Thomas Rast 2013-04-15 18:29 ` Junio C Hamano 2013-04-15 19:12 ` Junio C Hamano 2013-05-09 19:47 ` Junio C Hamano 2 siblings, 2 replies; 17+ messages in thread From: Thomas Rast @ 2013-04-15 8:44 UTC (permalink / raw) To: Miklos Vajna; +Cc: Junio C Hamano, Ramkumar Ramachandra, git Miklos Vajna <vmiklos@suse.cz> writes: > 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> > @@ -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) >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: diff --git i/revision.c w/revision.c index 3a20c96..181a8db 100644 --- i/revision.c +++ w/revision.c @@ -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) -- Thomas Rast trast@{inf,student}.ethz.ch ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3] cherry-pick: make sure all input objects are commits 2013-04-15 8:44 ` Thomas Rast @ 2013-04-15 18:29 ` Junio C Hamano 2013-04-15 19:12 ` Junio C Hamano 1 sibling, 0 replies; 17+ messages in thread From: Junio C Hamano @ 2013-04-15 18:29 UTC (permalink / raw) To: Thomas Rast; +Cc: Miklos Vajna, Ramkumar Ramachandra, git Thomas Rast <trast@inf.ethz.ch> writes: > 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"? That sounds like the right way to go to me. > The easy fix of course is just this: > > diff --git i/revision.c w/revision.c > index 3a20c96..181a8db 100644 > --- i/revision.c > +++ w/revision.c > @@ -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) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3] cherry-pick: make sure all input objects are commits 2013-04-15 8:44 ` Thomas Rast 2013-04-15 18:29 ` Junio C Hamano @ 2013-04-15 19:12 ` Junio C Hamano 2013-04-16 14:22 ` Michael Haggerty 1 sibling, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2013-04-15 19:12 UTC (permalink / raw) To: Thomas Rast; +Cc: Miklos Vajna, Ramkumar Ramachandra, git, Michael Haggerty Thomas Rast <trast@inf.ethz.ch> writes: > 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"? 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. > The easy fix of course is just this: > > diff --git i/revision.c w/revision.c > index 3a20c96..181a8db 100644 > --- i/revision.c > +++ w/revision.c > @@ -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) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3] cherry-pick: make sure all input objects are commits 2013-04-15 19:12 ` Junio C Hamano @ 2013-04-16 14:22 ` Michael Haggerty 0 siblings, 0 replies; 17+ messages in thread From: Michael Haggerty @ 2013-04-16 14:22 UTC (permalink / raw) To: Junio C Hamano; +Cc: Thomas Rast, Miklos Vajna, Ramkumar Ramachandra, git On 04/15/2013 09:12 PM, Junio C Hamano wrote: > 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 -- Michael Haggerty mhagger@alum.mit.edu http://softwareswirl.blogspot.com/ ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3] cherry-pick: make sure all input objects are commits 2013-04-11 13:06 ` [PATCH v3] " Miklos Vajna 2013-04-11 13:27 ` Ramkumar Ramachandra 2013-04-15 8:44 ` Thomas Rast @ 2013-05-09 19:47 ` Junio C Hamano 2013-05-09 20:27 ` Junio C Hamano 2 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2013-05-09 19:47 UTC (permalink / raw) To: Miklos Vajna; +Cc: Ramkumar Ramachandra, git Miklos Vajna <vmiklos@suse.cz> writes: > 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> > > Signed-off-by: Miklos Vajna <vmiklos@suse.cz> 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. > sequencer.c | 18 ++++++++++++++++++ > t/t3508-cherry-pick-many-commits.sh | 6 ++++++ > 2 files changed, 24 insertions(+) > > diff --git a/sequencer.c b/sequencer.c > index baa0310..61fdb68 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -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 / > diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh > index 4e7136b..19c99d7 100755 > --- a/t/t3508-cherry-pick-many-commits.sh > +++ b/t/t3508-cherry-pick-many-commits.sh > @@ -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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3] cherry-pick: make sure all input objects are commits 2013-05-09 19:47 ` Junio C Hamano @ 2013-05-09 20:27 ` Junio C Hamano 2013-05-10 7:07 ` Miklos Vajna 0 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2013-05-09 20:27 UTC (permalink / raw) To: Miklos Vajna; +Cc: Ramkumar Ramachandra, git Junio C Hamano <gitster@pobox.com> writes: > Miklos Vajna <vmiklos@suse.cz> writes: > >> 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> >> >> Signed-off-by: Miklos Vajna <vmiklos@suse.cz> > > 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. 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. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- sequencer.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sequencer.c b/sequencer.c index 61fdb68..f2c9d98 100644 --- a/sequencer.c +++ b/sequencer.c @@ -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); } ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3] cherry-pick: make sure all input objects are commits 2013-05-09 20:27 ` Junio C Hamano @ 2013-05-10 7:07 ` Miklos Vajna 0 siblings, 0 replies; 17+ messages in thread From: Miklos Vajna @ 2013-05-10 7:07 UTC (permalink / raw) To: Junio C Hamano; +Cc: Ramkumar Ramachandra, git [-- Attachment #1: Type: text/plain, Size: 521 bytes --] On Thu, May 09, 2013 at 01:27:49PM -0700, Junio C Hamano <gitster@pobox.com> wrote: > 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. Yes, that makes a lot of sense. I myself never cherry-pick tags, but I understand that is part of some workflow. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2013-05-10 7:07 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-04-03 9:27 [PATCH] cherry-pick: better error message when the parameter is a non-commit Miklos Vajna 2013-04-08 12:27 ` Miklos Vajna 2013-04-08 16:45 ` Junio C Hamano 2013-04-08 16:56 ` Junio C Hamano 2013-04-11 9:26 ` [PATCH v2] cherry-pick: make sure all input objects are commits Miklos Vajna 2013-04-11 10:22 ` Ramkumar Ramachandra 2013-04-11 11:03 ` Miklos Vajna 2013-04-11 11:42 ` Ramkumar Ramachandra 2013-04-11 13:06 ` [PATCH v3] " Miklos Vajna 2013-04-11 13:27 ` Ramkumar Ramachandra 2013-04-15 8:44 ` Thomas Rast 2013-04-15 18:29 ` Junio C Hamano 2013-04-15 19:12 ` Junio C Hamano 2013-04-16 14:22 ` Michael Haggerty 2013-05-09 19:47 ` Junio C Hamano 2013-05-09 20:27 ` Junio C Hamano 2013-05-10 7:07 ` Miklos Vajna
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.