From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeff King Subject: [PATCH 3/3] get_sha1: don't die() on bogus search strings Date: Wed, 10 Feb 2016 16:19:25 -0500 Message-ID: <20160210211925.GC5799@sigill.intra.peff.net> References: <20160210211206.GA5755@sigill.intra.peff.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Cc: Duy Nguyen , Kirill Likhodedov , Johannes Schindelin , git To: Junio C Hamano X-From: git-owner@vger.kernel.org Wed Feb 10 22:19:50 2016 Return-path: Envelope-to: gcvg-git-2@plane.gmane.org Received: from vger.kernel.org ([209.132.180.67]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1aTcAw-0007rP-7L for gcvg-git-2@plane.gmane.org; Wed, 10 Feb 2016 22:19:50 +0100 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751003AbcBJVTd (ORCPT ); Wed, 10 Feb 2016 16:19:33 -0500 Received: from cloud.peff.net ([50.56.180.127]:40012 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1750759AbcBJVT1 (ORCPT ); Wed, 10 Feb 2016 16:19:27 -0500 Received: (qmail 16392 invoked by uid 102); 10 Feb 2016 21:19:27 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.84) with SMTP; Wed, 10 Feb 2016 16:19:27 -0500 Received: (qmail 31802 invoked by uid 107); 10 Feb 2016 21:19:30 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) by peff.net (qpsmtpd/0.84) with SMTP; Wed, 10 Feb 2016 16:19:30 -0500 Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Wed, 10 Feb 2016 16:19:25 -0500 Content-Disposition: inline In-Reply-To: <20160210211206.GA5755@sigill.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Archived-At: 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 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' There's not a simple way to fix this short of passing a "quiet" flag all the way through the get_sha1() stack. Signed-off-by: Jeff King --- 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. 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. 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 -- 2.7.1.545.gfd1d4e5