[BUG] git cherry-pick does not complain about unknown options

[BUG] git cherry-pick does not complain about unknown options

[Andrei Rybak]

Hi,

I was trying to cherry pick commits, while simultaneously changing the
author.  Unfortunately, cherry-pick doesn't have the same --author
option as git-commit.  However, instead of complaining about unknown
option:

- when trying to cherry-pick one commit, it reported a BUG
- when trying to cherry-pick several commits, cherry-pick silently
  did nothing

All commits in tests existed in repository:

    $ git cherry-pick --author='TEST' <one-commit> # case 1
    error: BUG: expected exactly one commit from walk
    fatal: cherry-pick failed
    $ echo $?
    128
    $ git cherry-pick --author='TEST' <commit1> <commit2>  # case 2
    $ echo $?
    0
    $ git --version
    git version 2.18.0.windows.1

I've encountered this issue in Windows version, and  Johannes Schindelin
has confirmed that the issue is also present in Linux version.

Originally reported here: https://github.com/git-for-windows/git/issues/1751

--
Best regards, Andrei Rybak

Re: [BUG] git cherry-pick does not complain about unknown options

[Jeff King]

On Mon, Jul 09, 2018 at 04:16:16PM +0200, Andrei Rybak wrote:

> I was trying to cherry pick commits, while simultaneously changing the
> author.  Unfortunately, cherry-pick doesn't have the same --author
> option as git-commit.  However, instead of complaining about unknown
> option:

Yeah, its "--author" option is interpreted by revision.c similar to "log
--author" (because we just pass all unknown options to the traversal
machinery). So you could say:

  git cherry-pick --author=peff v1.0..v2.0

to pick all of my commits between the two versions. I'm not sure if that
would ever be _useful_, but that's how it has behaved since 7e2bfd3f99
(revert: allow cherry-picking more than one commit, 2010-06-02), I
think.

I agree that having something similar to commit's "--author" (or even
just "--reset-author") would be useful. When I've had to do this before,
I usually just cherry-pick and then follow-up with "commit --amend
--author" (or use "rebase -i" if there are several commits). You can
also do "cherry-pick -n $commit" followed by "commit -c $commit".

> All commits in tests existed in repository:
> 
>     $ git cherry-pick --author='TEST' <one-commit> # case 1
>     error: BUG: expected exactly one commit from walk
>     fatal: cherry-pick failed
>     $ echo $?
>     128

I think it's reasonable for this to issue an error. But a BUG is
definitely wrong. The error is more like "empty commit set passed",
which is what we get for something like:

  $ git cherry-pick HEAD..HEAD
  error: empty commit set passed
  fatal: cherry-pick failed

that triggers the revision walker but has no positive commits. But we
don't trigger that code because the single-rev case has some magic to
allow a cherry-pick during another sequencer operation.

So I think that BUG message needs to be softened.

>     $ git cherry-pick --author='TEST' <commit1> <commit2>  # case 2
>     $ echo $?
>     0

And here we come up with an empty set, but we don't flag it as an error
at all. I could even accept that philosophically as "empty input,
nothing to do", but we clearly do flag it as an error in the case I
showed above.

I think the problem is that we catch the error in
sequencer.c:prepare_revs(), after prepare_revision_walk() returns no
commits. But we _do_ have a starting commit for this case, and it's not
culled until we actually ask get_revisions() to start walking.

I think we'd want something like this:

diff --git a/sequencer.c b/sequencer.c
index 4034c0461b..b978157399 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1860,8 +1860,6 @@ static int prepare_revs(struct replay_opts *opts)
 	if (prepare_revision_walk(opts->revs))
 		return error(_("revision walk setup failed"));
 
-	if (!opts->revs->commits)
-		return error(_("empty commit set passed"));
 	return 0;
 }
 
@@ -2314,6 +2312,10 @@ static int walk_revs_populate_todo(struct todo_list *todo_list,
 			short_commit_name(commit), subject_len, subject);
 		unuse_commit_buffer(commit, commit_buffer);
 	}
+
+	if (!todo_list->nr)
+		return error(_("empty commit set passed"));
+
 	return 0;
 }
 

-Peff

[PATCH 0/2] de-confuse git cherry-pick --author behavior

[Jeff King]

On Mon, Jul 09, 2018 at 03:16:07PM -0400, Jeff King wrote:

> I agree that having something similar to commit's "--author" (or even
> just "--reset-author") would be useful. When I've had to do this before,
> I usually just cherry-pick and then follow-up with "commit --amend
> --author" (or use "rebase -i" if there are several commits). You can
> also do "cherry-pick -n $commit" followed by "commit -c $commit".

I'll leave adding any options there as an exercise if somebody is
interested. This series just focuses on the inconsistent error behavior
you found.

  [1/2]: sequencer: handle empty-set cases consistently
  [2/2]: sequencer: don't say BUG on bogus input

 sequencer.c                     | 8 +++++---
 t/t3510-cherry-pick-sequence.sh | 7 ++++++-
 2 files changed, 11 insertions(+), 4 deletions(-)

-Peff

[PATCH 1/2] sequencer: handle empty-set cases consistently

[Jeff King]

If the user gives us a set that prepare_revision_walk()
takes to be empty, like:

  git cherry-pick base..base

then we report an error. It's nonsense, and there's nothing
to pick.

But if they use revision options that later cull the list,
like:

  git cherry-pick --author=nobody base~2..base

then we quietly create an empty todo list and return
success.

Arguably either behavior is acceptable, but we should
definitely be consistent about it. Reporting an error
seems to match the original intent, which dates all the way
back to 7e2bfd3f99 (revert: allow cherry-picking more than
one commit, 2010-06-02). That in turn was trying to match
the single-commit case that exited before then (and which
continues to issue an error).

Signed-off-by: Jeff King <peff@peff.net>
---
 sequencer.c                     | 6 ++++--
 t/t3510-cherry-pick-sequence.sh | 7 ++++++-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 5354d4d51e..f692b2ef44 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1863,8 +1863,6 @@ static int prepare_revs(struct replay_opts *opts)
 	if (prepare_revision_walk(opts->revs))
 		return error(_("revision walk setup failed"));
 
-	if (!opts->revs->commits)
-		return error(_("empty commit set passed"));
 	return 0;
 }
 
@@ -2317,6 +2315,10 @@ static int walk_revs_populate_todo(struct todo_list *todo_list,
 			short_commit_name(commit), subject_len, subject);
 		unuse_commit_buffer(commit, commit_buffer);
 	}
+
+	if (!todo_list->nr)
+		return error(_("empty commit set passed"));
+
 	return 0;
 }
 
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index b42cd66d3a..3505b6aa14 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -480,11 +480,16 @@ test_expect_success 'malformed instruction sheet 2' '
 	test_expect_code 128 git cherry-pick --continue
 '
 
-test_expect_success 'empty commit set' '
+test_expect_success 'empty commit set (no commits to walk)' '
 	pristine_detach initial &&
 	test_expect_code 128 git cherry-pick base..base
 '
 
+test_expect_success 'empty commit set (culled during walk)' '
+	pristine_detach initial &&
+	test_expect_code 128 git cherry-pick -2 --author=no.such.author base
+'
+
 test_expect_success 'malformed instruction sheet 3' '
 	pristine_detach initial &&
 	test_expect_code 1 git cherry-pick base..anotherpick &&
-- 
2.18.0.400.g702e398724

[PATCH 2/2] sequencer: don't say BUG on bogus input

[Jeff King]

When cherry-picking a single commit, we go through a special
code path that avoids creating a sequencer todo list at all.
This path expects our revision parsing to turn up exactly
one commit, and dies with a BUG if it doesn't.

But it's actually quite easy to fool. For example:

  $ git cherry-pick --author=no.such.person HEAD
  error: BUG: expected exactly one commit from walk
  fatal: cherry-pick failed

This isn't a bug; it's just bogus input.

Let's drop the "BUG" to make it clear that the input is the
problem. And let's also use the phrase "empty commit set
passed", which matches what we say when we do a real
revision walk and it turns up empty.

