Re: [PATCH 3/3] get_sha1: don't die() on bogus search strings

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 10, 2016, 21:52 UTC
Message-ID: <xmqq1t8kz0zw.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160210211925.GC5799@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 9 quoted lines

> The get_sha1() function generally returns an error code
> rather than dying, and we sometimes speculatively call it
> with something that may be a revision or a pathspec, in
> order to see which one it might be.
>
> If it sees a bogus ":/" search string, though, it complains,
> without giving the caller the opportunity to recover. We can
> demonstrate this in t6133 by looking for ":/*.t", which
> should mean "*.t at the root of the tree", but instead dies

Slight nit. It means "'*.t at anywhere in the tree" aka "pretend as if you gave this pathspec while at the root level of the working tree".

Show 13 quoted lines

> because of the invalid regex (the "*" has nothing to operate
> on).
>
> We can fix this by returning an error rather than calling
> die(). Unfortunately, the tradeoff is that the error message
> is slightly worse in cases where we _do_ know we have a rev.
> E.g., running "git log ':/*.t' --" before yielded:
>
>   fatal: Invalid search pattern: *.t
>
> and now we get only:
>
>   fatal: bad revision ':/*.t'

I do not think the latter is necessarily worse, though. It is being consistent with these:

    $ git log mext --			;# no such branch
    fatal: bad revision 'mext'
    $ git log ':/xxxt' --		;# no commit matches that pattern
    fatal: bad revision ':/xxxt'

so I would not even mind if somebody argued that the current "invalid search pattern" is a bug, and gave us this patch as a fix for the inconsistency.

> To be honest, I'm not sure this is worth it. Part of me wants to say
> that get_sha1() is simply wrong for dying. And it is, but given how
> infrequently this would come up, it's perhaps a practical tradeoff to
> get the more accurate error message.

I am on the fence, too, and part of me wants to say the same thing. I however happen to view the "practical tradeoff" a bit differently, so I am slightly inclined to take this.

Show 9 quoted lines

> And while it does confuse ":/*.t", which is obviously a pathspec, that's
> just one specific case, that works because of the bogus regex. Something
> like ":/foo.*" could mean "find foo.* at the root" or it could mean
> "find a commit message with foo followed by anything", and we literally
> do not know which.
>
> We're likely to treat that one as a rev (assuming you use "foo" in your
> commit messages, but who doesn't?). So you'd need to use "--" in the
> general case anyway.

Yeah, I agree it probably would not make much practical difference.

Show 42 quoted lines

>  sha1_name.c                  |  4 ++--
>  t/t6133-pathspec-rev-dwim.sh | 10 ++++++++++
>  2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/sha1_name.c b/sha1_name.c
> index 892db21..d61b3b9 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -882,12 +882,12 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1,
>  
>  	if (prefix[0] == '!') {
>  		if (prefix[1] != '!')
> -			die ("Invalid search pattern: %s", prefix);
> +			return -1;
>  		prefix++;
>  	}
>  
>  	if (regcomp(&regex, prefix, REG_EXTENDED))
> -		die("Invalid search pattern: %s", prefix);
> +		return -1;
>  
>  	for (l = list; l; l = l->next) {
>  		l->item->object.flags |= ONELINE_SEEN;
> diff --git a/t/t6133-pathspec-rev-dwim.sh b/t/t6133-pathspec-rev-dwim.sh
> index 8e5b338..a290ffc 100755
> --- a/t/t6133-pathspec-rev-dwim.sh
> +++ b/t/t6133-pathspec-rev-dwim.sh
> @@ -35,4 +35,14 @@ test_expect_success '@{foo} with metacharacters dwims to rev' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success ':/*.t from a subdir dwims to a pathspec' '
> +	mkdir subdir &&
> +	(
> +		cd subdir &&
> +		git log -- ":/*.t" >expect &&
> +		git log    ":/*.t" >actual &&
> +		test_cmp expect actual
> +	)
> +'
> +
>  test_done