Jeff King <peff@xxxxxxxx> writes:
> 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".
> 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.
> 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.
> 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(®ex, 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html