git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [BUG] git cherry-pick does not complain about unknown options
@ 2018-07-09 14:16 Andrei Rybak
  2018-07-09 19:16 ` Jeff King
  0 siblings, 1 reply; 15+ messages in thread
From: Andrei Rybak @ 2018-07-09 14:16 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [BUG] git cherry-pick does not complain about unknown options
  2018-07-09 14:16 [BUG] git cherry-pick does not complain about unknown options Andrei Rybak
@ 2018-07-09 19:16 ` Jeff King
  2018-07-09 19:46   ` [PATCH 0/2] de-confuse git cherry-pick --author behavior Jeff King
  0 siblings, 1 reply; 15+ messages in thread
From: Jeff King @ 2018-07-09 19:16 UTC (permalink / raw)
  To: Andrei Rybak; +Cc: git, Johannes Schindelin

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 0/2] de-confuse git cherry-pick --author behavior
  2018-07-09 19:16 ` Jeff King
@ 2018-07-09 19:46   ` Jeff King
  2018-07-09 19:48     ` [PATCH 1/2] sequencer: handle empty-set cases consistently Jeff King
  2018-07-09 19:49     ` [PATCH 2/2] sequencer: don't say BUG on bogus input Jeff King
  0 siblings, 2 replies; 15+ messages in thread
From: Jeff King @ 2018-07-09 19:46 UTC (permalink / raw)
  To: Andrei Rybak; +Cc: git, Johannes Schindelin, Christian Couder, Jonathan Nieder

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/2] sequencer: handle empty-set cases consistently
  2018-07-09 19:46   ` [PATCH 0/2] de-confuse git cherry-pick --author behavior Jeff King
@ 2018-07-09 19:48     ` Jeff King
  2018-07-09 20:20       ` Junio C Hamano
  2018-07-09 20:21       ` Johannes Schindelin
  2018-07-09 19:49     ` [PATCH 2/2] sequencer: don't say BUG on bogus input Jeff King
  1 sibling, 2 replies; 15+ messages in thread
From: Jeff King @ 2018-07-09 19:48 UTC (permalink / raw)
  To: Andrei Rybak; +Cc: git, Johannes Schindelin, Christian Couder, Jonathan Nieder

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 2/2] sequencer: don't say BUG on bogus input
  2018-07-09 19:46   ` [PATCH 0/2] de-confuse git cherry-pick --author behavior Jeff King
  2018-07-09 19:48     ` [PATCH 1/2] sequencer: handle empty-set cases consistently Jeff King
@ 2018-07-09 19:49     ` Jeff King
  2018-07-09 20:24       ` Johannes Schindelin
  1 sibling, 1 reply; 15+ messages in thread
From: Jeff King @ 2018-07-09 19:49 UTC (permalink / raw)
  To: Andrei Rybak; +Cc: git, Johannes Schindelin, Christian Couder, Jonathan Nieder

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] sequencer: handle empty-set cases consistently
  2018-07-09 19:48     ` [PATCH 1/2] sequencer: handle empty-set cases consistently Jeff King
@ 2018-07-09 20:20       ` Junio C Hamano
  2018-07-09 20:21       ` Johannes Schindelin
  1 sibling, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2018-07-09 20:20 UTC (permalink / raw)
  To: Jeff King
  Cc: Andrei Rybak, git, Johannes Schindelin, Christian Couder,
	Jonathan Nieder

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] sequencer: handle empty-set cases consistently
  2018-07-09 19:48     ` [PATCH 1/2] sequencer: handle empty-set cases consistently Jeff King
  2018-07-09 20:20       ` Junio C Hamano
@ 2018-07-09 20:21       ` Johannes Schindelin
  1 sibling, 0 replies; 15+ messages in thread
From: Johannes Schindelin @ 2018-07-09 20:21 UTC (permalink / raw)
  To: Jeff King; +Cc: Andrei Rybak, git, Christian Couder, Jonathan Nieder

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] sequencer: don't say BUG on bogus input
  2018-07-09 19:49     ` [PATCH 2/2] sequencer: don't say BUG on bogus input Jeff King
@ 2018-07-09 20:24       ` Johannes Schindelin
  2018-07-09 21:11         ` Junio C Hamano
  2018-07-10  2:15         ` Jeff King
  0 siblings, 2 replies; 15+ messages in thread
