git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Duy Nguyen <pclouds@gmail.com>,
	Kirill Likhodedov <kirill.likhodedov@jetbrains.com>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	git <git@vger.kernel.org>
Subject: Re: git show doesn't work on file names with square brackets
Date: Mon, 8 Feb 2016 15:56:38 -0500	[thread overview]
Message-ID: <20160208205637.GA13732@sigill.intra.peff.net> (raw)
In-Reply-To: <20160208202043.GA6002@sigill.intra.peff.net>

On Mon, Feb 08, 2016 at 03:20:43PM -0500, Jeff King wrote:

> On Mon, Feb 08, 2016 at 02:52:30PM -0500, Jeff King wrote:
> 
> > Here is my patch again, with that part removed, and the tests fixed up.
> > Though on reflection, I do think it would be better if we could simply
> > expand the wildcard globs to say "does this match anything in the file
> > system". That makes a nice, simple rule that follows the spirit of the
> > original. I'm not sure if it would be easy to apply magic like ":(top)"
> > there, but even if we don't, we're not worse off than we are today
> > (where that requires "--" unless it happens to have a wildcard, as
> > above).
> 
> So here is a hacky attempt at that. It uses glob(), which is not quite
> right for the reasons below, though I suspect works OK in practice.
> 
> I think doing it correctly would require actually calling our
> read_directory() function. That feels kind of heavy-weight for this
> case, but I guess in theory the pathspec limits it (and it's not like
> glob() does not have to walk the filesystem, too). So maybe it's not so
> bad.

And here that is. It does end up traversing quite a bit for something as
simple as "*.foo", because that doesn't let fill_directory() limit us at
all.

But having looked at this, I can't help but wonder if the rule should
not be "does the file exist" in the first place, but "is the file in the
index". This dwimmery is about commands like "log" that are reading
existing commits. I cannot think of a case where we would want to
include something that exists in the filesystem but not in the index.

---
diff --git a/setup.c b/setup.c
index 2c4b22c..1a40516 100644
--- a/setup.c
+++ b/setup.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "dir.h"
+#include "pathspec.h"
 #include "string-list.h"
 
 static int inside_git_dir = -1;
@@ -130,6 +131,33 @@ int path_inside_repo(const char *prefix, const char *path)
 	return 0;
 }
 
+/*
+ * Return true if a file exists that matches the pattern
+ * glob.
+ */
+static int pathspec_exists(const char *one_pathspec)
+{
+	struct dir_struct dir;
+	const char *pathspec_v[] = { one_pathspec, NULL };
+	struct pathspec pathspec;
+	int ret = 0;
+	int i;
+
+	memset(&dir, 0, sizeof(dir));
+	parse_pathspec(&pathspec, 0, 0, "", pathspec_v);
+
+	fill_directory(&dir, &pathspec);
+	for (i = 0; i < dir.nr; i++) {
+		if (dir_path_match(dir.entries[i], &pathspec, 0, NULL)) {
+			ret = 1;
+			break;
+		}
+	}
+
+	free(dir.entries);
+	return ret;
+}
+
 int check_filename(const char *prefix, const char *arg)
 {
 	const char *name;
@@ -139,12 +167,14 @@ int check_filename(const char *prefix, const char *arg)
 		if (arg[2] == '\0') /* ":/" is root dir, always exists */
 			return 1;
 		name = arg + 2;
-	} else if (!no_wildcard(arg))
-		return 1;
-	else if (prefix)
+	} else if (prefix)
 		name = prefix_filename(prefix, strlen(prefix), arg);
 	else
 		name = arg;
+
+	if (!no_wildcard(arg))
+		return pathspec_exists(name);
+
 	if (!lstat(name, &st))
 		return 1; /* file exists */
 	if (errno == ENOENT || errno == ENOTDIR)

  reply	other threads:[~2016-02-08 20:56 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-02-06 13:16 git show doesn't work on file names with square brackets Kirill Likhodedov
2016-02-06 14:21 ` Johannes Schindelin
2016-02-06 14:29   ` Kirill Likhodedov
2016-02-06 16:10     ` Johannes Schindelin
2016-02-06 23:48       ` Duy Nguyen
2016-02-07 15:11         ` Kirill Likhodedov
2016-02-08  5:06           ` Duy Nguyen
2016-02-08 14:15             ` Jeff King
2016-02-08 14:24               ` Jeff King
2016-02-08 15:07               ` Jeff King
2016-02-08 19:35                 ` Junio C Hamano
2016-02-08 19:52                   ` Jeff King
2016-02-08 20:20                     ` Jeff King
2016-02-08 20:56                       ` Jeff King [this message]
2016-02-08 22:36                         ` Junio C Hamano
2016-02-10 15:45                           ` Jeff King
2016-02-09 20:37                     ` Junio C Hamano
2016-02-10 16:15                       ` Jeff King
2016-02-10 17:35                         ` Junio C Hamano
2016-02-10 21:12                           ` Jeff King
2016-02-10 21:12                             ` [PATCH 1/3] checkout: reorder check_filename conditional Jeff King
2016-02-10 21:31                               ` Junio C Hamano
2016-02-10 21:14                             ` [PATCH 2/3] check_filename: tighten dwim-wildcard ambiguity Jeff King
2016-02-10 21:19                             ` [PATCH 3/3] get_sha1: don't die() on bogus search strings Jeff King
2016-02-10 21:52                               ` Junio C Hamano
2016-02-07 15:09       ` git show doesn't work on file names with square brackets Kirill Likhodedov
2016-02-07 17:10         ` Johannes Schindelin

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

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

  git send-email \
    --in-reply-to=20160208205637.GA13732@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=kirill.likhodedov@jetbrains.com \
    --cc=pclouds@gmail.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.
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 NNTP newsgroup(s).