Re: [PATCH v3] cherry-pick: make sure all input objects are commits
On Thu, May 09, 2013 at 01:27:49PM -0700, Junio C Hamano 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. signature.asc Description: Digital signature
Re: [PATCH v3] cherry-pick: make sure all input objects are commits
Junio C Hamano writes:
> Miklos Vajna 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: : Can't cherry-pick a
>>
>> Signed-off-by: Miklos Vajna
>
> 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
---
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);
}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] cherry-pick: make sure all input objects are commits
Miklos Vajna 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: : Can't cherry-pick a
>
> Signed-off-by: Miklos Vajna
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 ", 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] cherry-pick: make sure all input objects are commits
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 mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] cherry-pick: make sure all input objects are commits
Thomas Rast 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)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] cherry-pick: make sure all input objects are commits
Thomas Rast 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)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] cherry-pick: make sure all input objects are commits
Miklos Vajna writes:
> Fix this bug and make sure all arguments are commits, and
> for the first non-commit, error out with:
>
> fatal: : Can't cherry-pick a
> @@ -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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] cherry-pick: make sure all input objects are commits
Miklos Vajna wrote: > Signed-off-by: Miklos Vajna This one looks good. FWIW, Reviewed-by: Ramkumar Ramachandra -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
