{"thread":{"id":"48846","subject":"[BUG] git cherry-pick does not complain about unknown options","startedAt":"2018-07-09T14:16:23Z","lastAt":"2018-07-11T15:37:09Z","messageCount":15,"participants":["Andrei Rybak","Jeff King","Junio C Hamano","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"351944","messageId":"33d3e45b-1245-9e2f-42ea-a654a593bf10@gmail.com","threadId":"48846","inReplyTo":null,"subject":"[BUG] git cherry-pick does not complain about unknown options","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2018-07-09T14:16:16Z","receivedAt":"2018-07-09T14:16:23Z","isPatch":false,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Hi,\n\nI was trying to cherry pick commits, while simultaneously changing the\nauthor.  Unfortunately, cherry-pick doesn't have the same --author\noption as git-commit.  However, instead of complaining about unknown\noption:\n\n- when trying to cherry-pick one commit, it reported a BUG\n- when trying to cherry-pick several commits, cherry-pick silently\n  did nothing\n\nAll commits in tests existed in repository:\n\n    $ git cherry-pick --author='TEST' <one-commit> # case 1\n    error: BUG: expected exactly one commit from walk\n    fatal: cherry-pick failed\n    $ echo $?\n    128\n    $ git cherry-pick --author='TEST' <commit1> <commit2>  # case 2\n    $ echo $?\n    0\n    $ git --version\n    git version 2.18.0.windows.1\n\nI've encountered this issue in Windows version, and  Johannes Schindelin\nhas confirmed that the issue is also present in Linux version.\n\nOriginally reported here: https://github.com/git-for-windows/git/issues/1751\n\n--\nBest regards, Andrei Rybak\n"},{"id":"351983","messageId":"20180709191607.GA9852@sigill.intra.peff.net","threadId":"48846","inReplyTo":"33d3e45b-1245-9e2f-42ea-a654a593bf10@gmail.com","subject":"Re: [BUG] git cherry-pick does not complain about unknown options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-09T19:16:07Z","receivedAt":"2018-07-09T19:16:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 09, 2018 at 04:16:16PM +0200, Andrei Rybak wrote:\n\n> I was trying to cherry pick commits, while simultaneously changing the\n> author.  Unfortunately, cherry-pick doesn't have the same --author\n> option as git-commit.  However, instead of complaining about unknown\n> option:\n\nYeah, its \"--author\" option is interpreted by revision.c similar to \"log\n--author\" (because we just pass all unknown options to the traversal\nmachinery). So you could say:\n\n  git cherry-pick --author=peff v1.0..v2.0\n\nto pick all of my commits between the two versions. I'm not sure if that\nwould ever be _useful_, but that's how it has behaved since 7e2bfd3f99\n(revert: allow cherry-picking more than one commit, 2010-06-02), I\nthink.\n\nI agree that having something similar to commit's \"--author\" (or even\njust \"--reset-author\") would be useful. When I've had to do this before,\nI usually just cherry-pick and then follow-up with \"commit --amend\n--author\" (or use \"rebase -i\" if there are several commits). You can\nalso do \"cherry-pick -n $commit\" followed by \"commit -c $commit\".\n\n> All commits in tests existed in repository:\n> \n>     $ git cherry-pick --author='TEST' <one-commit> # case 1\n>     error: BUG: expected exactly one commit from walk\n>     fatal: cherry-pick failed\n>     $ echo $?\n>     128\n\nI think it's reasonable for this to issue an error. But a BUG is\ndefinitely wrong. The error is more like \"empty commit set passed\",\nwhich is what we get for something like:\n\n  $ git cherry-pick HEAD..HEAD\n  error: empty commit set passed\n  fatal: cherry-pick failed\n\nthat triggers the revision walker but has no positive commits. But we\ndon't trigger that code because the single-rev case has some magic to\nallow a cherry-pick during another sequencer operation.\n\nSo I think that BUG message needs to be softened.\n\n>     $ git cherry-pick --author='TEST' <commit1> <commit2>  # case 2\n>     $ echo $?\n>     0\n\nAnd here we come up with an empty set, but we don't flag it as an error\nat all. I could even accept that philosophically as \"empty input,\nnothing to do\", but we clearly do flag it as an error in the case I\nshowed above.\n\nI think the problem is that we catch the error in\nsequencer.c:prepare_revs(), after prepare_revision_walk() returns no\ncommits. But we _do_ have a starting commit for this case, and it's not\nculled until we actually ask get_revisions() to start walking.\n\nI think we'd want something like this:\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 4034c0461b..b978157399 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1860,8 +1860,6 @@ static int prepare_revs(struct replay_opts *opts)\n \tif (prepare_revision_walk(opts->revs))\n \t\treturn error(_(\"revision walk setup failed\"));\n \n-\tif (!opts->revs->commits)\n-\t\treturn error(_(\"empty commit set passed\"));\n \treturn 0;\n }\n \n@@ -2314,6 +2312,10 @@ static int walk_revs_populate_todo(struct todo_list *todo_list,\n \t\t\tshort_commit_name(commit), subject_len, subject);\n \t\tunuse_commit_buffer(commit, commit_buffer);\n \t}\n+\n+\tif (!todo_list->nr)\n+\t\treturn error(_(\"empty commit set passed\"));\n+\n \treturn 0;\n }\n \n\n-Peff\n"},{"id":"351996","messageId":"20180709194636.GB9852@sigill.intra.peff.net","threadId":"48846","inReplyTo":"20180709191607.GA9852@sigill.intra.peff.net","subject":"[PATCH 0/2] de-confuse git cherry-pick --author behavior","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-09T19:46:36Z","receivedAt":"2018-07-09T19:46:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 09, 2018 at 03:16:07PM -0400, Jeff King wrote:\n\n> I agree that having something similar to commit's \"--author\" (or even\n> just \"--reset-author\") would be useful. When I've had to do this before,\n> I usually just cherry-pick and then follow-up with \"commit --amend\n> --author\" (or use \"rebase -i\" if there are several commits). You can\n> also do \"cherry-pick -n $commit\" followed by \"commit -c $commit\".\n\nI'll leave adding any options there as an exercise if somebody is\ninterested. This series just focuses on the inconsistent error behavior\nyou found.\n\n  [1/2]: sequencer: handle empty-set cases consistently\n  [2/2]: sequencer: don't say BUG on bogus input\n\n sequencer.c                     | 8 +++++---\n t/t3510-cherry-pick-sequence.sh | 7 ++++++-\n 2 files changed, 11 insertions(+), 4 deletions(-)\n\n-Peff\n"},{"id":"351997","messageId":"20180709194819.GA7980@sigill.intra.peff.net","threadId":"48846","inReplyTo":"20180709194636.GB9852@sigill.intra.peff.net","subject":"[PATCH 1/2] sequencer: handle empty-set cases consistently","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-09T19:48:19Z","receivedAt":"2018-07-09T19:48:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If the user gives us a set that prepare_revision_walk()\ntakes to be empty, like:\n\n  git cherry-pick base..base\n\nthen we report an error. It's nonsense, and there's nothing\nto pick.\n\nBut if they use revision options that later cull the list,\nlike:\n\n  git cherry-pick --author=nobody base~2..base\n\nthen we quietly create an empty todo list and return\nsuccess.\n\nArguably either behavior is acceptable, but we should\ndefinitely be consistent about it. Reporting an error\nseems to match the original intent, which dates all the way\nback to 7e2bfd3f99 (revert: allow cherry-picking more than\none commit, 2010-06-02). That in turn was trying to match\nthe single-commit case that exited before then (and which\ncontinues to issue an error).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sequencer.c                     | 6 ++++--\n t/t3510-cherry-pick-sequence.sh | 7 ++++++-\n 2 files changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 5354d4d51e..f692b2ef44 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1863,8 +1863,6 @@ static int prepare_revs(struct replay_opts *opts)\n \tif (prepare_revision_walk(opts->revs))\n \t\treturn error(_(\"revision walk setup failed\"));\n \n-\tif (!opts->revs->commits)\n-\t\treturn error(_(\"empty commit set passed\"));\n \treturn 0;\n }\n \n@@ -2317,6 +2315,10 @@ static int walk_revs_populate_todo(struct todo_list *todo_list,\n \t\t\tshort_commit_name(commit), subject_len, subject);\n \t\tunuse_commit_buffer(commit, commit_buffer);\n \t}\n+\n+\tif (!todo_list->nr)\n+\t\treturn error(_(\"empty commit set passed\"));\n+\n \treturn 0;\n }\n \ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex b42cd66d3a..3505b6aa14 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -480,11 +480,16 @@ test_expect_success 'malformed instruction sheet 2' '\n \ttest_expect_code 128 git cherry-pick --continue\n '\n \n-test_expect_success 'empty commit set' '\n+test_expect_success 'empty commit set (no commits to walk)' '\n \tpristine_detach initial &&\n \ttest_expect_code 128 git cherry-pick base..base\n '\n \n+test_expect_success 'empty commit set (culled during walk)' '\n+\tpristine_detach initial &&\n+\ttest_expect_code 128 git cherry-pick -2 --author=no.such.author base\n+'\n+\n test_expect_success 'malformed instruction sheet 3' '\n \tpristine_detach initial &&\n \ttest_expect_code 1 git cherry-pick base..anotherpick &&\n-- \n2.18.0.400.g702e398724\n\n"},{"id":"351998","messageId":"20180709194912.GB7980@sigill.intra.peff.net","threadId":"48846","inReplyTo":"20180709194636.GB9852@sigill.intra.peff.net","subject":"[PATCH 2/2] sequencer: don't say BUG on bogus input","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-09T19:49:12Z","receivedAt":"2018-07-09T19:49:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When cherry-picking a single commit, we go through a special\ncode path that avoids creating a sequencer todo list at all.\nThis path expects our revision parsing to turn up exactly\none commit, and dies with a BUG if it doesn't.\n\nBut it's actually quite easy to fool. For example:\n\n  $ git cherry-pick --author=no.such.person HEAD\n  error: BUG: expected exactly one commit from walk\n  fatal: cherry-pick failed\n\nThis isn't a bug; it's just bogus input.\n\nLet's drop the \"BUG\" to make it clear that the input is the\nproblem. And let's also use the phrase \"empty commit set\npassed\", which matches what we say when we do a real\nrevision walk and it turns up empty.\n\nThis BUG dates back to 7acaaac275 (revert: allow single-pick\nin the middle of cherry-pick sequence, 2011-12-10), and\ncould be triggered in the same way even then. So clearly\nthis outcome is unexpected. Another approach would be to\nmake the conditional from 7acaaac275 smarter, and avoid even\nentering this single-pick case.  But since the action is\nidentical either way (we have nothing to pick, so we exit)\nthere's not much point in trying to distinguish the two.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sequencer.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex f692b2ef44..234666b980 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3637,7 +3637,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \t\t\treturn error(_(\"revision walk setup failed\"));\n \t\tcmit = get_revision(opts->revs);\n \t\tif (!cmit || get_revision(opts->revs))\n-\t\t\treturn error(\"BUG: expected exactly one commit from walk\");\n+\t\t\treturn error(_(\"empty commit set passed\"));\n \t\treturn single_pick(cmit, opts);\n \t}\n \n-- \n2.18.0.400.g702e398724\n"},{"id":"352005","messageId":"xmqqlgakcga6.fsf@gitster-ct.c.googlers.com","threadId":"48846","inReplyTo":"20180709194819.GA7980@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] sequencer: handle empty-set cases consistently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-09T20:20:33Z","receivedAt":"2018-07-09T20:20:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> If the user gives us a set that prepare_revision_walk()\n> takes to be empty, like:\n>\n>   git cherry-pick base..base\n>\n> then we report an error. It's nonsense, and there's nothing\n> to pick.\n>\n> But if they use revision options that later cull the list,\n> like:\n>\n>   git cherry-pick --author=nobody base~2..base\n>\n> then we quietly create an empty todo list and return\n> success.\n>\n> Arguably either behavior is acceptable, but we should\n> definitely be consistent about it. Reporting an error\n> seems to match the original intent, which dates all the way\n> back to 7e2bfd3f99 (revert: allow cherry-picking more than\n> one commit, 2010-06-02). That in turn was trying to match\n> the single-commit case that exited before then (and which\n> continues to issue an error).\n\nOther than s/exited/existed/, the above looks perfect ;-)  I wish\nall analysis part of proposed log messages were written like this.\n\n\n\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  sequencer.c                     | 6 ++++--\n>  t/t3510-cherry-pick-sequence.sh | 7 ++++++-\n>  2 files changed, 10 insertions(+), 3 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 5354d4d51e..f692b2ef44 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -1863,8 +1863,6 @@ static int prepare_revs(struct replay_opts *opts)\n>  \tif (prepare_revision_walk(opts->revs))\n>  \t\treturn error(_(\"revision walk setup failed\"));\n>  \n> -\tif (!opts->revs->commits)\n> -\t\treturn error(_(\"empty commit set passed\"));\n>  \treturn 0;\n>  }\n>  \n> @@ -2317,6 +2315,10 @@ static int walk_revs_populate_todo(struct todo_list *todo_list,\n>  \t\t\tshort_commit_name(commit), subject_len, subject);\n>  \t\tunuse_commit_buffer(commit, commit_buffer);\n>  \t}\n> +\n> +\tif (!todo_list->nr)\n> +\t\treturn error(_(\"empty commit set passed\"));\n> +\n>  \treturn 0;\n>  }\n>  \n> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\n> index b42cd66d3a..3505b6aa14 100755\n> --- a/t/t3510-cherry-pick-sequence.sh\n> +++ b/t/t3510-cherry-pick-sequence.sh\n> @@ -480,11 +480,16 @@ test_expect_success 'malformed instruction sheet 2' '\n>  \ttest_expect_code 128 git cherry-pick --continue\n>  '\n>  \n> -test_expect_success 'empty commit set' '\n> +test_expect_success 'empty commit set (no commits to walk)' '\n>  \tpristine_detach initial &&\n>  \ttest_expect_code 128 git cherry-pick base..base\n>  '\n>  \n> +test_expect_success 'empty commit set (culled during walk)' '\n> +\tpristine_detach initial &&\n> +\ttest_expect_code 128 git cherry-pick -2 --author=no.such.author base\n> +'\n> +\n>  test_expect_success 'malformed instruction sheet 3' '\n>  \tpristine_detach initial &&\n>  \ttest_expect_code 1 git cherry-pick base..anotherpick &&\n"},{"id":"352006","messageId":"nycvar.QRO.7.76.6.1807092220460.75@tvgsbejvaqbjf.bet","threadId":"48846","inReplyTo":"20180709194819.GA7980@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] sequencer: handle empty-set cases consistently","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-09T20:21:04Z","receivedAt":"2018-07-09T20:21:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Mon, 9 Jul 2018, Jeff King wrote:\n\n> If the user gives us a set that prepare_revision_walk()\n> takes to be empty, like:\n> \n>   git cherry-pick base..base\n> \n> then we report an error. It's nonsense, and there's nothing\n> to pick.\n> \n> But if they use revision options that later cull the list,\n> like:\n> \n>   git cherry-pick --author=nobody base~2..base\n> \n> then we quietly create an empty todo list and return\n> success.\n> \n> Arguably either behavior is acceptable, but we should\n> definitely be consistent about it. Reporting an error\n> seems to match the original intent, which dates all the way\n> back to 7e2bfd3f99 (revert: allow cherry-picking more than\n> one commit, 2010-06-02). That in turn was trying to match\n> the single-commit case that exited before then (and which\n> continues to issue an error).\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n\nMakes sense to me.\n\nThanks,\nDscho\n"},{"id":"352008","messageId":"nycvar.QRO.7.76.6.1807092222300.75@tvgsbejvaqbjf.bet","threadId":"48846","inReplyTo":"20180709194912.GB7980@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] sequencer: don't say BUG on bogus input","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-09T20:24:25Z","receivedAt":"2018-07-09T20:24:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Mon, 9 Jul 2018, Jeff King wrote:\n\n> diff --git a/sequencer.c b/sequencer.c\n> index f692b2ef44..234666b980 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3637,7 +3637,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n>  \t\t\treturn error(_(\"revision walk setup failed\"));\n>  \t\tcmit = get_revision(opts->revs);\n>  \t\tif (!cmit || get_revision(opts->revs))\n> -\t\t\treturn error(\"BUG: expected exactly one commit from walk\");\n> +\t\t\treturn error(_(\"empty commit set passed\"));\n\nShould this not rather be\n\n-\t\tif (!cmit || get_revision(opts->revs))\n-\t\t\treturn error(\"BUG: expected exactly one commit from walk\");\n+\t\tif (!cmit)\n+\t\t\treturn error(_(\"empty commit set passed\"));\n+\t\tif (get_revision(opts->revs))\n+\t\t\treturn error(_(\"unexpected extra commit from walk\"));\n\nCiao,\nDscho\n"},{"id":"352028","messageId":"xmqqsh4sazct.fsf@gitster-ct.c.googlers.com","threadId":"48846","inReplyTo":"nycvar.QRO.7.76.6.1807092222300.75@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/2] sequencer: don't say BUG on bogus input","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-09T21:11:30Z","receivedAt":"2018-07-09T21:11:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Hi Peff,\n>\n> On Mon, 9 Jul 2018, Jeff King wrote:\n>\n>> diff --git a/sequencer.c b/sequencer.c\n>> index f692b2ef44..234666b980 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -3637,7 +3637,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n>>  \t\t\treturn error(_(\"revision walk setup failed\"));\n>>  \t\tcmit = get_revision(opts->revs);\n>>  \t\tif (!cmit || get_revision(opts->revs))\n>> -\t\t\treturn error(\"BUG: expected exactly one commit from walk\");\n>> +\t\t\treturn error(_(\"empty commit set passed\"));\n>\n> Should this not rather be\n>\n> -\t\tif (!cmit || get_revision(opts->revs))\n> -\t\t\treturn error(\"BUG: expected exactly one commit from walk\");\n> +\t\tif (!cmit)\n> +\t\t\treturn error(_(\"empty commit set passed\"));\n> +\t\tif (get_revision(opts->revs))\n> +\t\t\treturn error(_(\"unexpected extra commit from walk\"));\n\nGood eyes.\n"},{"id":"352056","messageId":"20180710021505.GA10248@sigill.intra.peff.net","threadId":"48846","inReplyTo":"nycvar.QRO.7.76.6.1807092222300.75@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/2] sequencer: don't say BUG on bogus input","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-10T02:15:05Z","receivedAt":"2018-07-10T02:15:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 09, 2018 at 10:24:25PM +0200, Johannes Schindelin wrote:\n\n> > diff --git a/sequencer.c b/sequencer.c\n> > index f692b2ef44..234666b980 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -3637,7 +3637,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n> >  \t\t\treturn error(_(\"revision walk setup failed\"));\n> >  \t\tcmit = get_revision(opts->revs);\n> >  \t\tif (!cmit || get_revision(opts->revs))\n> > -\t\t\treturn error(\"BUG: expected exactly one commit from walk\");\n> > +\t\t\treturn error(_(\"empty commit set passed\"));\n> \n> Should this not rather be\n> \n> -\t\tif (!cmit || get_revision(opts->revs))\n> -\t\t\treturn error(\"BUG: expected exactly one commit from walk\");\n> +\t\tif (!cmit)\n> +\t\t\treturn error(_(\"empty commit set passed\"));\n> +\t\tif (get_revision(opts->revs))\n> +\t\t\treturn error(_(\"unexpected extra commit from walk\"));\n\nYeah, you're right. I'm not sure how a single rev with no-walk would\never turn up more than one commit, though. So I think we should probably\ngo with:\n\n  if (!cmit)\n\treturn error(_(\"empty commit set passed\"));\n  if (get_revision(opts->revs))\n\tBUG(\"unexpected extra commit from walk\");\n\nAnd then if we ever see that case, we can decide from there what the\nright action is (though _probably_ it's just to emit an error like you\nhave above, it might be a sign that our single-pick logic is wrong).\n\nI'll re-roll in that direction, and discuss further in the commit\nmessage.\n\n-Peff\n"},{"id":"352060","messageId":"20180710043120.GA1330@sigill.intra.peff.net","threadId":"48846","inReplyTo":"20180710021505.GA10248@sigill.intra.peff.net","subject":"[PATCH v2 0/2] de-confuse git cherry-pick --author","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-10T04:31:20Z","receivedAt":"2018-07-10T04:31:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 09, 2018 at 10:15:05PM -0400, Jeff King wrote:\n\n> > Should this not rather be\n> > \n> > -\t\tif (!cmit || get_revision(opts->revs))\n> > -\t\t\treturn error(\"BUG: expected exactly one commit from walk\");\n> > +\t\tif (!cmit)\n> > +\t\t\treturn error(_(\"empty commit set passed\"));\n> > +\t\tif (get_revision(opts->revs))\n> > +\t\t\treturn error(_(\"unexpected extra commit from walk\"));\n> \n> Yeah, you're right. I'm not sure how a single rev with no-walk would\n> ever turn up more than one commit, though. So I think we should probably\n> go with:\n> \n>   if (!cmit)\n> \treturn error(_(\"empty commit set passed\"));\n>   if (get_revision(opts->revs))\n> \tBUG(\"unexpected extra commit from walk\");\n> \n> And then if we ever see that case, we can decide from there what the\n> right action is (though _probably_ it's just to emit an error like you\n> have above, it might be a sign that our single-pick logic is wrong).\n> \n> I'll re-roll in that direction, and discuss further in the commit\n> message.\n\nAfter poking at it a bit more, I've convinced myself that this is the\nright thing, as options like \"--branches\" which expand into multiple\ntips already push us into the other code path.\n\nSo here's a re-roll. The first one is identical except for the typo-fix\nin the commit message.\n\n  [1/2]: sequencer: handle empty-set cases consistently\n  [2/2]: sequencer: don't say BUG on bogus input\n\n sequencer.c                     | 12 ++++++++----\n t/t3510-cherry-pick-sequence.sh |  7 ++++++-\n 2 files changed, 14 insertions(+), 5 deletions(-)\n\n-Peff\n"},{"id":"352061","messageId":"20180710043142.GA1909@sigill.intra.peff.net","threadId":"48846","inReplyTo":"20180710043120.GA1330@sigill.intra.peff.net","subject":"[PATCH v2 1/2] sequencer: handle empty-set cases consistently","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-10T04:31:42Z","receivedAt":"2018-07-10T04:31:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If the user gives us a set that prepare_revision_walk()\ntakes to be empty, like:\n\n  git cherry-pick base..base\n\nthen we report an error. It's nonsense, and there's nothing\nto pick.\n\nBut if they use revision options that later cull the list,\nlike:\n\n  git cherry-pick --author=nobody base~2..base\n\nthen we quietly create an empty todo list and return\nsuccess.\n\nArguably either behavior is acceptable, but we should\ndefinitely be consistent about it. Reporting an error\nseems to match the original intent, which dates all the way\nback to 7e2bfd3f99 (revert: allow cherry-picking more than\none commit, 2010-06-02). That in turn was trying to match\nthe single-commit case that existed before then (and which\ncontinues to issue an error).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sequencer.c                     | 6 ++++--\n t/t3510-cherry-pick-sequence.sh | 7 ++++++-\n 2 files changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 5354d4d51e..f692b2ef44 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1863,8 +1863,6 @@ static int prepare_revs(struct replay_opts *opts)\n \tif (prepare_revision_walk(opts->revs))\n \t\treturn error(_(\"revision walk setup failed\"));\n \n-\tif (!opts->revs->commits)\n-\t\treturn error(_(\"empty commit set passed\"));\n \treturn 0;\n }\n \n@@ -2317,6 +2315,10 @@ static int walk_revs_populate_todo(struct todo_list *todo_list,\n \t\t\tshort_commit_name(commit), subject_len, subject);\n \t\tunuse_commit_buffer(commit, commit_buffer);\n \t}\n+\n+\tif (!todo_list->nr)\n+\t\treturn error(_(\"empty commit set passed\"));\n+\n \treturn 0;\n }\n \ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex b42cd66d3a..3505b6aa14 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -480,11 +480,16 @@ test_expect_success 'malformed instruction sheet 2' '\n \ttest_expect_code 128 git cherry-pick --continue\n '\n \n-test_expect_success 'empty commit set' '\n+test_expect_success 'empty commit set (no commits to walk)' '\n \tpristine_detach initial &&\n \ttest_expect_code 128 git cherry-pick base..base\n '\n \n+test_expect_success 'empty commit set (culled during walk)' '\n+\tpristine_detach initial &&\n+\ttest_expect_code 128 git cherry-pick -2 --author=no.such.author base\n+'\n+\n test_expect_success 'malformed instruction sheet 3' '\n \tpristine_detach initial &&\n \ttest_expect_code 1 git cherry-pick base..anotherpick &&\n-- \n2.18.0.400.g702e398724\n\n"},{"id":"352062","messageId":"20180710043207.GB1909@sigill.intra.peff.net","threadId":"48846","inReplyTo":"20180710043120.GA1330@sigill.intra.peff.net","subject":"[PATCH v2 2/2] sequencer: don't say BUG on bogus input","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-10T04:32:08Z","receivedAt":"2018-07-10T04:32:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When cherry-picking a single commit, we go through a special\ncode path that avoids creating a sequencer todo list at all.\nThis path expects our revision parsing to turn up exactly\none commit, and dies with a BUG if it doesn't.\n\nBut it's actually quite easy to fool. For example:\n\n  $ git cherry-pick --author=no.such.person HEAD\n  error: BUG: expected exactly one commit from walk\n  fatal: cherry-pick failed\n\nThis isn't a bug; it's just bogus input.\n\nThe condition to trigger this message actually has two\nparts:\n\n  1. We saw no commits. That's the case in the example\n     above. Let's drop the \"BUG\" here to make it clear that\n     the input is the problem. And let's also use the phrase\n     \"empty commit set passed\", which matches what we say\n     when we do a real revision walk and it turns up empty.\n\n  2. We saw more than one commit. That one _should_ be\n     impossible to trigger, since we fed at most one tip and\n     provided the no_walk option (and we'll have already\n     expanded options like \"--branches\" that can turn into\n     multiple tips). If this ever triggers, it's an\n     indication that the conditional added by 7acaaac275\n     (revert: allow single-pick in the middle of cherry-pick\n     sequence, 2011-12-10) needs to more carefully define\n     the single-pick case.\n\n     So this can remain a bug, but we'll upgrade it to use\n     the BUG() macro, which would make it easier to detect\n     and analyze if it does trigger.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sequencer.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex f692b2ef44..8f0a015160 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3636,8 +3636,10 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \t\tif (prepare_revision_walk(opts->revs))\n \t\t\treturn error(_(\"revision walk setup failed\"));\n \t\tcmit = get_revision(opts->revs);\n-\t\tif (!cmit || get_revision(opts->revs))\n-\t\t\treturn error(\"BUG: expected exactly one commit from walk\");\n+\t\tif (!cmit)\n+\t\t\treturn error(_(\"empty commit set passed\"));\n+\t\tif (get_revision(opts->revs))\n+\t\t\tBUG(\"unexpected extra commit from walk\");\n \t\treturn single_pick(cmit, opts);\n \t}\n \n-- \n2.18.0.400.g702e398724\n"},{"id":"352220","messageId":"nycvar.QRO.7.76.6.1807111057500.75@tvgsbejvaqbjf.bet","threadId":"48846","inReplyTo":"20180710043120.GA1330@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/2] de-confuse git cherry-pick --author","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-11T08:58:07Z","receivedAt":"2018-07-11T08:58:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Tue, 10 Jul 2018, Jeff King wrote:\n\n> On Mon, Jul 09, 2018 at 10:15:05PM -0400, Jeff King wrote:\n> \n> > > Should this not rather be\n> > > \n> > > -\t\tif (!cmit || get_revision(opts->revs))\n> > > -\t\t\treturn error(\"BUG: expected exactly one commit from walk\");\n> > > +\t\tif (!cmit)\n> > > +\t\t\treturn error(_(\"empty commit set passed\"));\n> > > +\t\tif (get_revision(opts->revs))\n> > > +\t\t\treturn error(_(\"unexpected extra commit from walk\"));\n> > \n> > Yeah, you're right. I'm not sure how a single rev with no-walk would\n> > ever turn up more than one commit, though. So I think we should probably\n> > go with:\n> > \n> >   if (!cmit)\n> > \treturn error(_(\"empty commit set passed\"));\n> >   if (get_revision(opts->revs))\n> > \tBUG(\"unexpected extra commit from walk\");\n> > \n> > And then if we ever see that case, we can decide from there what the\n> > right action is (though _probably_ it's just to emit an error like you\n> > have above, it might be a sign that our single-pick logic is wrong).\n> > \n> > I'll re-roll in that direction, and discuss further in the commit\n> > message.\n> \n> After poking at it a bit more, I've convinced myself that this is the\n> right thing, as options like \"--branches\" which expand into multiple\n> tips already push us into the other code path.\n> \n> So here's a re-roll. The first one is identical except for the typo-fix\n> in the commit message.\n> \n>   [1/2]: sequencer: handle empty-set cases consistently\n>   [2/2]: sequencer: don't say BUG on bogus input\n> \n>  sequencer.c                     | 12 ++++++++----\n>  t/t3510-cherry-pick-sequence.sh |  7 ++++++-\n>  2 files changed, 14 insertions(+), 5 deletions(-)\n\nACK,\nDscho\n"},{"id":"352258","messageId":"xmqqr2k94wdb.fsf@gitster-ct.c.googlers.com","threadId":"48846","inReplyTo":"nycvar.QRO.7.76.6.1807111057500.75@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 0/2] de-confuse git cherry-pick --author","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-11T15:37:04Z","receivedAt":"2018-07-11T15:37:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> After poking at it a bit more, I've convinced myself that this is the\n>> right thing, as options like \"--branches\" which expand into multiple\n>> tips already push us into the other code path.\n>> \n>> So here's a re-roll. The first one is identical except for the typo-fix\n>> in the commit message.\n>> \n>>   [1/2]: sequencer: handle empty-set cases consistently\n>>   [2/2]: sequencer: don't say BUG on bogus input\n>> \n>>  sequencer.c                     | 12 ++++++++----\n>>  t/t3510-cherry-pick-sequence.sh |  7 ++++++-\n>>  2 files changed, 14 insertions(+), 5 deletions(-)\n>\n> ACK,\n> Dscho\n\nThanks, both.  Queued and pushed out.\n"}]}