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

From: Jeff King <peff@peff.net>
Date: Aug 22, 2018, 19:23 UTC
Message-ID: <20180822192308.GB19730@sigill.intra.peff.net>
In-Reply-To: <xmqqefeqme0p.fsf@gitster-ct.c.googlers.com>

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

Show 15 quoted lines

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

Show 21 quoted lines

> 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