[PATCH] cherry-pick: better error message when the parameter is a non-commit

[PATCH] cherry-pick: better error message when the parameter is a non-commit

[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:

	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

Re: [PATCH] cherry-pick: better error message when the parameter is a non-commit

[Miklos Vajna]

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

Re: [PATCH] cherry-pick: better error message when the parameter is a non-commit

[Junio C Hamano]

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.

Re: [PATCH] cherry-pick: better error message when the parameter is a non-commit

[Junio C Hamano]

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);
>  	}

[PATCH v2] cherry-pick: make sure all input objects are commits

[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.  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

Re: [PATCH v2] cherry-pick: make sure all input objects are commits

[Ramkumar Ramachandra]

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.

Re: [PATCH v2] cherry-pick: make sure all input objects are commits

[Miklos Vajna]

[-- 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 --]

Re: [PATCH v2] cherry-pick: make sure all input objects are commits

[Ramkumar Ramachandra]

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.

[PATCH v3] cherry-pick: make sure all input objects are commits

[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>

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

Re: [PATCH v3] cherry-pick: make sure all input objects are commits

[Ramkumar Ramachandra]

Miklos Vajna wrote:
> Signed-off-by: Miklos Vajna <vmiklos@suse.cz>

This one looks good.  FWIW,

Reviewed-by: Ramkumar Ramachandra <artagnon@gmail.com>

Re: [PATCH v3] cherry-pick: make sure all input objects are commits

[Thomas Rast]

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

Re: [PATCH v3] cherry-pick: make sure all input objects are commits

[Junio C Hamano]

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)

Re: [PATCH v3] cherry-pick: make sure all input objects are commits

[Junio C Hamano]

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)

Re: [PATCH v3] cherry-pick: make sure all input objects are commits

[Michael Haggerty]

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/

Re: [PATCH v3] cherry-pick: make sure all input objects are commits

[Junio C Hamano]

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

Re: [PATCH v3] cherry-pick: make sure all input objects are commits

[Junio C Hamano]

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);
 	}

Re: [PATCH v3] cherry-pick: make sure all input objects are commits

[Miklos Vajna]

[-- 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 --]