git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>
Subject: [PATCH DONOTAPPLY 5/4] revision: let --stdin set rev_input_given
Date: Wed, 2 Aug 2017 18:34:16 -0400	[thread overview]
Message-ID: <20170802223416.gwiezhbuxbdmbjzx@sigill.intra.peff.net> (raw)
In-Reply-To: <20170802222425.7xkoxniz2xbjlnku@sigill.intra.peff.net>

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


  parent reply	other threads:[~2017-08-02 22:34 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-02 22:24 [PATCH 0/4] handling empty inputs in the revision machinery Jeff King
2017-08-02 22:24 ` [PATCH 1/4] t6018: flesh out empty input/output rev-list tests Jeff King
2017-08-02 22:25 ` [PATCH 2/4] revision: add rev_input_given flag Jeff King
2017-08-02 22:41   ` Junio C Hamano
2017-08-02 23:11     ` Jeff King
2017-08-02 22:26 ` [PATCH 3/4] rev-list: don't show usage when we see empty ref patterns Jeff King
2017-08-02 22:30 ` [PATCH 4/4] revision: do not fallback to default when rev_input_given is set Jeff King
2017-08-02 22:44   ` Junio C Hamano
2017-08-02 23:22     ` Stefan Beller
2017-08-02 22:34 ` Jeff King [this message]
2017-08-03 15:59   ` [PATCH DONOTAPPLY 5/4] revision: let --stdin set rev_input_given Junio C Hamano
2017-08-03 17:00     ` Jeff King
2017-08-02 22:35 ` [PATCH 0/4] handling empty inputs in the revision machinery Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170802223416.gwiezhbuxbdmbjzx@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).