From: Johannes Schindelin @ 2018-07-09 20:24 UTC (permalink / raw)
  To: Jeff King; +Cc: Andrei Rybak, git, Christian Couder, Jonathan Nieder

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] sequencer: don't say BUG on bogus input
  2018-07-09 20:24       ` Johannes Schindelin
@ 2018-07-09 21:11         ` Junio C Hamano
  2018-07-10  2:15         ` Jeff King
  1 sibling, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2018-07-09 21:11 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Jeff King, Andrei Rybak, git, Christian Couder, Jonathan Nieder

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] sequencer: don't say BUG on bogus input
  2018-07-09 20:24       ` Johannes Schindelin
  2018-07-09 21:11         ` Junio C Hamano
@ 2018-07-10  2:15         ` Jeff King
  2018-07-10  4:31           ` [PATCH v2 0/2] de-confuse git cherry-pick --author Jeff King
  1 sibling, 1 reply; 15+ messages in thread
From: Jeff King @ 2018-07-10  2:15 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Andrei Rybak, git, Christian Couder, Jonathan Nieder

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 0/2] de-confuse git cherry-pick --author
  2018-07-10  2:15         ` Jeff King
@ 2018-07-10  4:31           ` Jeff King
  2018-07-10  4:31             ` [PATCH v2 1/2] sequencer: handle empty-set cases consistently Jeff King
                               ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Jeff King @ 2018-07-10  4:31 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Andrei Rybak, git, Christian Couder, Jonathan Nieder

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 1/2] sequencer: handle empty-set cases consistently
  2018-07-10  4:31           ` [PATCH v2 0/2] de-confuse git cherry-pick --author Jeff King
@ 2018-07-10  4:31             ` Jeff King
  2018-07-10  4:32             ` [PATCH v2 2/2] sequencer: don't say BUG on bogus input Jeff King
  2018-07-11  8:58             ` [PATCH v2 0/2] de-confuse git cherry-pick --author Johannes Schindelin
  2 siblings, 0 replies; 15+ messages in thread
From: Jeff King @ 2018-07-10  4:31 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Andrei Rybak, git, Christian Couder, Jonathan Nieder

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 2/2] sequencer: don't say BUG on bogus input
  2018-07-10  4:31           ` [PATCH v2 0/2] de-confuse git cherry-pick --author Jeff King
  2018-07-10  4:31             ` [PATCH v2 1/2] sequencer: handle empty-set cases consistently Jeff King
@ 2018-07-10  4:32             ` Jeff King
  2018-07-11  8:58             ` [PATCH v2 0/2] de-confuse git cherry-pick --author Johannes Schindelin
  2 siblings, 0 replies; 15+ messages in thread
From: Jeff King @ 2018-07-10  4:32 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Andrei Rybak, git, Christian Couder, Jonathan Nieder

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 0/2] de-confuse git cherry-pick --author
  2018-07-10  4:31           ` [PATCH v2 0/2] de-confuse git cherry-pick --author Jeff King
  2018-07-10  4:31             ` [PATCH v2 1/2] sequencer: handle empty-set cases consistently Jeff King
  2018-07-10  4:32             ` [PATCH v2 2/2] sequencer: don't say BUG on bogus input Jeff King
@ 2018-07-11  8:58             ` Johannes Schindelin
  2018-07-11 15:37               ` Junio C Hamano
  2 siblings, 1 reply; 15+ messages in thread
From: Johannes Schindelin @ 2018-07-11  8:58 UTC (permalink / raw)
  To: Jeff King; +Cc: Andrei Rybak, git, Christian Couder, Jonathan Nieder

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 0/2] de-confuse git cherry-pick --author
  2018-07-11  8:58             ` [PATCH v2 0/2] de-confuse git cherry-pick --author Johannes Schindelin
@ 2018-07-11 15:37               ` Junio C Hamano
  0 siblings, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2018-07-11 15:37 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Jeff King, Andrei Rybak, git, Christian Couder, Jonathan Nieder

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, back to index

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-09 14:16 [BUG] git cherry-pick does not complain about unknown options Andrei Rybak
2018-07-09 19:16 ` Jeff King
2018-07-09 19:46   ` [PATCH 0/2] de-confuse git cherry-pick --author behavior Jeff King
2018-07-09 19:48     ` [PATCH 1/2] sequencer: handle empty-set cases consistently Jeff King
2018-07-09 20:20       ` Junio C Hamano
2018-07-09 20:21       ` Johannes Schindelin
2018-07-09 19:49     ` [PATCH 2/2] sequencer: don't say BUG on bogus input Jeff King
2018-07-09 20:24       ` Johannes Schindelin
2018-07-09 21:11         ` Junio C Hamano
2018-07-10  2:15         ` Jeff King
2018-07-10  4:31           ` [PATCH v2 0/2] de-confuse git cherry-pick --author Jeff King
2018-07-10  4:31             ` [PATCH v2 1/2] sequencer: handle empty-set cases consistently Jeff King
2018-07-10  4:32             ` [PATCH v2 2/2] sequencer: don't say BUG on bogus input Jeff King
2018-07-11  8:58             ` [PATCH v2 0/2] de-confuse git cherry-pick --author Johannes Schindelin
2018-07-11 15:37               ` Junio C Hamano

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox