[PATCH 0/4] handling empty inputs in the revision machinery

[PATCH 0/4] handling empty inputs in the revision machinery

[Jeff King]

I noticed that:

  git log --tags=does-not-exist

will show all of HEAD, which is rather confusing. This fixes it, and
also hits several other cases that were marked as expect_failure for
rev-list. There is one case it doesn't handle: --stdin. It's not clear
to me what the right behavior is there. I'll follow up with more
discussion.

  [1/4]: t6018: flesh out empty input/output rev-list tests
  [2/4]: revision: add rev_input_given flag
  [3/4]: rev-list: don't show usage when we see empty ref patterns
  [4/4]: revision: do not fallback to default when rev_input_given is set

 builtin/rev-list.c       |  3 ++-
 revision.c               |  3 ++-
 revision.h               |  7 +++++++
 t/t4202-log.sh           |  6 ++++++
 t/t6018-rev-list-glob.sh | 20 +++++++++-----------
 5 files changed, 26 insertions(+), 13 deletions(-)

-Peff

[PATCH 1/4] t6018: flesh out empty input/output rev-list tests

[Jeff King]

In 751a2ac6e (rev-list --exclude: tests, 2013-11-01), we
added a few tests for handling "empty" inputs with rev-list
(i.e., where the user gave us some pattern but it turned out
not to queue any objects for traversal), all of which were
marked as failing.

In preparation for working on this area of the code, let's
give each test a more descriptive name. Let's also include
one more case which we should cover: feeding a --glob
pattern that doesn't match anything.

We can also drop the explanatory comment; we'll be
converting these to expect_success in the next few patches,
so the discussion isn't necessary.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t6018-rev-list-glob.sh | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh
index 381f35ed1..f8367b829 100755
--- a/t/t6018-rev-list-glob.sh
+++ b/t/t6018-rev-list-glob.sh
@@ -255,27 +255,19 @@ test_expect_success 'rev-list accumulates multiple --exclude' '
 	compare rev-list "--exclude=refs/remotes/* --exclude=refs/tags/* --all" --branches
 '
 
-
-# "git rev-list<ENTER>" is likely to be a bug in the calling script and may
-# deserve an error message, but do cases where set of refs programmatically
-# given using globbing and/or --stdin need to fail with the same error, or
-# are we better off reporting a success with no output?  The following few
-# tests document the current behaviour to remind us that we might want to
-# think about this issue.
-
-test_expect_failure 'rev-list may want to succeed with empty output on no input (1)' '
+test_expect_failure 'rev-list should succeed with empty output on empty stdin' '
 	>expect &&
 	git rev-list --stdin <expect >actual &&
 	test_cmp expect actual
 '
 
-test_expect_failure 'rev-list may want to succeed with empty output on no input (2)' '
+test_expect_failure 'rev-list should succeed with empty output with all refs excluded' '
 	>expect &&
 	git rev-list --exclude=* --all >actual &&
 	test_cmp expect actual
 '
 
-test_expect_failure 'rev-list may want to succeed with empty output on no input (3)' '
+test_expect_failure 'rev-list should succeed with empty output with empty --all' '
 	(
 		test_create_repo empty &&
 		cd empty &&
@@ -285,6 +277,12 @@ test_expect_failure 'rev-list may want to succeed with empty output on no input
 	)
 '
 
+test_expect_failure 'rev-list should succeed with empty output with empty glob' '
+	>expect &&
+	git rev-list --glob=does-not-match-anything >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'shortlog accepts --glob/--tags/--remotes' '
 
 	compare shortlog "subspace/one subspace/two" --branches=subspace &&
-- 
2.14.0.rc1.586.g00244b0b6

[PATCH 2/4] revision: add rev_input_given flag

[Jeff King]

Normally a caller that invokes setup_revisions() has to
check rev.pending to see if anything was actually queued for
the traversal. But they can't tell the difference between
two cases:

  1. The user gave us no tip from which to start a
     traversal.

  2. The user tried to give us tips via --glob, --all, etc,
     but their patterns ended up being empty.

Let's set a flag in the rev_info struct that callers can use
to tell the difference.  We can set this from the
init_all_refs_cb() function.  That's a little funny because
it's not exactly about initializing the "cb" struct itself.
But that function is the common setup place for doing
pattern traversals that is used by --glob, --all, etc.

Signed-off-by: Jeff King <peff@peff.net>
---
 revision.c | 1 +
 revision.h | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/revision.c b/revision.c