This BUG dates back to 7acaaac275 (revert: allow single-pick
in the middle of cherry-pick sequence, 2011-12-10), and
could be triggered in the same way even then. So clearly
this outcome is unexpected. Another approach would be to
make the conditional from 7acaaac275 smarter, and avoid even
entering this single-pick case.  But since the action is
identical either way (we have nothing to pick, so we exit)
there's not much point in trying to distinguish the two.

Signed-off-by: Jeff King <peff@peff.net>
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index f692b2ef44..234666b980 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3637,7 +3637,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 			return error(_("revision walk setup failed"));
 		cmit = get_revision(opts->revs);
 		if (!cmit || get_revision(opts->revs))
-			return error("BUG: expected exactly one commit from walk");
+			return error(_("empty commit set passed"));
 		return single_pick(cmit, opts);
 	}
 
-- 
2.18.0.400.g702e398724

Re: [PATCH 1/2] sequencer: handle empty-set cases consistently

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> If the user gives us a set that prepare_revision_walk()
> takes to be empty, like:
>
>   git cherry-pick base..base
>
> then we report an error. It's nonsense, and there's nothing
> to pick.
>
> But if they use revision options that later cull the list,
> like:
>
>   git cherry-pick --author=nobody base~2..base
>
> then we quietly create an empty todo list and return
> success.
>
> Arguably either behavior is acceptable, but we should
> definitely be consistent about it. Reporting an error
> seems to match the original intent, which dates all the way
> back to 7e2bfd3f99 (revert: allow cherry-picking more than
> one commit, 2010-06-02). That in turn was trying to match
> the single-commit case that exited before then (and which
> continues to issue an error).

Other than s/exited/existed/, the above looks perfect ;-)  I wish
all analysis part of proposed log messages were written like this.



>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  sequencer.c                     | 6 ++++--
>  t/t3510-cherry-pick-sequence.sh | 7 ++++++-
>  2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 5354d4d51e..f692b2ef44 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1863,8 +1863,6 @@ static int prepare_revs(struct replay_opts *opts)
>  	if (prepare_revision_walk(opts->revs))
>  		return error(_("revision walk setup failed"));
>  
> -	if (!opts->revs->commits)
> -		return error(_("empty commit set passed"));
>  	return 0;
>  }
>  
> @@ -2317,6 +2315,10 @@ static int walk_revs_populate_todo(struct todo_list *todo_list,
>  			short_commit_name(commit), subject_len, subject);
>  		unuse_commit_buffer(commit, commit_buffer);
>  	}
> +
> +	if (!todo_list->nr)
> +		return error(_("empty commit set passed"));
> +
>  	return 0;
>  }
>  
> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
> index b42cd66d3a..3505b6aa14 100755
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -480,11 +480,16 @@ test_expect_success 'malformed instruction sheet 2' '
>  	test_expect_code 128 git cherry-pick --continue
>  '
>  
> -test_expect_success 'empty commit set' '
> +test_expect_success 'empty commit set (no commits to walk)' '
>  	pristine_detach initial &&
>  	test_expect_code 128 git cherry-pick base..base
>  '
>  
> +test_expect_success 'empty commit set (culled during walk)' '
> +	pristine_detach initial &&
> +	test_expect_code 128 git cherry-pick -2 --author=no.such.author base
> +'
> +
>  test_expect_success 'malformed instruction sheet 3' '
>  	pristine_detach initial &&
>  	test_expect_code 1 git cherry-pick base..anotherpick &&

Re: [PATCH 1/2] sequencer: handle empty-set cases consistently

[Johannes Schindelin]

Hi Peff,

On Mon, 9 Jul 2018, Jeff King wrote:

> If the user gives us a set that prepare_revision_walk()
> takes to be empty, like:
> 
>   git cherry-pick base..base
> 
> then we report an error. It's nonsense, and there's nothing
> to pick.
> 
> But if they use revision options that later cull the list,
> like:
> 
>   git cherry-pick --author=nobody base~2..base
> 
> then we quietly create an empty todo list and return
> success.
> 
> Arguably either behavior is acceptable, but we should
> definitely be consistent about it. Reporting an error
> seems to match the original intent, which dates all the way
> back to 7e2bfd3f99 (revert: allow cherry-picking more than
> one commit, 2010-06-02). That in turn was trying to match
> the single-commit case that exited before then (and which
> continues to issue an error).
> 
> Signed-off-by: Jeff King <peff@peff.net>

