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

From: Jeff King <peff@peff.net>
Date: Aug 3, 2017, 17:00 UTC
Message-ID: <20170803170002.qogk3ezvhgumnyi7@sigill.intra.peff.net>
In-Reply-To: <xmqq3798r6ai.fsf@gitster.mtv.corp.google.com>

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

Show 11 quoted lines

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

Show 5 quoted lines

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

Show 9 quoted lines

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

Show 7 quoted lines

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