index 6603af944..08d5806b8 100644
--- a/revision.c
+++ b/revision.c
@@ -1168,6 +1168,7 @@ static void init_all_refs_cb(struct all_refs_cb *cb, struct rev_info *revs,
 {
 	cb->all_revs = revs;
 	cb->all_flags = flags;
+	revs->rev_input_given = 1;
 }
 
 void clear_ref_exclusion(struct string_list **ref_excludes_p)
diff --git a/revision.h b/revision.h
index f96e7f7f4..c8f4e91f2 100644
--- a/revision.h
+++ b/revision.h
@@ -71,6 +71,13 @@ struct rev_info {
 	const char *def;
 	struct pathspec prune_data;
 
+	/*
+	 * Whether the arguments parsed by setup_revisions() included any
+	 * "input" revisions that might still have yielded an empty pending
+	 * list (e.g., patterns like "--all" or "--glob").
+	 */
+	int rev_input_given;
+
 	/* topo-sort */
 	enum rev_sort_order sort_order;
 
-- 
2.14.0.rc1.586.g00244b0b6

Re: [PATCH 2/4] revision: add rev_input_given flag

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Normally a caller that invokes setup_revisions() has to
> check rev.pending to see if anything was actually queued for
> the traversal. But they can't tell the difference between
> two cases:
>
>   1. The user gave us no tip from which to start a
>      traversal.
>
>   2. The user tried to give us tips via --glob, --all, etc,
>      but their patterns ended up being empty.
>
> Let's set a flag in the rev_info struct that callers can use
> to tell the difference.  We can set this from the
> init_all_refs_cb() function.  That's a little funny because
> it's not exactly about initializing the "cb" struct itself.
> But that function is the common setup place for doing
> pattern traversals that is used by --glob, --all, etc.

...and "--bisect", which is an oddball so we probably do not have to
care.  I didn't check if there is a fallout on that codepath.




> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  revision.c | 1 +
>  revision.h | 7 +++++++
>  2 files changed, 8 insertions(+)
>
> diff --git a/revision.c b/revision.c
> index 6603af944..08d5806b8 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1168,6 +1168,7 @@ static void init_all_refs_cb(struct all_refs_cb *cb, struct rev_info *revs,
>  {
>  	cb->all_revs = revs;
>  	cb->all_flags = flags;
> +	revs->rev_input_given = 1;
>  }
>  
>  void clear_ref_exclusion(struct string_list **ref_excludes_p)
> diff --git a/revision.h b/revision.h
> index f96e7f7f4..c8f4e91f2 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -71,6 +71,13 @@ struct rev_info {
>  	const char *def;
>  	struct pathspec prune_data;
>  
> +	/*
> +	 * Whether the arguments parsed by setup_revisions() included any
> +	 * "input" revisions that might still have yielded an empty pending
> +	 * list (e.g., patterns like "--all" or "--glob").
> +	 */
> +	int rev_input_given;
> +
>  	/* topo-sort */
>  	enum rev_sort_order sort_order;

Re: [PATCH 2/4] revision: add rev_input_given flag

[Jeff King]

On Wed, Aug 02, 2017 at 03:41:52PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Normally a caller that invokes setup_revisions() has to
> > check rev.pending to see if anything was actually queued for
> > the traversal. But they can't tell the difference between
> > two cases:
> >
> >   1. The user gave us no tip from which to start a
> >      traversal.
> >
> >   2. The user tried to give us tips via --glob, --all, etc,
> >      but their patterns ended up being empty.
> >
> > Let's set a flag in the rev_info struct that callers can use
> > to tell the difference.  We can set this from the
> > init_all_refs_cb() function.  That's a little funny because
> > it's not exactly about initializing the "cb" struct itself.
> > But that function is the common setup place for doing
> > pattern traversals that is used by --glob, --all, etc.
> 
> ...and "--bisect", which is an oddball so we probably do not have to
> care.  I didn't check if there is a fallout on that codepath.

Yeah, I saw that one and figured it should probably as "input given".
There's also "--reflog" and "--indexed-objects", which aren't covered
here. I'm not sure if anybody really cares (you'd generally use them
with "--all" anyway), so I left them out for now.

-Peff

[PATCH 3/4] rev-list: don't show usage when we see empty ref patterns

[Jeff King]

If the user gives us no starting point for a traversal, we
want to complain with our normal usage message. But if they
tried to do so with "--all" or "--glob", but that happened
not to match any refs, the usage message isn't helpful. We
should just give them the empty output they asked for
instead.

Signed-off-by: Jeff King <peff@peff.net>
---
This will have a minor textual conflict with my reflog series, which
touches the same conditional.

 builtin/rev-list.c       | 3 ++-
 t/t6018-rev-list-glob.sh | 6 +++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 95d84d5cd..1e9cc5948 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -350,7 +350,8 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 
 	if ((!revs.commits &&
 	     (!(revs.tag_objects || revs.tree_objects || revs.blob_objects) &&
-	      !revs.pending.nr)) ||
+	      !revs.pending.nr) &&
+	     !revs.rev_input_given) ||
 	    revs.diff)
 		usage(rev_list_usage);
 
diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh
index f8367b829..d3453c583 100755
--- a/t/t6018-rev-list-glob.sh
+++ b/t/t6018-rev-list-glob.sh
@@ -261,13 +261,13 @@ test_expect_failure 'rev-list should succeed with empty output on empty stdin' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'rev-list should succeed with empty output with all refs excluded' '
+test_expect_success 'rev-list should succeed with empty output with all refs excluded' '
 	>expect &&
 	git rev-list --exclude=* --all >actual &&
 	test_cmp expect actual
 '
 
-test_expect_failure 'rev-list should succeed with empty output with empty --all' '
+test_expect_success 'rev-list should succeed with empty output with empty --all' '
 	(
 		test_create_repo empty &&
 		cd empty &&
@@ -277,7 +277,7 @@ test_expect_failure 'rev-list should succeed with empty output with empty --all'
 	)
 '
 
-test_expect_failure 'rev-list should succeed with empty output with empty glob' '
+test_expect_success 'rev-list should succeed with empty output with empty glob' '
 	>expect &&
 	git rev-list --glob=does-not-match-anything >actual &&
 	test_cmp expect actual
-- 
2.14.0.rc1.586.g00244b0b6

[PATCH 4/4] revision: do not fallback to default when rev_input_given is set

[Jeff King]

If revs->def is set (as it is in "git log") and there are no
pending objects after parsing the user's input, then we show
whatever is in "def". But if the user _did_ ask for some
input that just happened to be empty (e.g., "--glob" that
does not match anything), showing the default revision is
confusing. We should just show nothing, as that is what the
user's request yielded.

Signed-off-by: Jeff King <peff@peff.net>
---
The "!got_rev_arg" that's already in the conditional is interesting. I
wondered if it could be subsumed by the rev_input_given flag. But
digging in the history, I think it's mostly about doing reflog walks.
Usually if we see a rev arg it will result either in an object added to
the pending queue, or a fatal error. But empty reflogs are the
exception. And since my other nearby series adds a separate check for
"are we doing an empty reflog walk", I don't think it makes sense to
tangle this up the new flag I'm adding here.

 revision.c     | 2 +-
 t/t4202-log.sh | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/revision.c b/revision.c
index 08d5806b8..ba2b166cd 100644
--- a/revision.c
+++ b/revision.c
@@ -2316,7 +2316,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		opt->tweak(revs, opt);
 	if (revs->show_merge)
 		prepare_show_merge(revs);
-	if (revs->def && !revs->pending.nr && !got_rev_arg) {
+	if (revs->def && !revs->pending.nr && !revs->rev_input_given && !got_rev_arg) {
 		struct object_id oid;
 		struct object *object;
 		struct object_context oc;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 3f3531f0a..36d120c96 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1523,6 +1523,12 @@ test_expect_success 'log diagnoses bogus HEAD' '
 	test_i18ngrep broken stderr
 '
 
+test_expect_success 'log does not default to HEAD when rev input is given' '
+	>expect &&
+	git log --branches=does-not-exist >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'set up --source tests' '
 	git checkout --orphan source-a &&
 	test_commit one &&
-- 
2.14.0.rc1.586.g00244b0b6

Re: [PATCH 4/4] revision: do not fallback to default when rev_input_given is set

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> If revs->def is set (as it is in "git log") and there are no
> pending objects after parsing the user's input, then we show
> whatever is in "def". But if the user _did_ ask for some
> input that just happened to be empty (e.g., "--glob" that
> does not match anything), showing the default revision is
> confusing. We should just show nothing, as that is what the
> user's request yielded.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> The "!got_rev_arg" that's already in the conditional is interesting. I
> wondered if it could be subsumed by the rev_input_given flag. But
> digging in the history, I think it's mostly about doing reflog walks.
> Usually if we see a rev arg it will result either in an object added to
> the pending queue, or a fatal error. But empty reflogs are the
> exception. And since my other nearby series adds a separate check for
> "are we doing an empty reflog walk", I don't think it makes sense to
> tangle this up the new flag I'm adding here.

OK, I'll have to stare at possible merge conflicts to see if I like
this or some other design decision ;-)

This shows one of the reasons why I want consumers of revision
machinery not to be futzing these internal implementation detail
bits in the revs structure, by the way.

>  revision.c     | 2 +-
>  t/t4202-log.sh | 6 ++++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/revision.c b/revision.c
> index 08d5806b8..ba2b166cd 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2316,7 +2316,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  		opt->tweak(revs, opt);
>  	if (revs->show_merge)
>  		prepare_show_merge(revs);
> -	if (revs->def && !revs->pending.nr && !got_rev_arg) {
> +	if (revs->def && !revs->pending.nr && !revs->rev_input_given && !got_rev_arg) {
>  		struct object_id oid;
>  		struct object *object;
>  		struct object_context oc;
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 3f3531f0a..36d120c96 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -1523,6 +1523,12 @@ test_expect_success 'log diagnoses bogus HEAD' '
>  	test_i18ngrep broken stderr
>  '
>  
> +test_expect_success 'log does not default to HEAD when rev input is given' '
> +	>expect &&
> +	git log --branches=does-not-exist >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'set up --source tests' '
>  	git checkout --orphan source-a &&
>  	test_commit one &&

Re: [PATCH 4/4] revision: do not fallback to default when rev_input_given is set

[Stefan Beller]

On Wed, Aug 2, 2017 at 3:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> If revs->def is set (as it is in "git log") and there are no
>> pending objects after parsing the user's input, then we show
>> whatever is in "def". But if the user _did_ ask for some
>> input that just happened to be empty (e.g., "--glob" that
>> does not match anything), showing the default revision is
>> confusing. We should just show nothing, as that is what the
>> user's request yielded.
>>
>> Signed-off-by: Jeff King <peff@peff.net>
>> ---
>> The "!got_rev_arg" that's already in the conditional is interesting. I
>> wondered if it could be subsumed by the rev_input_given flag. But
>> digging in the history, I think it's mostly about doing reflog walks.
>> Usually if we see a rev arg it will result either in an object added to
>> the pending queue, or a fatal error. But empty reflogs are the
>> exception. And since my other nearby series adds a separate check for
>> "are we doing an empty reflog walk", I don't think it makes sense to
>> tangle this up the new flag I'm adding here.
>
> OK, I'll have to stare at possible merge conflicts to see if I like
> this or some other design decision ;-)
>
> This shows one of the reasons why I want consumers of revision
> machinery not to be futzing these internal implementation detail
> bits in the revs structure, by the way.

cc'd Johannes to see this example and discussion.

>
>>  revision.c     | 2 +-
>>  t/t4202-log.sh | 6 ++++++
>>  2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/revision.c b/revision.c
>> index 08d5806b8..ba2b166cd 100644
>> --- a/revision.c
>> +++ b/revision.c
>> @@ -2316,7 +2316,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>>               opt->tweak(revs, opt);
>>       if (revs->show_merge)
>>               prepare_show_merge(revs);
>> -     if (revs->def && !revs->pending.nr && !got_rev_arg) {
>> +     if (revs->def && !revs->pending.nr && !revs->rev_input_given && !got_rev_arg) {
>>               struct object_id oid;
>>               struct object *object;
>>               struct object_context oc;
>> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
>> index 3f3531f0a..36d120c96 100755
>> --- a/t/t4202-log.sh
>> +++ b/t/t4202-log.sh
>> @@ -1523,6 +1523,12 @@ test_expect_success 'log diagnoses bogus HEAD' '
>>       test_i18ngrep broken stderr
>>  '
>>
>> +test_expect_success 'log does not default to HEAD when rev input is given' '
>> +     >expect &&
>> +     git log --branches=does-not-exist >actual &&
>> +     test_cmp expect actual
>> +'
>> +
>>  test_expect_success 'set up --source tests' '
>>       git checkout --orphan source-a &&
>>       test_commit one &&

[PATCH DONOTAPPLY 5/4] revision: let --stdin set rev_input_given

[Jeff King]

On Wed, Aug 02, 2017 at 06:24:25PM -0400, Jeff King wrote:

> I noticed that:
> 
>   git log --tags=does-not-exist
> 
> will show all of HEAD, which is rather confusing. This fixes it, and
> also hits several other cases that were marked as expect_failure for
> rev-list. There is one case it doesn't handle: --stdin. It's not clear
> to me what the right behavior is there. I'll follow up with more
> discussion.

So here that is. The patch below is what I had intended to send, but I
found some interesting corner cases.

This patch makes "rev-list --stdin </dev/null" return an empty set.
Which makes sense to me. But a side effect is that:

  git log --stdin </dev/null

now shows nothing (rather than HEAD). I think that's probably the right
thing. But:

  (echo --; echo t) | git log --stdin

no longer defaults to HEAD. Which maybe people would see as a
regression. I could see arguments either way.

But this also breaks filter-branch (or at least a few of its tests),
which really wants to do:

  git rev-list --default HEAD --stdin <maybe-empty

and traverse HEAD. I didn't dig enough to see if it's actually sane or
not. The failing tests seem to be weird noop filters that our test
script uses. But I'm worried it would break some real case, too.

-- >8 --
Subject: [PATCH] revision: let --stdin set rev_input_given

Currently "git rev-list --stdin </dev/null" returns a usage
error.

This is similar to the "rev-list --glob=does-not-exist" case
we fixed recently: in both cases the user tried to give us
some input, but it happened to be empty. But what we should
do in that case is less clear than with ref patterns like
"--glob". In those cases the user clearly asked us
to look for something which turned out to be the empty set,
and we now handle that by returning an empty output.

With --stdin, on the other hand, they just asked us to take
input from a different place. So one could argue that a
totally empty input is still a usage problem. Or even that:

  (
    # no commits!
    echo "--"
    echo "pathspec"
  ) | git rev-list --stdin

should complain, because they gave us no starting points.

But in practice that distinction isn't really helpful.
Giving "--stdin" does show a conscious effort to provide
some input (so showing the usage message is likely to be
annoying and useless). And it's handy for scripted callers
to be able to map an empty input to an empty output; it's
one less corner case for them to worry about.

So let's treat "--stdin" as "giving input", even if it's
empty.

Signed-off-by: Jeff King <peff@peff.net>
---
 revision.c               | 1 +
 t/t6018-rev-list-glob.sh | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/revision.c b/revision.c
index ba2b166cd..6a1ccd407 100644
--- a/revision.c
+++ b/revision.c
@@ -2253,6 +2253,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 				}
 				if (read_from_stdin++)
 					die("--stdin given twice?");
+				revs->rev_input_given = 1;
 				read_revisions_from_stdin(revs, &prune_data);
 				continue;
 			}
diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh
index d3453c583..bd300521b 100755
--- a/t/t6018-rev-list-glob.sh
+++ b/t/t6018-rev-list-glob.sh
@@ -255,7 +255,7 @@ test_expect_success 'rev-list accumulates multiple --exclude' '
 	compare rev-list "--exclude=refs/remotes/* --exclude=refs/tags/* --all" --branches
 '
 
-test_expect_failure 'rev-list should succeed with empty output on empty stdin' '
+test_expect_success 'rev-list should succeed with empty output on empty stdin' '
 	>expect &&
 	git rev-list --stdin <expect >actual &&
 	test_cmp expect actual
-- 
2.14.0.rc1.586.g00244b0b6

Re: [PATCH DONOTAPPLY 5/4] revision: let --stdin set rev_input_given

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This patch makes "rev-list --stdin </dev/null" return an empty set.
> Which makes sense to me. But a side effect is that:
>
>   git log --stdin </dev/null
>
> now shows nothing (rather than HEAD). I think that's probably the right
> thing. But:
>
>   (echo --; echo t) | git log --stdin
>
> no longer defaults to HEAD. Which maybe people would see as a
> regression. I could see arguments either way.

Yeah, thanks for thinking this through.  I do think this would be a
regression.  On the other hand,

    (printf "%s\n" --tags=no-such -- t) | git log --stdin

should not default to HEAD and show nothing, I would think.

So if we wanted to do the "--stdin" thing properly, we probably need
to keep the "--stdin" option itself neutral wrt "did we get rev
input?"; instead, each input item that comes in from the standard
input stream would decide if the user wants us to fall back to the
default, perhaps?

> But this also breaks filter-branch (or at least a few of its tests),
> which really wants to do:
>
>   git rev-list --default HEAD --stdin <maybe-empty
>
> and traverse HEAD.

Hmph.  Do you mean the former should traverse from HEAD while the
latter should give us empty in the following two, because unlike
"log", "rev-list" does not do the "default to HEAD" thing if it is
not told to do so?

    git rev-list --default HEAD --stdin </dev/null
    git rev-list --stdin </dev/null

If so, I think the reasoning makes sense.

> I didn't dig enough to see if it's actually sane or
> not. The failing tests seem to be weird noop filters that our test
> script uses. But I'm worried it would break some real case, too.

Thanks.  Let's not rush things.  

The ones you sent for application 1-4/4 all are improvements.

Re: [PATCH DONOTAPPLY 5/4] revision: let --stdin set rev_input_given

[Jeff King]

On Thu, Aug 03, 2017 at 08:59:33AM -0700, Junio C Hamano wrote:

> >   (echo --; echo t) | git log --stdin
> >
> > no longer defaults to HEAD. Which maybe people would see as a
> > regression. I could see arguments either way.
> 
> Yeah, thanks for thinking this through.  I do think this would be a
> regression.  On the other hand,
> 
>     (printf "%s\n" --tags=no-such -- t) | git log --stdin
> 
> should not default to HEAD and show nothing, I would think.

That was the same conclusion I got to, but it actually doesn't matter,
because we don't allow it anyway:

  $ echo --tags=no-such | git log --stdin
  fatal: options not supported in --stdin mode

If we were to make that work later, it should automatically do the right
thing with respect to rev_input_given, since the stdin code would just
call into handle_revision_arg().

> So if we wanted to do the "--stdin" thing properly, we probably need
> to keep the "--stdin" option itself neutral wrt "did we get rev
> input?"; instead, each input item that comes in from the standard
> input stream would decide if the user wants us to fall back to the
> default, perhaps?

Yes, exactly. It is a shame that scripts can't rely on an empty input to
"rev-list --stdin" to produce a empty output, though. That seems like it
would be handy. On the other hand, nobody has complained about it in the
past 10 years, so perhaps it just doesn't come up.

> Hmph.  Do you mean the former should traverse from HEAD while the
> latter should give us empty in the following two, because unlike
> "log", "rev-list" does not do the "default to HEAD" thing if it is
> not told to do so?
> 
>     git rev-list --default HEAD --stdin </dev/null
>     git rev-list --stdin </dev/null
> 
> If so, I think the reasoning makes sense.

TBH, I think both would make sense with an empty output. But it looks
like filter-branch is relying on the former to show HEAD. But like I
said, I didn't dig into whether its expectation is sane. If anybody is
interested in digging further you can apply the --stdin patch in this
thread and run t7003.

> > I didn't dig enough to see if it's actually sane or
> > not. The failing tests seem to be weird noop filters that our test
> > script uses. But I'm worried it would break some real case, too.
> 
> Thanks.  Let's not rush things.  
> 
> The ones you sent for application 1-4/4 all are improvements.

Thanks. I agree that is probably an OK stopping point, as the --stdin
behavior is largely orthogonal (it just happens to be in the same
general area).

-Peff

Re: [PATCH 0/4] handling empty inputs in the revision machinery

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I noticed that:
>
>   git log --tags=does-not-exist
>
> will show all of HEAD, which is rather confusing. This fixes it, and
> also hits several other cases that were marked as expect_failure for
> rev-list. There is one case it doesn't handle: --stdin. It's not clear
> to me what the right behavior is there. I'll follow up with more
> discussion.

Naïvely, I would expect that an empty input from --stdin should
still prevent us from defaulting to "HEAD", just like --glob=* and
friends should do.  Perhaps there are conter-example scenarios I
haven't thought of..

Thanks.


>
>   [1/4]: t6018: flesh out empty input/output rev-list tests
>   [2/4]: revision: add rev_input_given flag
>   [3/4]: rev-list: don't show usage when we see empty ref patterns
>   [4/4]: revision: do not fallback to default when rev_input_given is set
>
>  builtin/rev-list.c       |  3 ++-
>  revision.c               |  3 ++-
>  revision.h               |  7 +++++++
>  t/t4202-log.sh           |  6 ++++++
>  t/t6018-rev-list-glob.sh | 20 +++++++++-----------
>  5 files changed, 26 insertions(+), 13 deletions(-)
>
> -Peff