Makes sense to me.

Thanks,
Dscho

Re: [PATCH 2/2] sequencer: don't say BUG on bogus input

[Johannes Schindelin]

Hi Peff,

On Mon, 9 Jul 2018, Jeff King wrote:

> diff --git a/sequencer.c b/sequencer.c
> index f692b2ef44..234666b980 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3637,7 +3637,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
>  			return error(_("revision walk setup failed"));
>  		cmit = get_revision(opts->revs);
>  		if (!cmit || get_revision(opts->revs))
> -			return error("BUG: expected exactly one commit from walk");
> +			return error(_("empty commit set passed"));

Should this not rather be

-		if (!cmit || get_revision(opts->revs))
-			return error("BUG: expected exactly one commit from walk");
+		if (!cmit)
+			return error(_("empty commit set passed"));
+		if (get_revision(opts->revs))
+			return error(_("unexpected extra commit from walk"));

Ciao,
Dscho

Re: [PATCH 2/2] sequencer: don't say BUG on bogus input

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Peff,
>
> On Mon, 9 Jul 2018, Jeff King wrote:
>
>> diff --git a/sequencer.c b/sequencer.c
>> index f692b2ef44..234666b980 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -3637,7 +3637,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
>>  			return error(_("revision walk setup failed"));
>>  		cmit = get_revision(opts->revs);
>>  		if (!cmit || get_revision(opts->revs))
>> -			return error("BUG: expected exactly one commit from walk");
>> +			return error(_("empty commit set passed"));
>
> Should this not rather be
>
> -		if (!cmit || get_revision(opts->revs))
> -			return error("BUG: expected exactly one commit from walk");
> +		if (!cmit)
> +			return error(_("empty commit set passed"));
> +		if (get_revision(opts->revs))
> +			return error(_("unexpected extra commit from walk"));

Good eyes.

Re: [PATCH 2/2] sequencer: don't say BUG on bogus input

[Jeff King]

On Mon, Jul 09, 2018 at 10:24:25PM +0200, Johannes Schindelin wrote:

> > diff --git a/sequencer.c b/sequencer.c
> > index f692b2ef44..234666b980 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -3637,7 +3637,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
> >  			return error(_("revision walk setup failed"));
> >  		cmit = get_revision(opts->revs);
> >  		if (!cmit || get_revision(opts->revs))
> > -			return error("BUG: expected exactly one commit from walk");
> > +			return error(_("empty commit set passed"));
> 
> Should this not rather be
> 
> -		if (!cmit || get_revision(opts->revs))
> -			return error("BUG: expected exactly one commit from walk");
> +		if (!cmit)
> +			return error(_("empty commit set passed"));
> +		if (get_revision(opts->revs))
> +			return error(_("unexpected extra commit from walk"));

Yeah, you're right. I'm not sure how a single rev with no-walk would
ever turn up more than one commit, though. So I think we should probably
go with:

  if (!cmit)
	return error(_("empty commit set passed"));
  if (get_revision(opts->revs))
	BUG("unexpected extra commit from walk");

And then if we ever see that case, we can decide from there what the
right action is (though _probably_ it's just to emit an error like you
have above, it might be a sign that our single-pick logic is wrong).

I'll re-roll in that direction, and discuss further in the commit
message.

-Peff

[PATCH v2 0/2] de-confuse git cherry-pick --author

[Jeff King]

On Mon, Jul 09, 2018 at 10:15:05PM -0400, Jeff King wrote:

