{"thread":{"id":"46505","subject":"[PATCH 0/4] handling empty inputs in the revision machinery","startedAt":"2017-08-02T22:24:32Z","lastAt":"2017-08-03T17:00:11Z","messageCount":13,"participants":["Jeff King","Junio C Hamano","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"325486","messageId":"20170802222425.7xkoxniz2xbjlnku@sigill.intra.peff.net","threadId":"46505","inReplyTo":null,"subject":"[PATCH 0/4] handling empty inputs in the revision machinery","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-02T22:24:25Z","receivedAt":"2017-08-02T22:24:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I noticed that:\n\n  git log --tags=does-not-exist\n\nwill show all of HEAD, which is rather confusing. This fixes it, and\nalso hits several other cases that were marked as expect_failure for\nrev-list. There is one case it doesn't handle: --stdin. It's not clear\nto me what the right behavior is there. I'll follow up with more\ndiscussion.\n\n  [1/4]: t6018: flesh out empty input/output rev-list tests\n  [2/4]: revision: add rev_input_given flag\n  [3/4]: rev-list: don't show usage when we see empty ref patterns\n  [4/4]: revision: do not fallback to default when rev_input_given is set\n\n builtin/rev-list.c       |  3 ++-\n revision.c               |  3 ++-\n revision.h               |  7 +++++++\n t/t4202-log.sh           |  6 ++++++\n t/t6018-rev-list-glob.sh | 20 +++++++++-----------\n 5 files changed, 26 insertions(+), 13 deletions(-)\n\n-Peff\n"},{"id":"325487","messageId":"20170802222451.pyh5o6irnbli7mhx@sigill.intra.peff.net","threadId":"46505","inReplyTo":"20170802222425.7xkoxniz2xbjlnku@sigill.intra.peff.net","subject":"[PATCH 1/4] t6018: flesh out empty input/output rev-list tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-02T22:24:51Z","receivedAt":"2017-08-02T22:24:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In 751a2ac6e (rev-list --exclude: tests, 2013-11-01), we\nadded a few tests for handling \"empty\" inputs with rev-list\n(i.e., where the user gave us some pattern but it turned out\nnot to queue any objects for traversal), all of which were\nmarked as failing.\n\nIn preparation for working on this area of the code, let's\ngive each test a more descriptive name. Let's also include\none more case which we should cover: feeding a --glob\npattern that doesn't match anything.\n\nWe can also drop the explanatory comment; we'll be\nconverting these to expect_success in the next few patches,\nso the discussion isn't necessary.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t6018-rev-list-glob.sh | 20 +++++++++-----------\n 1 file changed, 9 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh\nindex 381f35ed1..f8367b829 100755\n--- a/t/t6018-rev-list-glob.sh\n+++ b/t/t6018-rev-list-glob.sh\n@@ -255,27 +255,19 @@ test_expect_success 'rev-list accumulates multiple --exclude' '\n \tcompare rev-list \"--exclude=refs/remotes/* --exclude=refs/tags/* --all\" --branches\n '\n \n-\n-# \"git rev-list<ENTER>\" is likely to be a bug in the calling script and may\n-# deserve an error message, but do cases where set of refs programmatically\n-# given using globbing and/or --stdin need to fail with the same error, or\n-# are we better off reporting a success with no output?  The following few\n-# tests document the current behaviour to remind us that we might want to\n-# think about this issue.\n-\n-test_expect_failure 'rev-list may want to succeed with empty output on no input (1)' '\n+test_expect_failure 'rev-list should succeed with empty output on empty stdin' '\n \t>expect &&\n \tgit rev-list --stdin <expect >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'rev-list may want to succeed with empty output on no input (2)' '\n+test_expect_failure 'rev-list should succeed with empty output with all refs excluded' '\n \t>expect &&\n \tgit rev-list --exclude=* --all >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'rev-list may want to succeed with empty output on no input (3)' '\n+test_expect_failure 'rev-list should succeed with empty output with empty --all' '\n \t(\n \t\ttest_create_repo empty &&\n \t\tcd empty &&\n@@ -285,6 +277,12 @@ test_expect_failure 'rev-list may want to succeed with empty output on no input\n \t)\n '\n \n+test_expect_failure 'rev-list should succeed with empty output with empty glob' '\n+\t>expect &&\n+\tgit rev-list --glob=does-not-match-anything >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'shortlog accepts --glob/--tags/--remotes' '\n \n \tcompare shortlog \"subspace/one subspace/two\" --branches=subspace &&\n-- \n2.14.0.rc1.586.g00244b0b6\n\n"},{"id":"325488","messageId":"20170802222527.mnn7e6hlyevsgk7a@sigill.intra.peff.net","threadId":"46505","inReplyTo":"20170802222425.7xkoxniz2xbjlnku@sigill.intra.peff.net","subject":"[PATCH 2/4] revision: add rev_input_given flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-02T22:25:27Z","receivedAt":"2017-08-02T22:25:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Normally a caller that invokes setup_revisions() has to\ncheck rev.pending to see if anything was actually queued for\nthe traversal. But they can't tell the difference between\ntwo cases:\n\n  1. The user gave us no tip from which to start a\n     traversal.\n\n  2. The user tried to give us tips via --glob, --all, etc,\n     but their patterns ended up being empty.\n\nLet's set a flag in the rev_info struct that callers can use\nto tell the difference.  We can set this from the\ninit_all_refs_cb() function.  That's a little funny because\nit's not exactly about initializing the \"cb\" struct itself.\nBut that function is the common setup place for doing\npattern traversals that is used by --glob, --all, etc.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n revision.c | 1 +\n revision.h | 7 +++++++\n 2 files changed, 8 insertions(+)\n\ndiff --git a/revision.c b/revision.c\nindex 6603af944..08d5806b8 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1168,6 +1168,7 @@ static void init_all_refs_cb(struct all_refs_cb *cb, struct rev_info *revs,\n {\n \tcb->all_revs = revs;\n \tcb->all_flags = flags;\n+\trevs->rev_input_given = 1;\n }\n \n void clear_ref_exclusion(struct string_list **ref_excludes_p)\ndiff --git a/revision.h b/revision.h\nindex f96e7f7f4..c8f4e91f2 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -71,6 +71,13 @@ struct rev_info {\n \tconst char *def;\n \tstruct pathspec prune_data;\n \n+\t/*\n+\t * Whether the arguments parsed by setup_revisions() included any\n+\t * \"input\" revisions that might still have yielded an empty pending\n+\t * list (e.g., patterns like \"--all\" or \"--glob\").\n+\t */\n+\tint rev_input_given;\n+\n \t/* topo-sort */\n \tenum rev_sort_order sort_order;\n \n-- \n2.14.0.rc1.586.g00244b0b6\n\n"},{"id":"325489","messageId":"20170802222606.lbibwzygs5mr2xv5@sigill.intra.peff.net","threadId":"46505","inReplyTo":"20170802222425.7xkoxniz2xbjlnku@sigill.intra.peff.net","subject":"[PATCH 3/4] rev-list: don't show usage when we see empty ref patterns","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-02T22:26:06Z","receivedAt":"2017-08-02T22:26:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If the user gives us no starting point for a traversal, we\nwant to complain with our normal usage message. But if they\ntried to do so with \"--all\" or \"--glob\", but that happened\nnot to match any refs, the usage message isn't helpful. We\nshould just give them the empty output they asked for\ninstead.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis will have a minor textual conflict with my reflog series, which\ntouches the same conditional.\n\n builtin/rev-list.c       | 3 ++-\n t/t6018-rev-list-glob.sh | 6 +++---\n 2 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 95d84d5cd..1e9cc5948 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -350,7 +350,8 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \n \tif ((!revs.commits &&\n \t     (!(revs.tag_objects || revs.tree_objects || revs.blob_objects) &&\n-\t      !revs.pending.nr)) ||\n+\t      !revs.pending.nr) &&\n+\t     !revs.rev_input_given) ||\n \t    revs.diff)\n \t\tusage(rev_list_usage);\n \ndiff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh\nindex f8367b829..d3453c583 100755\n--- a/t/t6018-rev-list-glob.sh\n+++ b/t/t6018-rev-list-glob.sh\n@@ -261,13 +261,13 @@ test_expect_failure 'rev-list should succeed with empty output on empty stdin' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'rev-list should succeed with empty output with all refs excluded' '\n+test_expect_success 'rev-list should succeed with empty output with all refs excluded' '\n \t>expect &&\n \tgit rev-list --exclude=* --all >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'rev-list should succeed with empty output with empty --all' '\n+test_expect_success 'rev-list should succeed with empty output with empty --all' '\n \t(\n \t\ttest_create_repo empty &&\n \t\tcd empty &&\n@@ -277,7 +277,7 @@ test_expect_failure 'rev-list should succeed with empty output with empty --all'\n \t)\n '\n \n-test_expect_failure 'rev-list should succeed with empty output with empty glob' '\n+test_expect_success 'rev-list should succeed with empty output with empty glob' '\n \t>expect &&\n \tgit rev-list --glob=does-not-match-anything >actual &&\n \ttest_cmp expect actual\n-- \n2.14.0.rc1.586.g00244b0b6\n\n"},{"id":"325491","messageId":"20170802223018.h2uabdpmtz3gw27g@sigill.intra.peff.net","threadId":"46505","inReplyTo":"20170802222425.7xkoxniz2xbjlnku@sigill.intra.peff.net","subject":"[PATCH 4/4] revision: do not fallback to default when rev_input_given is set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-02T22:30:19Z","receivedAt":"2017-08-02T22:30:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If revs->def is set (as it is in \"git log\") and there are no\npending objects after parsing the user's input, then we show\nwhatever is in \"def\". But if the user _did_ ask for some\ninput that just happened to be empty (e.g., \"--glob\" that\ndoes not match anything), showing the default revision is\nconfusing. We should just show nothing, as that is what the\nuser's request yielded.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe \"!got_rev_arg\" that's already in the conditional is interesting. I\nwondered if it could be subsumed by the rev_input_given flag. But\ndigging in the history, I think it's mostly about doing reflog walks.\nUsually if we see a rev arg it will result either in an object added to\nthe pending queue, or a fatal error. But empty reflogs are the\nexception. And since my other nearby series adds a separate check for\n\"are we doing an empty reflog walk\", I don't think it makes sense to\ntangle this up the new flag I'm adding here.\n\n revision.c     | 2 +-\n t/t4202-log.sh | 6 ++++++\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex 08d5806b8..ba2b166cd 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2316,7 +2316,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\topt->tweak(revs, opt);\n \tif (revs->show_merge)\n \t\tprepare_show_merge(revs);\n-\tif (revs->def && !revs->pending.nr && !got_rev_arg) {\n+\tif (revs->def && !revs->pending.nr && !revs->rev_input_given && !got_rev_arg) {\n \t\tstruct object_id oid;\n \t\tstruct object *object;\n \t\tstruct object_context oc;\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 3f3531f0a..36d120c96 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1523,6 +1523,12 @@ test_expect_success 'log diagnoses bogus HEAD' '\n \ttest_i18ngrep broken stderr\n '\n \n+test_expect_success 'log does not default to HEAD when rev input is given' '\n+\t>expect &&\n+\tgit log --branches=does-not-exist >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'set up --source tests' '\n \tgit checkout --orphan source-a &&\n \ttest_commit one &&\n-- \n2.14.0.rc1.586.g00244b0b6\n"},{"id":"325492","messageId":"20170802223416.gwiezhbuxbdmbjzx@sigill.intra.peff.net","threadId":"46505","inReplyTo":"20170802222425.7xkoxniz2xbjlnku@sigill.intra.peff.net","subject":"[PATCH DONOTAPPLY 5/4] revision: let --stdin set rev_input_given","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-02T22:34:16Z","receivedAt":"2017-08-02T22:34:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 02, 2017 at 06:24:25PM -0400, Jeff King wrote:\n\n> I noticed that:\n> \n>   git log --tags=does-not-exist\n> \n> will show all of HEAD, which is rather confusing. This fixes it, and\n> also hits several other cases that were marked as expect_failure for\n> rev-list. There is one case it doesn't handle: --stdin. It's not clear\n> to me what the right behavior is there. I'll follow up with more\n> discussion.\n\nSo here that is. The patch below is what I had intended to send, but I\nfound some interesting corner cases.\n\nThis patch makes \"rev-list --stdin </dev/null\" return an empty set.\nWhich makes sense to me. But a side effect is that:\n\n  git log --stdin </dev/null\n\nnow shows nothing (rather than HEAD). I think that's probably the right\nthing. But:\n\n  (echo --; echo t) | git log --stdin\n\nno longer defaults to HEAD. Which maybe people would see as a\nregression. I could see arguments either way.\n\nBut this also breaks filter-branch (or at least a few of its tests),\nwhich really wants to do:\n\n  git rev-list --default HEAD --stdin <maybe-empty\n\nand traverse HEAD. I didn't dig enough to see if it's actually sane or\nnot. The failing tests seem to be weird noop filters that our test\nscript uses. But I'm worried it would break some real case, too.\n\n-- >8 --\nSubject: [PATCH] revision: let --stdin set rev_input_given\n\nCurrently \"git rev-list --stdin </dev/null\" returns a usage\nerror.\n\nThis is similar to the \"rev-list --glob=does-not-exist\" case\nwe fixed recently: in both cases the user tried to give us\nsome input, but it happened to be empty. But what we should\ndo in that case is less clear than with ref patterns like\n\"--glob\". In those cases the user clearly asked us\nto look for something which turned out to be the empty set,\nand we now handle that by returning an empty output.\n\nWith --stdin, on the other hand, they just asked us to take\ninput from a different place. So one could argue that a\ntotally empty input is still a usage problem. Or even that:\n\n  (\n    # no commits!\n    echo \"--\"\n    echo \"pathspec\"\n  ) | git rev-list --stdin\n\nshould complain, because they gave us no starting points.\n\nBut in practice that distinction isn't really helpful.\nGiving \"--stdin\" does show a conscious effort to provide\nsome input (so showing the usage message is likely to be\nannoying and useless). And it's handy for scripted callers\nto be able to map an empty input to an empty output; it's\none less corner case for them to worry about.\n\nSo let's treat \"--stdin\" as \"giving input\", even if it's\nempty.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n revision.c               | 1 +\n t/t6018-rev-list-glob.sh | 2 +-\n 2 files changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex ba2b166cd..6a1ccd407 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2253,6 +2253,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\t\t}\n \t\t\t\tif (read_from_stdin++)\n \t\t\t\t\tdie(\"--stdin given twice?\");\n+\t\t\t\trevs->rev_input_given = 1;\n \t\t\t\tread_revisions_from_stdin(revs, &prune_data);\n \t\t\t\tcontinue;\n \t\t\t}\ndiff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh\nindex d3453c583..bd300521b 100755\n--- a/t/t6018-rev-list-glob.sh\n+++ b/t/t6018-rev-list-glob.sh\n@@ -255,7 +255,7 @@ test_expect_success 'rev-list accumulates multiple --exclude' '\n \tcompare rev-list \"--exclude=refs/remotes/* --exclude=refs/tags/* --all\" --branches\n '\n \n-test_expect_failure 'rev-list should succeed with empty output on empty stdin' '\n+test_expect_success 'rev-list should succeed with empty output on empty stdin' '\n \t>expect &&\n \tgit rev-list --stdin <expect >actual &&\n \ttest_cmp expect actual\n-- \n2.14.0.rc1.586.g00244b0b6\n\n"},{"id":"325493","messageId":"xmqqh8xpr41q.fsf@gitster.mtv.corp.google.com","threadId":"46505","inReplyTo":"20170802222425.7xkoxniz2xbjlnku@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] handling empty inputs in the revision machinery","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-02T22:35:45Z","receivedAt":"2017-08-02T22:35:58Z","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> I noticed that:\n>\n>   git log --tags=does-not-exist\n>\n> will show all of HEAD, which is rather confusing. This fixes it, and\n> also hits several other cases that were marked as expect_failure for\n> rev-list. There is one case it doesn't handle: --stdin. It's not clear\n> to me what the right behavior is there. I'll follow up with more\n> discussion.\n\nNaïvely, I would expect that an empty input from --stdin should\nstill prevent us from defaulting to \"HEAD\", just like --glob=* and\nfriends should do.  Perhaps there are conter-example scenarios I\nhaven't thought of..\n\nThanks.\n\n\n>\n>   [1/4]: t6018: flesh out empty input/output rev-list tests\n>   [2/4]: revision: add rev_input_given flag\n>   [3/4]: rev-list: don't show usage when we see empty ref patterns\n>   [4/4]: revision: do not fallback to default when rev_input_given is set\n>\n>  builtin/rev-list.c       |  3 ++-\n>  revision.c               |  3 ++-\n>  revision.h               |  7 +++++++\n>  t/t4202-log.sh           |  6 ++++++\n>  t/t6018-rev-list-glob.sh | 20 +++++++++-----------\n>  5 files changed, 26 insertions(+), 13 deletions(-)\n>\n> -Peff\n"},{"id":"325494","messageId":"xmqqd18dr3rj.fsf@gitster.mtv.corp.google.com","threadId":"46505","inReplyTo":"20170802222527.mnn7e6hlyevsgk7a@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] revision: add rev_input_given flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-02T22:41:52Z","receivedAt":"2017-08-02T22:41:59Z","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> Normally a caller that invokes setup_revisions() has to\n> check rev.pending to see if anything was actually queued for\n> the traversal. But they can't tell the difference between\n> two cases:\n>\n>   1. The user gave us no tip from which to start a\n>      traversal.\n>\n>   2. The user tried to give us tips via --glob, --all, etc,\n>      but their patterns ended up being empty.\n>\n> Let's set a flag in the rev_info struct that callers can use\n> to tell the difference.  We can set this from the\n> init_all_refs_cb() function.  That's a little funny because\n> it's not exactly about initializing the \"cb\" struct itself.\n> But that function is the common setup place for doing\n> pattern traversals that is used by --glob, --all, etc.\n\n...and \"--bisect\", which is an oddball so we probably do not have to\ncare.  I didn't check if there is a fallout on that codepath.\n\n\n\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  revision.c | 1 +\n>  revision.h | 7 +++++++\n>  2 files changed, 8 insertions(+)\n>\n> diff --git a/revision.c b/revision.c\n> index 6603af944..08d5806b8 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1168,6 +1168,7 @@ static void init_all_refs_cb(struct all_refs_cb *cb, struct rev_info *revs,\n>  {\n>  \tcb->all_revs = revs;\n>  \tcb->all_flags = flags;\n> +\trevs->rev_input_given = 1;\n>  }\n>  \n>  void clear_ref_exclusion(struct string_list **ref_excludes_p)\n> diff --git a/revision.h b/revision.h\n> index f96e7f7f4..c8f4e91f2 100644\n> --- a/revision.h\n> +++ b/revision.h\n> @@ -71,6 +71,13 @@ struct rev_info {\n>  \tconst char *def;\n>  \tstruct pathspec prune_data;\n>  \n> +\t/*\n> +\t * Whether the arguments parsed by setup_revisions() included any\n> +\t * \"input\" revisions that might still have yielded an empty pending\n> +\t * list (e.g., patterns like \"--all\" or \"--glob\").\n> +\t */\n> +\tint rev_input_given;\n> +\n>  \t/* topo-sort */\n>  \tenum rev_sort_order sort_order;\n"},{"id":"325495","messageId":"xmqq8tj1r3n8.fsf@gitster.mtv.corp.google.com","threadId":"46505","inReplyTo":"20170802223018.h2uabdpmtz3gw27g@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] revision: do not fallback to default when rev_input_given is set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-02T22:44:27Z","receivedAt":"2017-08-02T22:44:40Z","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 revs->def is set (as it is in \"git log\") and there are no\n> pending objects after parsing the user's input, then we show\n> whatever is in \"def\". But if the user _did_ ask for some\n> input that just happened to be empty (e.g., \"--glob\" that\n> does not match anything), showing the default revision is\n> confusing. We should just show nothing, as that is what the\n> user's request yielded.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> The \"!got_rev_arg\" that's already in the conditional is interesting. I\n> wondered if it could be subsumed by the rev_input_given flag. But\n> digging in the history, I think it's mostly about doing reflog walks.\n> Usually if we see a rev arg it will result either in an object added to\n> the pending queue, or a fatal error. But empty reflogs are the\n> exception. And since my other nearby series adds a separate check for\n> \"are we doing an empty reflog walk\", I don't think it makes sense to\n> tangle this up the new flag I'm adding here.\n\nOK, I'll have to stare at possible merge conflicts to see if I like\nthis or some other design decision ;-)\n\nThis shows one of the reasons why I want consumers of revision\nmachinery not to be futzing these internal implementation detail\nbits in the revs structure, by the way.\n\n>  revision.c     | 2 +-\n>  t/t4202-log.sh | 6 ++++++\n>  2 files changed, 7 insertions(+), 1 deletion(-)\n>\n> diff --git a/revision.c b/revision.c\n> index 08d5806b8..ba2b166cd 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2316,7 +2316,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n>  \t\topt->tweak(revs, opt);\n>  \tif (revs->show_merge)\n>  \t\tprepare_show_merge(revs);\n> -\tif (revs->def && !revs->pending.nr && !got_rev_arg) {\n> +\tif (revs->def && !revs->pending.nr && !revs->rev_input_given && !got_rev_arg) {\n>  \t\tstruct object_id oid;\n>  \t\tstruct object *object;\n>  \t\tstruct object_context oc;\n> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> index 3f3531f0a..36d120c96 100755\n> --- a/t/t4202-log.sh\n> +++ b/t/t4202-log.sh\n> @@ -1523,6 +1523,12 @@ test_expect_success 'log diagnoses bogus HEAD' '\n>  \ttest_i18ngrep broken stderr\n>  '\n>  \n> +test_expect_success 'log does not default to HEAD when rev input is given' '\n> +\t>expect &&\n> +\tgit log --branches=does-not-exist >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_expect_success 'set up --source tests' '\n>  \tgit checkout --orphan source-a &&\n>  \ttest_commit one &&\n"},{"id":"325496","messageId":"20170802231114.oqf5ms47mccpjwhp@sigill.intra.peff.net","threadId":"46505","inReplyTo":"xmqqd18dr3rj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/4] revision: add rev_input_given flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-02T23:11:15Z","receivedAt":"2017-08-02T23:11:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 02, 2017 at 03:41:52PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Normally a caller that invokes setup_revisions() has to\n> > check rev.pending to see if anything was actually queued for\n> > the traversal. But they can't tell the difference between\n> > two cases:\n> >\n> >   1. The user gave us no tip from which to start a\n> >      traversal.\n> >\n> >   2. The user tried to give us tips via --glob, --all, etc,\n> >      but their patterns ended up being empty.\n> >\n> > Let's set a flag in the rev_info struct that callers can use\n> > to tell the difference.  We can set this from the\n> > init_all_refs_cb() function.  That's a little funny because\n> > it's not exactly about initializing the \"cb\" struct itself.\n> > But that function is the common setup place for doing\n> > pattern traversals that is used by --glob, --all, etc.\n> \n> ...and \"--bisect\", which is an oddball so we probably do not have to\n> care.  I didn't check if there is a fallout on that codepath.\n\nYeah, I saw that one and figured it should probably as \"input given\".\nThere's also \"--reflog\" and \"--indexed-objects\", which aren't covered\nhere. I'm not sure if anybody really cares (you'd generally use them\nwith \"--all\" anyway), so I left them out for now.\n\n-Peff\n"},{"id":"325497","messageId":"CAGZ79kYLAGHBfNsyHgkcHX1-7cBH0eTL-PcYS=eRE80BzBJJJg@mail.gmail.com","threadId":"46505","inReplyTo":"xmqq8tj1r3n8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/4] revision: do not fallback to default when rev_input_given is set","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-02T23:22:31Z","receivedAt":"2017-08-02T23:22:38Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Aug 2, 2017 at 3:44 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jeff King <peff@peff.net> writes:\n>\n>> If revs->def is set (as it is in \"git log\") and there are no\n>> pending objects after parsing the user's input, then we show\n>> whatever is in \"def\". But if the user _did_ ask for some\n>> input that just happened to be empty (e.g., \"--glob\" that\n>> does not match anything), showing the default revision is\n>> confusing. We should just show nothing, as that is what the\n>> user's request yielded.\n>>\n>> Signed-off-by: Jeff King <peff@peff.net>\n>> ---\n>> The \"!got_rev_arg\" that's already in the conditional is interesting. I\n>> wondered if it could be subsumed by the rev_input_given flag. But\n>> digging in the history, I think it's mostly about doing reflog walks.\n>> Usually if we see a rev arg it will result either in an object added to\n>> the pending queue, or a fatal error. But empty reflogs are the\n>> exception. And since my other nearby series adds a separate check for\n>> \"are we doing an empty reflog walk\", I don't think it makes sense to\n>> tangle this up the new flag I'm adding here.\n>\n> OK, I'll have to stare at possible merge conflicts to see if I like\n> this or some other design decision ;-)\n>\n> This shows one of the reasons why I want consumers of revision\n> machinery not to be futzing these internal implementation detail\n> bits in the revs structure, by the way.\n\ncc'd Johannes to see this example and discussion.\n\n>\n>>  revision.c     | 2 +-\n>>  t/t4202-log.sh | 6 ++++++\n>>  2 files changed, 7 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/revision.c b/revision.c\n>> index 08d5806b8..ba2b166cd 100644\n>> --- a/revision.c\n>> +++ b/revision.c\n>> @@ -2316,7 +2316,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n>>               opt->tweak(revs, opt);\n>>       if (revs->show_merge)\n>>               prepare_show_merge(revs);\n>> -     if (revs->def && !revs->pending.nr && !got_rev_arg) {\n>> +     if (revs->def && !revs->pending.nr && !revs->rev_input_given && !got_rev_arg) {\n>>               struct object_id oid;\n>>               struct object *object;\n>>               struct object_context oc;\n>> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n>> index 3f3531f0a..36d120c96 100755\n>> --- a/t/t4202-log.sh\n>> +++ b/t/t4202-log.sh\n>> @@ -1523,6 +1523,12 @@ test_expect_success 'log diagnoses bogus HEAD' '\n>>       test_i18ngrep broken stderr\n>>  '\n>>\n>> +test_expect_success 'log does not default to HEAD when rev input is given' '\n>> +     >expect &&\n>> +     git log --branches=does-not-exist >actual &&\n>> +     test_cmp expect actual\n>> +'\n>> +\n>>  test_expect_success 'set up --source tests' '\n>>       git checkout --orphan source-a &&\n>>       test_commit one &&\n"},{"id":"325547","messageId":"xmqq3798r6ai.fsf@gitster.mtv.corp.google.com","threadId":"46505","inReplyTo":"20170802223416.gwiezhbuxbdmbjzx@sigill.intra.peff.net","subject":"Re: [PATCH DONOTAPPLY 5/4] revision: let --stdin set rev_input_given","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-03T15:59:33Z","receivedAt":"2017-08-03T15:59:43Z","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> This patch makes \"rev-list --stdin </dev/null\" return an empty set.\n> Which makes sense to me. But a side effect is that:\n>\n>   git log --stdin </dev/null\n>\n> now shows nothing (rather than HEAD). I think that's probably the right\n> thing. But:\n>\n>   (echo --; echo t) | git log --stdin\n>\n> no longer defaults to HEAD. Which maybe people would see as a\n> regression. I could see arguments either way.\n\nYeah, thanks for thinking this through.  I do think this would be a\nregression.  On the other hand,\n\n    (printf \"%s\\n\" --tags=no-such -- t) | git log --stdin\n\nshould not default to HEAD and show nothing, I would think.\n\nSo if we wanted to do the \"--stdin\" thing properly, we probably need\nto keep the \"--stdin\" option itself neutral wrt \"did we get rev\ninput?\"; instead, each input item that comes in from the standard\ninput stream would decide if the user wants us to fall back to the\ndefault, perhaps?\n\n> But this also breaks filter-branch (or at least a few of its tests),\n> which really wants to do:\n>\n>   git rev-list --default HEAD --stdin <maybe-empty\n>\n> and traverse HEAD.\n\nHmph.  Do you mean the former should traverse from HEAD while the\nlatter should give us empty in the following two, because unlike\n\"log\", \"rev-list\" does not do the \"default to HEAD\" thing if it is\nnot told to do so?\n\n    git rev-list --default HEAD --stdin </dev/null\n    git rev-list --stdin </dev/null\n\nIf so, I think the reasoning makes sense.\n\n> I didn't dig enough to see if it's actually sane or\n> not. The failing tests seem to be weird noop filters that our test\n> script uses. But I'm worried it would break some real case, too.\n\nThanks.  Let's not rush things.  \n\nThe ones you sent for application 1-4/4 all are improvements.\n"},{"id":"325549","messageId":"20170803170002.qogk3ezvhgumnyi7@sigill.intra.peff.net","threadId":"46505","inReplyTo":"xmqq3798r6ai.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH DONOTAPPLY 5/4] revision: let --stdin set rev_input_given","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-03T17:00:02Z","receivedAt":"2017-08-03T17:00:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 03, 2017 at 08:59:33AM -0700, Junio C Hamano wrote:\n\n> >   (echo --; echo t) | git log --stdin\n> >\n> > no longer defaults to HEAD. Which maybe people would see as a\n> > regression. I could see arguments either way.\n> \n> Yeah, thanks for thinking this through.  I do think this would be a\n> regression.  On the other hand,\n> \n>     (printf \"%s\\n\" --tags=no-such -- t) | git log --stdin\n> \n> should not default to HEAD and show nothing, I would think.\n\nThat was the same conclusion I got to, but it actually doesn't matter,\nbecause we don't allow it anyway:\n\n  $ echo --tags=no-such | git log --stdin\n  fatal: options not supported in --stdin mode\n\nIf we were to make that work later, it should automatically do the right\nthing with respect to rev_input_given, since the stdin code would just\ncall into handle_revision_arg().\n\n> So if we wanted to do the \"--stdin\" thing properly, we probably need\n> to keep the \"--stdin\" option itself neutral wrt \"did we get rev\n> input?\"; instead, each input item that comes in from the standard\n> input stream would decide if the user wants us to fall back to the\n> default, perhaps?\n\nYes, exactly. It is a shame that scripts can't rely on an empty input to\n\"rev-list --stdin\" to produce a empty output, though. That seems like it\nwould be handy. On the other hand, nobody has complained about it in the\npast 10 years, so perhaps it just doesn't come up.\n\n> Hmph.  Do you mean the former should traverse from HEAD while the\n> latter should give us empty in the following two, because unlike\n> \"log\", \"rev-list\" does not do the \"default to HEAD\" thing if it is\n> not told to do so?\n> \n>     git rev-list --default HEAD --stdin </dev/null\n>     git rev-list --stdin </dev/null\n> \n> If so, I think the reasoning makes sense.\n\nTBH, I think both would make sense with an empty output. But it looks\nlike filter-branch is relying on the former to show HEAD. But like I\nsaid, I didn't dig into whether its expectation is sane. If anybody is\ninterested in digging further you can apply the --stdin patch in this\nthread and run t7003.\n\n> > I didn't dig enough to see if it's actually sane or\n> > not. The failing tests seem to be weird noop filters that our test\n> > script uses. But I'm worried it would break some real case, too.\n> \n> Thanks.  Let's not rush things.  \n> \n> The ones you sent for application 1-4/4 all are improvements.\n\nThanks. I agree that is probably an OK stopping point, as the --stdin\nbehavior is largely orthogonal (it just happens to be in the same\ngeneral area).\n\n-Peff\n"}]}