Re: [PATCH] t6018-rev-list-glob: fix 'empty stdin' test

[Jeff King <peff@peff.net>]

On Wed, Aug 22, 2018 at 11:50:46AM -0700, Junio C Hamano wrote:

> >  test_expect_failure 'rev-list should succeed with empty output on empty stdin' '
> > -	git rev-list --stdin <expect >actual &&
> > +	git rev-list --stdin </dev/null >actual &&
> >  	test_must_be_empty actual
> >  '
> 
> By the way, it may be about time to turn that expect-failure into
> expect-success.  It is somewhat unfortunate that 0c5dc743 ("t6018:
> flesh out empty input/output rev-list tests", 2017-08-02) removed
> the comment that said "we _might_ want to change the behaviour in
> these cases" and explained the tests as reminders, anticipating that
> the series will change the behaviour for three cases where the
> pending list ends up empty to make the discussion moot, but it
> changed the behaviour of only two of them, leaving the "--stdin
> reads empty" case behind.

Yeah, I think I had intended to make --stdin work there, and when I
realized at the end of the series it did not, I failed to go back and
modify that initial commit touching the test.

More discussion in the original thread:

  https://public-inbox.org/git/20170802223416.gwiezhbuxbdmbjzx@sigill.intra.peff.net/

> It may be just the matter of doing something like the attached
> patch.  I won't be committing such a behaviour change during the
> pre-release feature freeze, but we may want to consider doing this
> early in the next cycle.

Yes, definitely this is tricky enough to avoid doing during the freeze.

> diff --git a/revision.c b/revision.c
> index d12e6d8a4a..21fb413511 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2441,6 +2441,19 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  		object = get_reference(revs, revs->def, &oid, 0);
>  		add_pending_object_with_mode(revs, object, revs->def, oc.mode);
>  	}
> +	/* 
> +	 * Even if revs->pending is empty after all the above, if we
> +	 * handled "--stdin", then the caller really meant to give us
> +	 * an empty commit range.  Just let the traversal give an
> +	 * empty result without causing a "no input?  do you know how
> +	 * to use this command?" failure.
> +	 *
> +	 * NOTE!!!  Because "--stdin </dev/null --default HEAD" should
> +	 * default to HEAD, this must come _after_ the above block
> +	 * that deals with revs->ref fallback.
> +	 */
> +	if (read_from_stdin)
> +		revs->rev_input_given = 1;

I think this should do the right thing. All of the issues discussed in
the earlier thread were about using revs->def, and this neatly sidesteps
that by touching the flag afterwards. It's a little funny that the flag
now means two things (earlier in the function it is "we should use the
default" and later it becomes "the caller may complain").

It may be worth splitting it into two flags: rev_input_given and
rev_read_stdin. That puts the responsibility on the caller to check both
flags. But really, rev-list.c is the only caller that checks it anyway.
And it would mean this scary comment can go away. ;)

It also _could_ be useful to other callers to distinguish the two cases
(e.g., if they wanted to know whether they were free to use stdin
themselves). But I don't offhand know of any callers that need that (I
have a vague recollection that it might have come up once over the
years).

-Peff