> > Should this not rather be
> > 
> > -		if (!cmit || get_revision(opts->revs))
> > -			return error("BUG: expected exactly one commit from walk");
> > +		if (!cmit)
> > +			return error(_("empty commit set passed"));
> > +		if (get_revision(opts->revs))
> > +			return error(_("unexpected extra commit from walk"));
> 
> Yeah, you're right. I'm not sure how a single rev with no-walk would
> ever turn up more than one commit, though. So I think we should probably
> go with:
> 
>   if (!cmit)
> 	return error(_("empty commit set passed"));
>   if (get_revision(opts->revs))
> 	BUG("unexpected extra commit from walk");
> 
> And then if we ever see that case, we can decide from there what the
> right action is (though _probably_ it's just to emit an error like you
> have above, it might be a sign that our single-pick logic is wrong).
> 
> I'll re-roll in that direction, and discuss further in the commit
> message.

After poking at it a bit more, I've convinced myself that this is the
right thing, as options like "--branches" which expand into multiple
tips already push us into the other code path.

So here's a re-roll. The first one is identical except for the typo-fix
in the commit message.

  [1/2]: sequencer: handle empty-set cases consistently
  [2/2]: sequencer: don't say BUG on bogus input

 sequencer.c                     | 12 ++++++++----
 t/t3510-cherry-pick-sequence.sh |  7 ++++++-
 2 files changed, 14 insertions(+), 5 deletions(-)

-Peff

[PATCH v2 1/2] sequencer: handle empty-set cases consistently

[Jeff King]

If the user gives us a set that prepare_revision_walk()
takes to be empty, like:

  git cherry-pick base..base

then we report an error. It's nonsense, and there's nothing
to pick.

But if they use revision options that later cull the list,
like:

  git cherry-pick --author=nobody base~2..base

then we quietly create an empty todo list and return
success.

Arguably either behavior is acceptable, but we should
definitely be consistent about it. Reporting an error
seems to match the original intent, which dates all the way
back to 7e2bfd3f99 (revert: allow cherry-picking more than
one commit, 2010-06-02). That in turn was trying to match
the single-commit case that existed before then (and which
continues to issue an error).

Signed-off-by: Jeff King <peff@peff.net>
---
 sequencer.c                     | 6 ++++--
 t/t3510-cherry-pick-sequence.sh | 7 ++++++-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 5354d4d51e..f692b2ef44 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1863,8 +1863,6 @@ static int prepare_revs(struct replay_opts *opts)
 	if (prepare_revision_walk(opts->revs))
 		return error(_("revision walk setup failed"));
 
-	if (!opts->revs->commits)
-		return error(_("empty commit set passed"));
 	return 0;
 }
 
@@ -2317,6 +2315,10 @@ static int walk_revs_populate_todo(struct todo_list *todo_list,
 			short_commit_name(commit), subject_len, subject);
 		unuse_commit_buffer(commit, commit_buffer);
 	}
+
+	if (!todo_list->nr)
+		return error(_("empty commit set passed"));
+
 	return 0;
 }
 
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index b42cd66d3a..3505b6aa14 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -480,11 +480,16 @@ test_expect_success 'malformed instruction sheet 2' '
 	test_expect_code 128 git cherry-pick --continue
 '
 
-test_expect_success 'empty commit set' '
+test_expect_success 'empty commit set (no commits to walk)' '
 	pristine_detach initial &&
 	test_expect_code 128 git cherry-pick base..base
 '
 
+test_expect_success 'empty commit set (culled during walk)' '
+	pristine_detach initial &&
+	test_expect_code 128 git cherry-pick -2 --author=no.such.author base
+'
+
 test_expect_success 'malformed instruction sheet 3' '
 	pristine_detach initial &&
 	test_expect_code 1 git cherry-pick base..anotherpick &&
-- 
2.18.0.400.g702e398724

[PATCH v2 2/2] sequencer: don't say BUG on bogus input

[Jeff King]

When cherry-picking a single commit, we go through a special
code path that avoids creating a sequencer todo list at all.
This path expects our revision parsing to turn up exactly
one commit, and dies with a BUG if it doesn't.

But it's actually quite easy to fool. For example:

  $ git cherry-pick --author=no.such.person HEAD
  error: BUG: expected exactly one commit from walk
  fatal: cherry-pick failed

This isn't a bug; it's just bogus input.

The condition to trigger this message actually has two
parts:

  1. We saw no commits. That's the case in the example
     above. Let's drop the "BUG" here to make it clear that
     the input is the problem. And let's also use the phrase
     "empty commit set passed", which matches what we say
     when we do a real revision walk and it turns up empty.

  2. We saw more than one commit. That one _should_ be
     impossible to trigger, since we fed at most one tip and
     provided the no_walk option (and we'll have already
     expanded options like "--branches" that can turn into
     multiple tips). If this ever triggers, it's an
     indication that the conditional added by 7acaaac275
     (revert: allow single-pick in the middle of cherry-pick
     sequence, 2011-12-10) needs to more carefully define
     the single-pick case.

     So this can remain a bug, but we'll upgrade it to use
     the BUG() macro, which would make it easier to detect
     and analyze if it does trigger.

Signed-off-by: Jeff King <peff@peff.net>
---
 sequencer.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index f692b2ef44..8f0a015160 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3636,8 +3636,10 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 		if (prepare_revision_walk(opts->revs))
 			return error(_("revision walk setup failed"));
 		cmit = get_revision(opts->revs);
-		if (!cmit || get_revision(opts->revs))
-			return error("BUG: expected exactly one commit from walk");
+		if (!cmit)
+			return error(_("empty commit set passed"));
+		if (get_revision(opts->revs))
+			BUG("unexpected extra commit from walk");
 		return single_pick(cmit, opts);
 	}
 
-- 
2.18.0.400.g702e398724

Re: [PATCH v2 0/2] de-confuse git cherry-pick --author

[Johannes Schindelin]

Hi Peff,

On Tue, 10 Jul 2018, Jeff King wrote:

> On Mon, Jul 09, 2018 at 10:15:05PM -0400, Jeff King wrote:
> 
> > > Should this not rather be
> > > 
> > > -		if (!cmit || get_revision(opts->revs))
> > > -			return error("BUG: expected exactly one commit from walk");
> > > +		if (!cmit)
> > > +			return error(_("empty commit set passed"));
> > > +		if (get_revision(opts->revs))
> > > +			return error(_("unexpected extra commit from walk"));
> > 
> > Yeah, you're right. I'm not sure how a single rev with no-walk would
> > ever turn up more than one commit, though. So I think we should probably
> > go with:
> > 
> >   if (!cmit)
> > 	return error(_("empty commit set passed"));
> >   if (get_revision(opts->revs))
> > 	BUG("unexpected extra commit from walk");
> > 
> > And then if we ever see that case, we can decide from there what the
> > right action is (though _probably_ it's just to emit an error like you
> > have above, it might be a sign that our single-pick logic is wrong).
> > 
> > I'll re-roll in that direction, and discuss further in the commit
> > message.
> 
> After poking at it a bit more, I've convinced myself that this is the
> right thing, as options like "--branches" which expand into multiple
> tips already push us into the other code path.
> 
> So here's a re-roll. The first one is identical except for the typo-fix
> in the commit message.
> 
>   [1/2]: sequencer: handle empty-set cases consistently
>   [2/2]: sequencer: don't say BUG on bogus input
> 
>  sequencer.c                     | 12 ++++++++----
>  t/t3510-cherry-pick-sequence.sh |  7 ++++++-
>  2 files changed, 14 insertions(+), 5 deletions(-)

ACK,
Dscho

Re: [PATCH v2 0/2] de-confuse git cherry-pick --author

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> After poking at it a bit more, I've convinced myself that this is the
>> right thing, as options like "--branches" which expand into multiple
>> tips already push us into the other code path.
>> 
>> So here's a re-roll. The first one is identical except for the typo-fix
>> in the commit message.
>> 
>>   [1/2]: sequencer: handle empty-set cases consistently
>>   [2/2]: sequencer: don't say BUG on bogus input
>> 
>>  sequencer.c                     | 12 ++++++++----
>>  t/t3510-cherry-pick-sequence.sh |  7 ++++++-
>>  2 files changed, 14 insertions(+), 5 deletions(-)
>
> ACK,
> Dscho

Thanks, both.  Queued and pushed out.