[PATCH 0/8] convert pathspec.c to take an index parameter

[PATCH 0/8] convert pathspec.c to take an index parameter

[Brandon Williams]

This is another conversion series to convert the pathspec library code to take
in an index parameter instead of relying on cache macros or on the global
variable 'the_index'.

While I was working in the pathspec code I thought it would be good to do a
little more cleanup and make the API cleaner.  More specifically consolidating
the 'strip submodule slash' flags into a single flag while splitting out the
'submodule leading path' check performed in the expensive case into its own
flag.

Brandon Williams (8):
  pathspec: provide a more descriptive die message
  submodule: add die_in_unpopulated_submodule function
  pathspec: change PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag
  pathspec: rename PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP
  pathspec: convert strip_submodule_slash to take an index
  pathspec: convert find_pathspecs_matching_against_index to take an
    index
  pathspec: convert init_pathspec_item to take an index
  pathspec: convert parse_pathspec to take an index

 archive.c                        |  4 +-
 builtin/add.c                    | 14 +++---
 builtin/blame.c                  |  2 +-
 builtin/check-ignore.c           |  7 +--
 builtin/checkout.c               |  2 +-
 builtin/clean.c                  |  2 +-
 builtin/commit.c                 |  4 +-
 builtin/grep.c                   |  2 +-
 builtin/ls-files.c               |  6 +--
 builtin/ls-tree.c                |  5 ++-
 builtin/rerere.c                 |  2 +-
 builtin/reset.c                  |  4 +-
 builtin/rm.c                     |  4 +-
 builtin/submodule--helper.c      |  4 +-
 builtin/update-index.c           |  2 +-
 line-log.c                       |  2 +-
 pathspec.c                       | 92 +++++++++++++++++-----------------------
 pathspec.h                       | 19 +++++----
 revision.c                       |  5 ++-
 submodule.c                      | 32 +++++++++++++-
 submodule.h                      |  2 +
 t/t6134-pathspec-in-submodule.sh |  6 +--
 tree-diff.c                      |  2 +-
 23 files changed, 122 insertions(+), 102 deletions(-)

-- 
2.13.0.rc2.291.g57267f2277-goog

[PATCH 1/8] pathspec: provide a more descriptive die message

[Brandon Williams]

The current message displayed upon an internal error in
'init_pathspec_item()' isn't very descriptive and doesn't provide much
context to where the error occurred.  Update the error message to
provide more context to where the error occured.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 pathspec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pathspec.c b/pathspec.c
index 303efda83..61b5b1273 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -555,7 +555,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
 		 * would trigger that.
 		 */
 		die_inside_submodule_path(item);
-		die ("BUG: item->nowildcard_len > item->len || item->prefix > item->len)");
+		die ("BUG: error initializing pathspec_item");
 	}
 }
 
-- 
2.13.0.rc2.291.g57267f2277-goog

[PATCH 2/8] submodule: add die_in_unpopulated_submodule function

[Brandon Williams]

Currently 'git add' is the only command which dies when launched from an
unpopulated submodule (the place-holder directory for a submodule which
hasn't been checked out).  This is triggered implicitly by passing the
PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag to 'parse_pathspec()'.

Instead make this desire more explicit by creating a function
'die_in_unpopulated_submodule()' which dies if the provided 'prefix' has
a leading path component which matches a submodule in the the index.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/add.c                    |  3 +++
 pathspec.c                       | 29 -----------------------------
 submodule.c                      | 30 ++++++++++++++++++++++++++++++
 submodule.h                      |  2 ++
 t/t6134-pathspec-in-submodule.sh |  6 +-----
 5 files changed, 36 insertions(+), 34 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 9f53f020d..ec58e3679 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -17,6 +17,7 @@
 #include "revision.h"
 #include "bulk-checkin.h"
 #include "argv-array.h"
+#include "submodule.h"
 
 static const char * const builtin_add_usage[] = {
 	N_("git add [<options>] [--] <pathspec>..."),
@@ -379,6 +380,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	if (read_cache() < 0)
 		die(_("index file corrupt"));
 
+	die_in_unpopulated_submodule(&the_index, prefix);
+
 	/*
 	 * Check the "pathspec '%s' did not match any files" block
 	 * below before enabling new magic.
diff --git a/pathspec.c b/pathspec.c
index 61b5b1273..e37256c3b 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -424,27 +424,6 @@ static void strip_submodule_slash_expensive(struct pathspec_item *item)
 	}
 }
 
-static void die_inside_submodule_path(struct pathspec_item *item)
-{
-	int i;
-
-	for (i = 0; i < active_nr; i++) {
-		struct cache_entry *ce = active_cache[i];
-		int ce_len = ce_namelen(ce);
-
-		if (!S_ISGITLINK(ce->ce_mode))
-			continue;
-
-		if (item->len < ce_len ||
-		    !(item->match[ce_len] == '/' || item->match[ce_len] == '\0') ||
-		    memcmp(ce->name, item->match, ce_len))
-			continue;
-
-		die(_("Pathspec '%s' is in submodule '%.*s'"),
-		    item->original, ce_len, ce->name);
-	}
-}
-
 /*
  * Perform the initialization of a pathspec_item based on a pathspec element.
  */
@@ -547,14 +526,6 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
 	/* sanity checks, pathspec matchers assume these are sane */
 	if (item->nowildcard_len > item->len ||
 	    item->prefix         > item->len) {
-		/*
-		 * This case can be triggered by the user pointing us to a
-		 * pathspec inside a submodule, which is an input error.
-		 * Detect that here and complain, but fallback in the
-		 * non-submodule case to a BUG, as we have no idea what
-		 * would trigger that.
-		 */
-		die_inside_submodule_path(item);
 		die ("BUG: error initializing pathspec_item");
 	}
 }
diff --git a/submodule.c b/submodule.c
index 7c3c4b17f..9e0502f25 100644
--- a/submodule.c
+++ b/submodule.c
@@ -281,6 +281,36 @@ int is_submodule_populated_gently(const char *path, int *return_error_code)
 	return ret;
 }
 
+/*
+ * Dies if the provided 'prefix' corresponds to an unpopulated submodule
+ */
+void die_in_unpopulated_submodule(const struct index_state *istate,
+				  const char *prefix)
+{
+	int i, prefixlen;
+
+	if (!prefix)
+		return;
+
+	prefixlen = strlen(prefix);
+
+	for (i = 0; i < istate->cache_nr; i++) {
+		struct cache_entry *ce = istate->cache[i];
+		int ce_len = ce_namelen(ce);
+
+		if (!S_ISGITLINK(ce->ce_mode))
+			continue;
+		if (prefixlen <= ce_len)
+			continue;
+		if (strncmp(ce->name, prefix, ce_len))
+			continue;
+		if (prefix[ce_len] != '/')
+			continue;
+
+		die(_("in unpopulated submodule '%s'"), ce->name);
+	}
+}
+
 int parse_submodule_update_strategy(const char *value,
 		struct submodule_update_strategy *dst)
 {
diff --git a/submodule.h b/submodule.h
index 8a8bc49dc..f4fe6374d 100644
--- a/submodule.h
+++ b/submodule.h
@@ -48,6 +48,8 @@ extern int is_submodule_initialized(const char *path);
  * Otherwise the return error code is the same as of resolve_gitdir_gently.
  */
 extern int is_submodule_populated_gently(const char *path, int *return_error_code);
+extern void die_in_unpopulated_submodule(const struct index_state *istate,
+					 const char *prefix);
 extern int parse_submodule_update_strategy(const char *value,
 		struct submodule_update_strategy *dst);
 extern const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);
diff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh
index fd401ca60..0f1cb49ce 100755
--- a/t/t6134-pathspec-in-submodule.sh
+++ b/t/t6134-pathspec-in-submodule.sh
@@ -24,13 +24,9 @@ test_expect_success 'error message for path inside submodule' '
 	test_cmp expect actual
 '
 
-cat <<EOF >expect
-fatal: Pathspec '.' is in submodule 'sub'
-EOF
-
 test_expect_success 'error message for path inside submodule from within submodule' '
 	test_must_fail git -C sub add . 2>actual &&
-	test_cmp expect actual
+	test_i18ngrep "in unpopulated submodule" actual
 '
 
 test_done
-- 
2.13.0.rc2.291.g57267f2277-goog

[PATCH 3/8] pathspec: change PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag

[Brandon Williams]

It's confusing to have two different 'strip submodule slash' flags which
do subtly different things.  Both
PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE and
PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP will accomplish the same task of
striping a slash from a pathspec which matches a submodule entry in the
index.  The only difference is that
PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE will perform additional checks
and die if a pathspec has a leading path component which corresponds to
a submodule.  This additional functionality should be split out into its
own flag.

To this end, rename the PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag to
PATHSPEC_SUBMODULE_LEADING_PATH and change its behavior to only die if a
path descends into a submodule.  In addition add the
PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag to callers which relied on the
old slash stripping functionality.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/add.c          |  3 ++-
 builtin/check-ignore.c |  3 ++-
 pathspec.c             | 32 ++++++++++++++++----------------
 pathspec.h             |  9 +++------
 4 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index ec58e3679..2aa9aeab9 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -389,7 +389,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_FULL |
 		       PATHSPEC_SYMLINK_LEADING_PATH |
-		       PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE,
+		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |
+		       PATHSPEC_SUBMODULE_LEADING_PATH,
 		       prefix, argv);
 
 	if (add_new_files) {
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 1d73d3ca3..73237b2b1 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -87,7 +87,8 @@ static int check_ignore(struct dir_struct *dir,
 	parse_pathspec(&pathspec,
 		       PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP,
 		       PATHSPEC_SYMLINK_LEADING_PATH |
-		       PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE |
+		       PATHSPEC_SUBMODULE_LEADING_PATH |
+		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |
 		       PATHSPEC_KEEP_ORDER,
 		       prefix, argv);
 
diff --git a/pathspec.c b/pathspec.c
index e37256c3b..f37d5769d 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -398,29 +398,29 @@ static void strip_submodule_slash_cheap(struct pathspec_item *item)
 	}
 }
 
-static void strip_submodule_slash_expensive(struct pathspec_item *item)
+static void die_path_inside_submodule(const struct pathspec_item *item,
+				      const struct index_state *istate)
 {
 	int i;
 
-	for (i = 0; i < active_nr; i++) {
-		struct cache_entry *ce = active_cache[i];
+	for (i = 0; i < istate->cache_nr; i++) {
+		struct cache_entry *ce = istate->cache[i];
 		int ce_len = ce_namelen(ce);
 
 		if (!S_ISGITLINK(ce->ce_mode))
 			continue;
 
-		if (item->len <= ce_len || item->match[ce_len] != '/' ||
-		    memcmp(ce->name, item->match, ce_len))
+		if (item->len <= ce_len)
+			continue;
+		if (item->match[ce_len] != '/')
+			continue;
+		if (strncmp(ce->name, item->match, ce_len))
+			continue;
+		if (item->len == ce_len + 1)
 			continue;
 
-		if (item->len == ce_len + 1) {
-			/* strip trailing slash */
-			item->len--;
-			item->match[item->len] = '\0';
-		} else {
-			die(_("Pathspec '%s' is in submodule '%.*s'"),
-			    item->original, ce_len, ce->name);
-		}
+		die(_("Pathspec '%s' is in submodule '%.*s'"),
+		    item->original, ce_len, ce->name);
 	}
 }
 
@@ -499,9 +499,6 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
 	if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
 		strip_submodule_slash_cheap(item);
 
-	if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
-		strip_submodule_slash_expensive(item);
-
 	if (magic & PATHSPEC_LITERAL) {
 		item->nowildcard_len = item->len;
 	} else {
@@ -639,6 +636,9 @@ void parse_pathspec(struct pathspec *pathspec,
 			die(_("pathspec '%s' is beyond a symbolic link"), entry);
 		}
 
+		if (flags & PATHSPEC_SUBMODULE_LEADING_PATH)
+			die_path_inside_submodule(item + i, &the_index);
+
 		if (item[i].nowildcard_len < item[i].len)
 			pathspec->has_wildcard = 1;
 		pathspec->magic |= item[i].magic;
diff --git a/pathspec.h b/pathspec.h
index 55e976972..93a819cbf 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -63,13 +63,10 @@ struct pathspec {
 /* die if a symlink is part of the given path's directory */
 #define PATHSPEC_SYMLINK_LEADING_PATH (1<<4)
 /*
- * This is like a combination of ..LEADING_PATH and .._SLASH_CHEAP
- * (but not the same): it strips the trailing slash if the given path
- * is a gitlink but also checks and dies if gitlink is part of the
- * leading path (i.e. the given path goes beyond a submodule). It's
- * safer than _SLASH_CHEAP and also more expensive.
+ * Checks and dies if gitlink is part of the leading path
+ * (i.e. the given path goes beyond a submodule).
  */
-#define PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE (1<<5)
+#define PATHSPEC_SUBMODULE_LEADING_PATH (1<<5)
 #define PATHSPEC_PREFIX_ORIGIN (1<<6)
 #define PATHSPEC_KEEP_ORDER (1<<7)
 /*
-- 
2.13.0.rc2.291.g57267f2277-goog

Re: [PATCH 3/8] pathspec: change PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> It's confusing to have two different 'strip submodule slash' flags which
> do subtly different things.  Both
> PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE and
> PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP will accomplish the same task of
> striping a slash from a pathspec which matches a submodule entry in the
> index.  The only difference is that
> PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE will perform additional checks
> and die if a pathspec has a leading path component which corresponds to
> a submodule.  This additional functionality should be split out into its
> own flag.
>
> To this end, rename the PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag to
> PATHSPEC_SUBMODULE_LEADING_PATH and change its behavior to only die if a
> path descends into a submodule.  In addition add the
> PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag to callers which relied on the
> old slash stripping functionality.

"PATHSPEC_SUBMODULE_LEADING_PATH" feels like an unfinished sentence
to me.  Do I understand your description correctly if I say it is
about "checking" the leading path to see if it overlaps with a
submodule path?  IOW, I am wondering if the name should have the
word CHECK somewhere in it.

Re: [PATCH 3/8] pathspec: change PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag

[Brandon Williams]

On 05/10, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > It's confusing to have two different 'strip submodule slash' flags which
> > do subtly different things.  Both
> > PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE and
> > PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP will accomplish the same task of
> > striping a slash from a pathspec which matches a submodule entry in the
> > index.  The only difference is that
> > PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE will perform additional checks
> > and die if a pathspec has a leading path component which corresponds to
> > a submodule.  This additional functionality should be split out into its
> > own flag.
> >
> > To this end, rename the PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag to
> > PATHSPEC_SUBMODULE_LEADING_PATH and change its behavior to only die if a
> > path descends into a submodule.  In addition add the
> > PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag to callers which relied on the
> > old slash stripping functionality.
> 
> "PATHSPEC_SUBMODULE_LEADING_PATH" feels like an unfinished sentence
> to me.  Do I understand your description correctly if I say it is
> about "checking" the leading path to see if it overlaps with a
> submodule path?  IOW, I am wondering if the name should have the
> word CHECK somewhere in it.
> 

You're probably right, the name has something left to be desired.  I
chose it simply because it conforms to another flag
"PATHSPEC_SYMLINK_LEADING_PATH" which dies if there is a symlink in the
leading path.

And you are correct, it checks if the leading path overlaps with a
submodule path.  If it strictly matches a submodule path that's alright
though.  The point of this flag is to disallow paths which descend into
submodules.  One such use case is to prevent a user from trying to 'git
add' a file from a submodule.

-- 
Brandon Williams

[PATCH 4/8] pathspec: rename PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP

[Brandon Williams]

Now that there is only a single flag which strips a submodule slash,
rename PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP to
PATHSPEC_STRIP_SUBMODULE_SLASH.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/add.c               | 2 +-
 builtin/check-ignore.c      | 2 +-
 builtin/ls-files.c          | 2 +-
 builtin/reset.c             | 2 +-
 builtin/rm.c                | 2 +-
 builtin/submodule--helper.c | 2 +-
 pathspec.c                  | 6 +++---
 pathspec.h                  | 2 +-
 8 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 2aa9aeab9..51d7a5506 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -389,7 +389,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_FULL |
 		       PATHSPEC_SYMLINK_LEADING_PATH |
-		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |
+		       PATHSPEC_STRIP_SUBMODULE_SLASH |
 		       PATHSPEC_SUBMODULE_LEADING_PATH,
 		       prefix, argv);
 
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 73237b2b1..694e4c61b 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -88,7 +88,7 @@ static int check_ignore(struct dir_struct *dir,
 		       PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP,
 		       PATHSPEC_SYMLINK_LEADING_PATH |
 		       PATHSPEC_SUBMODULE_LEADING_PATH |
-		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |
+		       PATHSPEC_STRIP_SUBMODULE_SLASH |
 		       PATHSPEC_KEEP_ORDER,
 		       prefix, argv);
 
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index d449e46db..e9dee2e41 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -618,7 +618,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 
 	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_CWD |
-		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
+		       PATHSPEC_STRIP_SUBMODULE_SLASH,
 		       prefix, argv);
 
 	/*
diff --git a/builtin/reset.c b/builtin/reset.c
index fc3b906c4..529f2f9be 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -236,7 +236,7 @@ static void parse_args(struct pathspec *pathspec,
 
 	parse_pathspec(pathspec, 0,
 		       PATHSPEC_PREFER_FULL |
-		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |
+		       PATHSPEC_STRIP_SUBMODULE_SLASH |
 		       (patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0),
 		       prefix, argv);
 }
diff --git a/builtin/rm.c b/builtin/rm.c
index fb79dcab1..8fe12d0a7 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -272,7 +272,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 
 	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_CWD |
-		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
+		       PATHSPEC_STRIP_SUBMODULE_SLASH,
 		       prefix, argv);
 	refresh_index(&the_index, REFRESH_QUIET, &pathspec, NULL, NULL);
 
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 85aafe46a..69149b557 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -234,7 +234,7 @@ static int module_list_compute(int argc, const char **argv,
 	char *ps_matched = NULL;
 	parse_pathspec(pathspec, 0,
 		       PATHSPEC_PREFER_FULL |
-		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
+		       PATHSPEC_STRIP_SUBMODULE_SLASH,
 		       prefix, argv);
 
 	if (pathspec->nr)
diff --git a/pathspec.c b/pathspec.c
index f37d5769d..a1297ca02 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -386,7 +386,7 @@ static const char *parse_element_magic(unsigned *magic, int *prefix_len,
 		return parse_short_magic(magic, elem);
 }
 
-static void strip_submodule_slash_cheap(struct pathspec_item *item)
+static void strip_submodule_slash(struct pathspec_item *item)
 {
 	if (item->len >= 1 && item->match[item->len - 1] == '/') {
 		int i = cache_name_pos(item->match, item->len - 1);
@@ -496,8 +496,8 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
 		item->original = xstrdup(elt);
 	}
 
-	if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
-		strip_submodule_slash_cheap(item);
+	if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH)
+		strip_submodule_slash(item);
 
 	if (magic & PATHSPEC_LITERAL) {
 		item->nowildcard_len = item->len;
diff --git a/pathspec.h b/pathspec.h
index 93a819cbf..dd6456d5d 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -59,7 +59,7 @@ struct pathspec {
 #define PATHSPEC_PREFER_FULL (1<<1) /* No args means match everything */
 #define PATHSPEC_MAXDEPTH_VALID (1<<2) /* max_depth field is valid */
 /* strip the trailing slash if the given path is a gitlink */
-#define PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP (1<<3)
+#define PATHSPEC_STRIP_SUBMODULE_SLASH (1<<3)
 /* die if a symlink is part of the given path's directory */
 #define PATHSPEC_SYMLINK_LEADING_PATH (1<<4)
 /*
-- 
2.13.0.rc2.291.g57267f2277-goog

Re: [PATCH 4/8] pathspec: rename PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> Now that there is only a single flag which strips a submodule slash,
> rename PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP to
> PATHSPEC_STRIP_SUBMODULE_SLASH.

This is a logical follow-up to the previous step.

[PATCH 5/8] pathspec: convert strip_submodule_slash to take an index

[Brandon Williams]

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 pathspec.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index a1297ca02..cff069536 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -386,12 +386,13 @@ static const char *parse_element_magic(unsigned *magic, int *prefix_len,
 		return parse_short_magic(magic, elem);
 }
 
-static void strip_submodule_slash(struct pathspec_item *item)
+static void strip_submodule_slash(struct pathspec_item *item,
+				  const struct index_state *istate)
 {
 	if (item->len >= 1 && item->match[item->len - 1] == '/') {
-		int i = cache_name_pos(item->match, item->len - 1);
+		int i = index_name_pos(istate, item->match, item->len - 1);
 
-		if (i >= 0 && S_ISGITLINK(active_cache[i]->ce_mode)) {
+		if (i >= 0 && S_ISGITLINK(istate->cache[i]->ce_mode)) {
 			item->len--;
 			item->match[item->len] = '\0';
 		}
@@ -497,7 +498,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
 	}
 
 	if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH)
-		strip_submodule_slash(item);
+		strip_submodule_slash(item, &the_index);
 
 	if (magic & PATHSPEC_LITERAL) {
 		item->nowildcard_len = item->len;
-- 
2.13.0.rc2.291.g57267f2277-goog

Re: [PATCH 5/8] pathspec: convert strip_submodule_slash to take an index

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  pathspec.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

This conversion is not wrong per-se, but I wonder if there is a
practical situation where we want to validate a pathspec element
against an in-core index that does not represent the index currently
in effect (aka "the_index").  I do not think of any offhand myself.

[PATCH 6/8] pathspec: convert find_pathspecs_matching_against_index to take an index

[Brandon Williams]

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/add.c          |  4 ++--
 builtin/check-ignore.c |  2 +-
 pathspec.c             | 10 ++++++----
 pathspec.h             |  7 +++++--
 4 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 51d7a5506..4e3bf20c2 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -136,7 +136,7 @@ static char *prune_directory(struct dir_struct *dir, struct pathspec *pathspec,
 			*dst++ = entry;
 	}
 	dir->nr = dst - dir->entries;
-	add_pathspec_matches_against_index(pathspec, seen);
+	add_pathspec_matches_against_index(pathspec, &the_index, seen);
 	return seen;
 }
 
@@ -418,7 +418,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		int i;
 
 		if (!seen)
-			seen = find_pathspecs_matching_against_index(&pathspec);
+			seen = find_pathspecs_matching_against_index(&pathspec, &the_index);
 
 		/*
 		 * file_exists() assumes exact match
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 694e4c61b..446b76dcf 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -97,7 +97,7 @@ static int check_ignore(struct dir_struct *dir,
 	 * should not be ignored, in order to be consistent with
 	 * 'git status', 'git add' etc.
 	 */
-	seen = find_pathspecs_matching_against_index(&pathspec);
+	seen = find_pathspecs_matching_against_index(&pathspec, &the_index);
 	for (i = 0; i < pathspec.nr; i++) {
 		full_path = pathspec.items[i].match;
 		exclude = NULL;
diff --git a/pathspec.c b/pathspec.c
index cff069536..bbd71b48b 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -17,6 +17,7 @@
  * to use find_pathspecs_matching_against_index() instead.
  */
 void add_pathspec_matches_against_index(const struct pathspec *pathspec,
+					const struct index_state *istate,
 					char *seen)
 {
 	int num_unmatched = 0, i;
@@ -32,8 +33,8 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,
 			num_unmatched++;
 	if (!num_unmatched)
 		return;
-	for (i = 0; i < active_nr; i++) {
-		const struct cache_entry *ce = active_cache[i];
+	for (i = 0; i < istate->cache_nr; i++) {
+		const struct cache_entry *ce = istate->cache[i];
 		ce_path_match(ce, pathspec, seen);
 	}
 }
@@ -46,10 +47,11 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,
  * nature of the "closest" (i.e. most specific) matches which each of the
  * given pathspecs achieves against all items in the index.
  */
-char *find_pathspecs_matching_against_index(const struct pathspec *pathspec)
+char *find_pathspecs_matching_against_index(const struct pathspec *pathspec,
+					    const struct index_state *istate)
 {
 	char *seen = xcalloc(pathspec->nr, 1);
-	add_pathspec_matches_against_index(pathspec, seen);
+	add_pathspec_matches_against_index(pathspec, istate, seen);
 	return seen;
 }
 
diff --git a/pathspec.h b/pathspec.h
index dd6456d5d..dfb22440a 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -103,7 +103,10 @@ static inline int ps_strcmp(const struct pathspec_item *item,
 		return strcmp(s1, s2);
 }
 
-extern char *find_pathspecs_matching_against_index(const struct pathspec *pathspec);
-extern void add_pathspec_matches_against_index(const struct pathspec *pathspec, char *seen);
+extern void add_pathspec_matches_against_index(const struct pathspec *pathspec,
+					       const struct index_state *istate,
+					       char *seen);
+extern char *find_pathspecs_matching_against_index(const struct pathspec *pathspec,
+						   const struct index_state *istate);
 
 #endif /* PATHSPEC_H */
-- 
2.13.0.rc2.291.g57267f2277-goog

[PATCH 7/8] pathspec: convert init_pathspec_item to take an index

[Brandon Williams]

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 pathspec.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index bbd71b48b..c7ed8b3fb 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -430,7 +430,9 @@ static void die_path_inside_submodule(const struct pathspec_item *item,
 /*
  * Perform the initialization of a pathspec_item based on a pathspec element.
  */
-static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
+static void init_pathspec_item(struct pathspec_item *item,
+			       const struct index_state *istate,
+			       unsigned flags,
 			       const char *prefix, int prefixlen,
 			       const char *elt)
 {
@@ -500,7 +502,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
 	}
 
 	if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH)
-		strip_submodule_slash(item, &the_index);
+		strip_submodule_slash(item, istate);
 
 	if (magic & PATHSPEC_LITERAL) {
 		item->nowildcard_len = item->len;
@@ -627,7 +629,8 @@ void parse_pathspec(struct pathspec *pathspec,
 	for (i = 0; i < n; i++) {
 		entry = argv[i];
 
-		init_pathspec_item(item + i, flags, prefix, prefixlen, entry);
+		init_pathspec_item(item + i, &the_index, flags,
+				   prefix, prefixlen, entry);
 
 		if (item[i].magic & PATHSPEC_EXCLUDE)
 			nr_exclude++;
@@ -653,7 +656,7 @@ void parse_pathspec(struct pathspec *pathspec,
 	 */
 	if (nr_exclude == n) {
 		int plen = (!(flags & PATHSPEC_PREFER_CWD)) ? 0 : prefixlen;
-		init_pathspec_item(item + n, 0, prefix, plen, "");
+		init_pathspec_item(item + n, &the_index, 0, prefix, plen, "");
 		pathspec->nr++;
 	}
 
-- 
2.13.0.rc2.291.g57267f2277-goog

[PATCH 8/8] pathspec: convert parse_pathspec to take an index

[Brandon Williams]

Convert 'parse_pathspec()' to take an index parameter.

Since the index is only needed when the PATHSPEC_SUBMODULE_LEADING_PATH
and PATHSPEC_STRIP_SUBMODULE_SLASH flags are given, add a check
requiring a non-NULL index when either of these flags are given.
Convert callers which use these two flags to pass in an index while
having other callers pass in NULL.

Now that pathspec.c does not use any cache macros and has no references
to 'the_index', mark it by defining NO_THE_INDEX_COMPATIBILITY_MACROS.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 archive.c                   |  4 ++--
 builtin/add.c               |  4 ++--
 builtin/blame.c             |  2 +-
 builtin/check-ignore.c      |  2 +-
 builtin/checkout.c          |  2 +-
 builtin/clean.c             |  2 +-
 builtin/commit.c            |  4 ++--
 builtin/grep.c              |  2 +-
 builtin/ls-files.c          |  4 ++--
 builtin/ls-tree.c           |  5 +++--
 builtin/rerere.c            |  2 +-
 builtin/reset.c             |  2 +-
 builtin/rm.c                |  2 +-
 builtin/submodule--helper.c |  2 +-
 builtin/update-index.c      |  2 +-
 line-log.c                  |  2 +-
 pathspec.c                  | 13 ++++++++++---
 pathspec.h                  |  1 +
 revision.c                  |  5 +++--
 submodule.c                 |  2 +-
 tree-diff.c                 |  2 +-
 21 files changed, 38 insertions(+), 28 deletions(-)

diff --git a/archive.c b/archive.c
index 60b889198..ce9b30f2e 100644
--- a/archive.c
+++ b/archive.c
@@ -306,7 +306,7 @@ static int path_exists(struct tree *tree, const char *path)
 	struct pathspec pathspec;
 	int ret;
 
-	parse_pathspec(&pathspec, 0, 0, "", paths);
+	parse_pathspec(&pathspec, NULL, 0, 0, "", paths);
 	pathspec.recursive = 1;
 	ret = read_tree_recursive(tree, "", 0, 0, &pathspec,
 				  reject_entry, &pathspec);
@@ -322,7 +322,7 @@ static void parse_pathspec_arg(const char **pathspec,
 	 * Also if pathspec patterns are dependent, we're in big
 	 * trouble as we test each one separately
 	 */
-	parse_pathspec(&ar_args->pathspec, 0,
+	parse_pathspec(&ar_args->pathspec, NULL, 0,
 		       PATHSPEC_PREFER_FULL,
 		       "", pathspec);
 	ar_args->pathspec.recursive = 1;
diff --git a/builtin/add.c b/builtin/add.c
index 4e3bf20c2..23606db39 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -181,7 +181,7 @@ int interactive_add(int argc, const char **argv, const char *prefix, int patch)
 {
 	struct pathspec pathspec;
 
-	parse_pathspec(&pathspec, 0,
+	parse_pathspec(&pathspec, NULL, 0,
 		       PATHSPEC_PREFER_FULL |
 		       PATHSPEC_SYMLINK_LEADING_PATH |
 		       PATHSPEC_PREFIX_ORIGIN,
@@ -386,7 +386,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	 * Check the "pathspec '%s' did not match any files" block
 	 * below before enabling new magic.
 	 */
-	parse_pathspec(&pathspec, 0,
+	parse_pathspec(&pathspec, &the_index, 0,
 		       PATHSPEC_PREFER_FULL |
 		       PATHSPEC_SYMLINK_LEADING_PATH |
 		       PATHSPEC_STRIP_SUBMODULE_SLASH |
diff --git a/builtin/blame.c b/builtin/blame.c
index f7aa95f4b..e37837c17 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -557,7 +557,7 @@ static struct origin *find_origin(struct scoreboard *sb,
 	paths[0] = origin->path;
 	paths[1] = NULL;
 
-	parse_pathspec(&diff_opts.pathspec,
+	parse_pathspec(&diff_opts.pathspec, NULL,
 		       PATHSPEC_ALL_MAGIC & ~PATHSPEC_LITERAL,
 		       PATHSPEC_LITERAL_PATH, "", paths);
 	diff_setup_done(&diff_opts);
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 446b76dcf..90169e79a 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -84,7 +84,7 @@ static int check_ignore(struct dir_struct *dir,
 	 * check-ignore just needs paths. Magic beyond :/ is really
 	 * irrelevant.
 	 */
-	parse_pathspec(&pathspec,
+	parse_pathspec(&pathspec, &the_index,
 		       PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP,
 		       PATHSPEC_SYMLINK_LEADING_PATH |
 		       PATHSPEC_SUBMODULE_LEADING_PATH |
diff --git a/builtin/checkout.c b/builtin/checkout.c
index bfa5419f3..cb8ed09f6 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1274,7 +1274,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	}
 
 	if (argc) {
-		parse_pathspec(&opts.pathspec, 0,
+		parse_pathspec(&opts.pathspec, NULL, 0,
 			       opts.patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0,
 			       prefix, argv);
 
diff --git a/builtin/clean.c b/builtin/clean.c
index d861f836a..ebc980b4f 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -926,7 +926,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 	for (i = 0; i < exclude_list.nr; i++)
 		add_exclude(exclude_list.items[i].string, "", 0, el, -(i+1));
 
-	parse_pathspec(&pathspec, 0,
+	parse_pathspec(&pathspec, NULL, 0,
 		       PATHSPEC_PREFER_CWD,
 		       prefix, argv);
 
diff --git a/builtin/commit.c b/builtin/commit.c
index 4e288bc51..014193728 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -342,7 +342,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 
 	if (is_status)
 		refresh_flags |= REFRESH_UNMERGED;
-	parse_pathspec(&pathspec, 0,
+	parse_pathspec(&pathspec, NULL, 0,
 		       PATHSPEC_PREFER_FULL,
 		       prefix, argv);
 
@@ -1373,7 +1373,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	handle_untracked_files_arg(&s);
 	if (show_ignored_in_status)
 		s.show_ignored_files = 1;
-	parse_pathspec(&s.pathspec, 0,
+	parse_pathspec(&s.pathspec, NULL, 0,
 		       PATHSPEC_PREFER_FULL,
 		       prefix, argv);
 
diff --git a/builtin/grep.c b/builtin/grep.c
index 65070c52f..d330a85cb 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1212,7 +1212,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			verify_filename(prefix, argv[j], j == i && allow_revs);
 	}
 
-	parse_pathspec(&pathspec, 0,
+	parse_pathspec(&pathspec, NULL, 0,
 		       PATHSPEC_PREFER_CWD |
 		       (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),
 		       prefix, argv + i);
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index e9dee2e41..f5d81ab1d 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -429,7 +429,7 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)
 	if (prefix) {
 		static const char *(matchbuf[1]);
 		matchbuf[0] = NULL;
-		parse_pathspec(&pathspec, PATHSPEC_ALL_MAGIC,
+		parse_pathspec(&pathspec, NULL, PATHSPEC_ALL_MAGIC,
 			       PATHSPEC_PREFER_CWD, prefix, matchbuf);
 	} else
 		memset(&pathspec, 0, sizeof(pathspec));
@@ -616,7 +616,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		die("ls-files --recurse-submodules does not support "
 		    "--error-unmatch");
 
-	parse_pathspec(&pathspec, 0,
+	parse_pathspec(&pathspec, &the_index, 0,
 		       PATHSPEC_PREFER_CWD |
 		       PATHSPEC_STRIP_SUBMODULE_SLASH,
 		       prefix, argv);
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index d7ebeb4ce..1d1083ed7 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -173,8 +173,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 	 * cannot be lifted until it is converted to use
 	 * match_pathspec() or tree_entry_interesting()
 	 */
-	parse_pathspec(&pathspec, PATHSPEC_ALL_MAGIC &
-				  ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
+	parse_pathspec(&pathspec, NULL,
+		       PATHSPEC_ALL_MAGIC &
+		       ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
 		       PATHSPEC_PREFER_CWD,
 		       prefix, argv + 1);
 	for (i = 0; i < pathspec.nr; i++)
diff --git a/builtin/rerere.c b/builtin/rerere.c
index 1bf72423b..749599a9c 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c
@@ -75,7 +75,7 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
 		struct pathspec pathspec;
 		if (argc < 2)
 			warning("'git rerere forget' without paths is deprecated");
-		parse_pathspec(&pathspec, 0, PATHSPEC_PREFER_CWD,
+		parse_pathspec(&pathspec, NULL, 0, PATHSPEC_PREFER_CWD,
 			       prefix, argv + 1);
 		return rerere_forget(&pathspec);
 	}
diff --git a/builtin/reset.c b/builtin/reset.c
index 529f2f9be..f9da86622 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -234,7 +234,7 @@ static void parse_args(struct pathspec *pathspec,
 	if (read_cache() < 0)
 		die(_("index file corrupt"));
 
-	parse_pathspec(pathspec, 0,
+	parse_pathspec(pathspec, &the_index, 0,
 		       PATHSPEC_PREFER_FULL |
 		       PATHSPEC_STRIP_SUBMODULE_SLASH |
 		       (patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0),
diff --git a/builtin/rm.c b/builtin/rm.c
index 8fe12d0a7..cffd27c20 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -270,7 +270,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	if (read_cache() < 0)
 		die(_("index file corrupt"));
 
-	parse_pathspec(&pathspec, 0,
+	parse_pathspec(&pathspec, &the_index, 0,
 		       PATHSPEC_PREFER_CWD |
 		       PATHSPEC_STRIP_SUBMODULE_SLASH,
 		       prefix, argv);
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 69149b557..b06d62d80 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -232,7 +232,7 @@ static int module_list_compute(int argc, const char **argv,
 {
 	int i, result = 0;
 	char *ps_matched = NULL;
-	parse_pathspec(pathspec, 0,
+	parse_pathspec(pathspec, &the_index, 0,
 		       PATHSPEC_PREFER_FULL |
 		       PATHSPEC_STRIP_SUBMODULE_SLASH,
 		       prefix, argv);
diff --git a/builtin/update-index.c b/builtin/update-index.c
index ebfc09faa..0aaf7e0da 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -714,7 +714,7 @@ static int do_reupdate(int ac, const char **av,
 	int has_head = 1;
 	struct pathspec pathspec;
 
-	parse_pathspec(&pathspec, 0,
+	parse_pathspec(&pathspec, NULL, 0,
 		       PATHSPEC_PREFER_CWD,
 		       prefix, av + 1);
 
diff --git a/line-log.c b/line-log.c
index a23b91047..4c3232639 100644
--- a/line-log.c
+++ b/line-log.c
@@ -754,7 +754,7 @@ void line_log_init(struct rev_info *rev, const char *prefix, struct string_list
 			argv_array_push(&array, r->path);
 		paths = argv_array_detach(&array);
 
-		parse_pathspec(&rev->diffopt.pathspec, 0,
+		parse_pathspec(&rev->diffopt.pathspec, NULL, 0,
 			       PATHSPEC_PREFER_FULL, "", paths);
 		/* strings are now owned by pathspec */
 		free(paths);
diff --git a/pathspec.c b/pathspec.c
index c7ed8b3fb..b9c87b8d9 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -1,3 +1,4 @@
+#define NO_THE_INDEX_COMPATIBILITY_MACROS
 #include "cache.h"
 #include "dir.h"
 #include "pathspec.h"
@@ -573,6 +574,7 @@ static void NORETURN unsupported_magic(const char *pattern,
  * pathspec. die() if any magic in magic_mask is used.
  */
 void parse_pathspec(struct pathspec *pathspec,
+		    const struct index_state *istate,
 		    unsigned magic_mask, unsigned flags,
 		    const char *prefix, const char **argv)
 {
@@ -593,6 +595,11 @@ void parse_pathspec(struct pathspec *pathspec,
 	    (flags & PATHSPEC_PREFER_FULL))
 		die("BUG: PATHSPEC_PREFER_CWD and PATHSPEC_PREFER_FULL are incompatible");
 
+	if (!istate && ((flags & PATHSPEC_STRIP_SUBMODULE_SLASH) ||
+			(flags & PATHSPEC_SUBMODULE_LEADING_PATH)))
+	    die("BUG: PATHSPEC_STRIP_SUBMODULE_SLASH and "
+		"PATHSPEC_SUBMODULE_LEADING_PATH require an index");
+
 	/* No arguments with prefix -> prefix pathspec */
 	if (!entry) {
 		if (flags & PATHSPEC_PREFER_FULL)
@@ -629,7 +636,7 @@ void parse_pathspec(struct pathspec *pathspec,
 	for (i = 0; i < n; i++) {
 		entry = argv[i];
 
-		init_pathspec_item(item + i, &the_index, flags,
+		init_pathspec_item(item + i, istate, flags,
 				   prefix, prefixlen, entry);
 
 		if (item[i].magic & PATHSPEC_EXCLUDE)
@@ -643,7 +650,7 @@ void parse_pathspec(struct pathspec *pathspec,
 		}
 
 		if (flags & PATHSPEC_SUBMODULE_LEADING_PATH)
-			die_path_inside_submodule(item + i, &the_index);
+			die_path_inside_submodule(item + i, istate);
 
 		if (item[i].nowildcard_len < item[i].len)
 			pathspec->has_wildcard = 1;
@@ -656,7 +663,7 @@ void parse_pathspec(struct pathspec *pathspec,
 	 */
 	if (nr_exclude == n) {
 		int plen = (!(flags & PATHSPEC_PREFER_CWD)) ? 0 : prefixlen;
-		init_pathspec_item(item + n, &the_index, 0, prefix, plen, "");
+		init_pathspec_item(item + n, istate, 0, prefix, plen, "");
 		pathspec->nr++;
 	}
 
diff --git a/pathspec.h b/pathspec.h
index dfb22440a..4d1007572 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -78,6 +78,7 @@ struct pathspec {
 #define PATHSPEC_LITERAL_PATH (1<<8)
 
 extern void parse_pathspec(struct pathspec *pathspec,
+			   const struct index_state *istate,
 			   unsigned magic_mask,
 			   unsigned flags,
 			   const char *prefix,
diff --git a/revision.c b/revision.c
index 7ff61ff5f..6efdb28ac 100644
--- a/revision.c
+++ b/revision.c
@@ -1424,7 +1424,8 @@ static void prepare_show_merge(struct rev_info *revs)
 			i++;
 	}
 	clear_pathspec(&revs->prune_data);
-	parse_pathspec(&revs->prune_data, PATHSPEC_ALL_MAGIC & ~PATHSPEC_LITERAL,
+	parse_pathspec(&revs->prune_data, NULL,
+		       PATHSPEC_ALL_MAGIC & ~PATHSPEC_LITERAL,
 		       PATHSPEC_PREFER_FULL | PATHSPEC_LITERAL_PATH, "", prune);
 	revs->limited = 1;
 }
@@ -2276,7 +2277,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		 */
 		ALLOC_GROW(prune_data.path, prune_data.nr + 1, prune_data.alloc);
 		prune_data.path[prune_data.nr++] = NULL;
-		parse_pathspec(&revs->prune_data, 0, 0,
+		parse_pathspec(&revs->prune_data, NULL, 0, 0,
 			       revs->prefix, prune_data.path);
 	}
 
diff --git a/submodule.c b/submodule.c
index 9e0502f25..ba9db9714 100644
--- a/submodule.c
+++ b/submodule.c
@@ -252,7 +252,7 @@ int is_submodule_initialized(const char *path)
 			argv_array_push(&args, item->string);
 		}
 
-		parse_pathspec(&ps, 0, 0, NULL, args.argv);
+		parse_pathspec(&ps, NULL, 0, 0, NULL, args.argv);
 		ret = match_pathspec(&ps, path, strlen(path), 0, NULL, 1);
 
 		argv_array_clear(&args);
diff --git a/tree-diff.c b/tree-diff.c
index e164e532b..58c66b5bc 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -641,7 +641,7 @@ static void try_to_follow_renames(const unsigned char *old, const unsigned char
 			path[0] = p->one->path;
 			path[1] = NULL;
 			clear_pathspec(&opt->pathspec);
-			parse_pathspec(&opt->pathspec,
+			parse_pathspec(&opt->pathspec, NULL,
 				       PATHSPEC_ALL_MAGIC & ~PATHSPEC_LITERAL,
 				       PATHSPEC_LITERAL_PATH, "", path);
 
-- 
2.13.0.rc2.291.g57267f2277-goog

Re: [PATCH 8/8] pathspec: convert parse_pathspec to take an index

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> Convert 'parse_pathspec()' to take an index parameter.
>
> Since the index is only needed when the PATHSPEC_SUBMODULE_LEADING_PATH
> and PATHSPEC_STRIP_SUBMODULE_SLASH flags are given, add a check
> requiring a non-NULL index when either of these flags are given.
> Convert callers which use these two flags to pass in an index while
> having other callers pass in NULL.
>
> Now that pathspec.c does not use any cache macros and has no references
> to 'the_index', mark it by defining NO_THE_INDEX_COMPATIBILITY_MACROS.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>

The same comment as 5/8 applies to this change, but it is a bit
easier to judge, because it has so many callers, and for some
builtins, especially manipulator commands like add, checkout, and
commit, there may be a good reason why they want to keep the primary
index while playing with an additional in-core index in a distant
future.

Does a pathspec parsed using one instance of index_state expected to
work when matching against a path in another instance of index_state?
Otherwise, passing a non-NULL istate to parse_pathspec() would tie
the resulting pathspec to a particular index_state in some way and
there may need a mechanism to catch an attempt to match paths in
another index_state with such a pathspec as an error.  Just
speculating out loud...

Re: [PATCH 8/8] pathspec: convert parse_pathspec to take an index

[Brandon Williams]

On 05/10, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > Convert 'parse_pathspec()' to take an index parameter.
> >
> > Since the index is only needed when the PATHSPEC_SUBMODULE_LEADING_PATH
> > and PATHSPEC_STRIP_SUBMODULE_SLASH flags are given, add a check
> > requiring a non-NULL index when either of these flags are given.
> > Convert callers which use these two flags to pass in an index while
> > having other callers pass in NULL.
> >
> > Now that pathspec.c does not use any cache macros and has no references
> > to 'the_index', mark it by defining NO_THE_INDEX_COMPATIBILITY_MACROS.
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> 
> The same comment as 5/8 applies to this change, but it is a bit
> easier to judge, because it has so many callers, and for some
> builtins, especially manipulator commands like add, checkout, and
> commit, there may be a good reason why they want to keep the primary
> index while playing with an additional in-core index in a distant
> future.
> 
> Does a pathspec parsed using one instance of index_state expected to
> work when matching against a path in another instance of index_state?
> Otherwise, passing a non-NULL istate to parse_pathspec() would tie
> the resulting pathspec to a particular index_state in some way and
> there may need a mechanism to catch an attempt to match paths in
> another index_state with such a pathspec as an error.  Just
> speculating out loud...
> 

Currently I don't believe this links a pathspec with a particular
index_state since an index is really just used to do some pre-processing
on the paths (check if the path goes into a submodule and die, or strip
a slash if the path matches a submodule), though I can see a future where
this is true.

I did have another version of this series where I completely removed the
two flags related to submodules.  Since builtin/add is the only caller
which needs to die if a path descends into a submodule, I had a function
which did this after the parse_pathspec() call.  Also, since (ae8d08242
pathspec: pass directory indicator to match_pathspec_item()) stripping
off the slash from a submodule path really is no longer needed for the
path matching logic, this means that we could potentially just drop the
strip slash flag.  The only caveat is ls-files.

ls-files is the only command (that I know of) which does cache pruning
based on the common prefix of all pathspecs given.  If there is a
submodule named 'sub' and you provided a pathspec 'sub/', the matching
logic can handle this but the cache pruning logic would prune all
entries from the index which don't have a leading 'sub/' which is the
common prefix of all pathspecs (if you didn't strip off the slash).
Meaning you'd prune the submodule 'sub' when  you really didn't want to.
This could probably be fixed to have the cache pruning logic to prune by
ignoring the trailing slash always.

So there's another option here if you don't feel quite right about
piping through an index into parse_pathspec just yet.

-- 
Brandon Williams

Re: [PATCH 8/8] pathspec: convert parse_pathspec to take an index

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> ls-files is the only command (that I know of) which does cache pruning
> based on the common prefix of all pathspecs given.  If there is a
> submodule named 'sub' and you provided a pathspec 'sub/', the matching
> logic can handle this but the cache pruning logic would prune all
> entries from the index which don't have a leading 'sub/' which is the
> common prefix of all pathspecs (if you didn't strip off the slash).
> Meaning you'd prune the submodule 'sub' when  you really didn't want to.
> This could probably be fixed to have the cache pruning logic to prune by
> ignoring the trailing slash always.
>
> So there's another option here if you don't feel quite right about
> piping through an index into parse_pathspec just yet.

Oh, don't get me wrong---I do not think passing an index_state
instance throughout the callchain (and perhaps later we may pass an
instance of "repository" instead) is a wrong move in the longer term
for various APIs.  I was just wondering if we have callers that can
benefit from this change immediately---manipulators like "commit" do
already use multiple instances of index_state structure.

But perhaps you are right---it may be wrong for the contents of the
current index (or any index) to affect how a pathspec element is
parsed in the first place.  If the current code (before this series)
uses the_index only for error checking, we may want to separate that
out of the parse_pathspec() callchain, so that it does not even look
at any index (not just the_index).  And that may be a better change
overall.

Thanks.

Re: [PATCH 8/8] pathspec: convert parse_pathspec to take an index

[Johannes Sixt]

Am 11.05.2017 um 03:48 schrieb Junio C Hamano:
> But perhaps you are right---it may be wrong for the contents of the
> current index (or any index) to affect how a pathspec element is
> parsed in the first place.  If the current code (before this series)
> uses the_index only for error checking, we may want to separate that
> out of the parse_pathspec() callchain, so that it does not even look
> at any index (not just the_index).  And that may be a better change
> overall.

Just a reminder: if core.ignoreCase is set, the variant of a path in the 
index takes precedence over the variant found in the working tree. 
Hence, pathspec must be matched against the index that corresponds to 
the working tree. I guess that's the_index.

-- Hannes

Re: [PATCH 8/8] pathspec: convert parse_pathspec to take an index

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 11.05.2017 um 03:48 schrieb Junio C Hamano:
>> But perhaps you are right---it may be wrong for the contents of the
>> current index (or any index) to affect how a pathspec element is
>> parsed in the first place.  If the current code (before this series)
>> uses the_index only for error checking, we may want to separate that
>> out of the parse_pathspec() callchain, so that it does not even look
>> at any index (not just the_index).  And that may be a better change
>> overall.
>
> Just a reminder: if core.ignoreCase is set, the variant of a path in
> the index takes precedence over the variant found in the working
> tree. Hence, pathspec must be matched against the index that
> corresponds to the working tree. I guess that's the_index.

Yes, but that is what happens after a path from a working tree is
found to match a pathspec, to coerse its case into the one that is
in the current index.  The parse_pathspec() thing we are discussing
would have finished looooong time before that actual "matching" is
attempted.

Thanks.

Re: [PATCH 8/8] pathspec: convert parse_pathspec to take an index

[Brandon Williams]

On 05/11, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > ls-files is the only command (that I know of) which does cache pruning
> > based on the common prefix of all pathspecs given.  If there is a
> > submodule named 'sub' and you provided a pathspec 'sub/', the matching
> > logic can handle this but the cache pruning logic would prune all
> > entries from the index which don't have a leading 'sub/' which is the
> > common prefix of all pathspecs (if you didn't strip off the slash).
> > Meaning you'd prune the submodule 'sub' when  you really didn't want to.
> > This could probably be fixed to have the cache pruning logic to prune by
> > ignoring the trailing slash always.
> >
> > So there's another option here if you don't feel quite right about
> > piping through an index into parse_pathspec just yet.
> 
> Oh, don't get me wrong---I do not think passing an index_state
> instance throughout the callchain (and perhaps later we may pass an
> instance of "repository" instead) is a wrong move in the longer term
> for various APIs.  I was just wondering if we have callers that can
> benefit from this change immediately---manipulators like "commit" do
> already use multiple instances of index_state structure.

I didn't get the feeling you thought it was a bad change.  I really
appreciate your thoughts since they are things which I also was
wondering about.

> But perhaps you are right---it may be wrong for the contents of the
> current index (or any index) to affect how a pathspec element is
> parsed in the first place.  If the current code (before this series)
> uses the_index only for error checking, we may want to separate that
> out of the parse_pathspec() callchain, so that it does not even look
> at any index (not just the_index).  And that may be a better change
> overall.

I'll polish up the other version of the series which does the processing
(using an index) outside of parse_pathspec() and let you see how you
feel about those changes.  Note that if we go with the route to not pass
in an index now, it doesn't necessarily mean that down the line we won't
have to pass a 'repository' instance into parse_pathspec().  Just food
for thought.

-- 
Brandon Williams

Re: [PATCH 8/8] pathspec: convert parse_pathspec to take an index

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> ...  Note that if we go with the route to not pass
> in an index now, it doesn't necessarily mean that down the line we won't
> have to pass a 'repository' instance into parse_pathspec().

Correct.  

An attribute annotated pathspec may want to check if the attributes
used in it are used in the repository at all for validation or
optimization purposes, for example.

[PATCH v2 0/6] convert pathspec.c to take an index parameter

[Brandon Williams]

The main difference in v2 is that instead of piping through an index_state
struct into parse_pathspec, I ripped out the logic that needed to access the
index and either removed it completely if it wasn't needed anymore (stripping
submodule slash) or factored it out into its own function which can be called
after initializing a pathspec object (dying if a path descends into a
submodule).

Brandon Williams (6):
  pathspec: provide a more descriptive die message
  submodule: add die_in_unpopulated_submodule function
  pathspec: remove PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag
  ls-files: prevent prune_cache from overeagerly pruning submodules
  pathspec: remove PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP
  pathspec: convert find_pathspecs_matching_against_index to take an
    index

 builtin/add.c                    | 12 ++++--
 builtin/check-ignore.c           |  6 ++-
 builtin/ls-files.c               | 31 ++++++++++++---
 builtin/reset.c                  |  1 -
 builtin/rm.c                     |  3 +-
 builtin/submodule--helper.c      |  3 +-
 pathspec.c                       | 86 ++++------------------------------------
 pathspec.h                       | 25 +++++-------
 submodule.c                      | 63 +++++++++++++++++++++++++++++
 submodule.h                      |  4 ++
 t/t6134-pathspec-in-submodule.sh |  6 +--
 11 files changed, 124 insertions(+), 116 deletions(-)

-- 
2.13.0.rc2.291.g57267f2277-goog

[PATCH v2 1/6] pathspec: provide a more descriptive die message

[Brandon Williams]

The current message displayed upon an internal error in
'init_pathspec_item()' isn't very descriptive and doesn't provide much
context to where the error occurred.  Update the error message to
provide more context to where the error occured.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 pathspec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pathspec.c b/pathspec.c
index 50f76fff4..904cfb739 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -555,7 +555,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
 		 * would trigger that.
 		 */
 		die_inside_submodule_path(item);
-		die ("BUG: item->nowildcard_len > item->len || item->prefix > item->len)");
+		die ("BUG: error initializing pathspec_item");
 	}
 }
 
-- 
2.13.0.rc2.291.g57267f2277-goog

[PATCH v2 2/6] submodule: add die_in_unpopulated_submodule function

[Brandon Williams]

Currently 'git add' is the only command which dies when launched from an
unpopulated submodule (the place-holder directory for a submodule which
hasn't been checked out).  This is triggered implicitly by passing the
PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag to 'parse_pathspec()'.

Instead make this desire more explicit by creating a function
'die_in_unpopulated_submodule()' which dies if the provided 'prefix' has
a leading path component which matches a submodule in the the index.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/add.c                    |  3 +++
 pathspec.c                       | 29 -----------------------------
 submodule.c                      | 30 ++++++++++++++++++++++++++++++
 submodule.h                      |  2 ++
 t/t6134-pathspec-in-submodule.sh |  6 +-----
 5 files changed, 36 insertions(+), 34 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 9f53f020d..ec58e3679 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -17,6 +17,7 @@
 #include "revision.h"
 #include "bulk-checkin.h"
 #include "argv-array.h"
+#include "submodule.h"
 
 static const char * const builtin_add_usage[] = {
 	N_("git add [<options>] [--] <pathspec>..."),
@@ -379,6 +380,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	if (read_cache() < 0)
 		die(_("index file corrupt"));
 
+	die_in_unpopulated_submodule(&the_index, prefix);
+
 	/*
 	 * Check the "pathspec '%s' did not match any files" block
 	 * below before enabling new magic.
diff --git a/pathspec.c b/pathspec.c
index 904cfb739..9b7634c5a 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -424,27 +424,6 @@ static void strip_submodule_slash_expensive(struct pathspec_item *item)
 	}
 }
 
-static void die_inside_submodule_path(struct pathspec_item *item)
-{
-	int i;
-
-	for (i = 0; i < active_nr; i++) {
-		struct cache_entry *ce = active_cache[i];
-		int ce_len = ce_namelen(ce);
-
-		if (!S_ISGITLINK(ce->ce_mode))
-			continue;
-
-		if (item->len < ce_len ||
-		    !(item->match[ce_len] == '/' || item->match[ce_len] == '\0') ||
-		    memcmp(ce->name, item->match, ce_len))
-			continue;
-
-		die(_("Pathspec '%s' is in submodule '%.*s'"),
-		    item->original, ce_len, ce->name);
-	}
-}
-
 /*
  * Perform the initialization of a pathspec_item based on a pathspec element.
  */
@@ -547,14 +526,6 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
 	/* sanity checks, pathspec matchers assume these are sane */
 	if (item->nowildcard_len > item->len ||
 	    item->prefix         > item->len) {
-		/*
-		 * This case can be triggered by the user pointing us to a
-		 * pathspec inside a submodule, which is an input error.
-		 * Detect that here and complain, but fallback in the
-		 * non-submodule case to a BUG, as we have no idea what
-		 * would trigger that.
-		 */
-		die_inside_submodule_path(item);
 		die ("BUG: error initializing pathspec_item");
 	}
 }
diff --git a/submodule.c b/submodule.c
index d3299e29c..885663c42 100644
--- a/submodule.c
+++ b/submodule.c
@@ -282,6 +282,36 @@ int is_submodule_populated_gently(const char *path, int *return_error_code)
 	return ret;
 }
 
+/*
+ * Dies if the provided 'prefix' corresponds to an unpopulated submodule
+ */
+void die_in_unpopulated_submodule(const struct index_state *istate,
+				  const char *prefix)
+{
+	int i, prefixlen;
+
+	if (!prefix)
+		return;
+
+	prefixlen = strlen(prefix);
+
+	for (i = 0; i < istate->cache_nr; i++) {
+		struct cache_entry *ce = istate->cache[i];
+		int ce_len = ce_namelen(ce);
+
+		if (!S_ISGITLINK(ce->ce_mode))
+			continue;
+		if (prefixlen <= ce_len)
+			continue;
+		if (strncmp(ce->name, prefix, ce_len))
+			continue;
+		if (prefix[ce_len] != '/')
+			continue;
+
+		die(_("in unpopulated submodule '%s'"), ce->name);
+	}
+}
+
 int parse_submodule_update_strategy(const char *value,
 		struct submodule_update_strategy *dst)
 {
diff --git a/submodule.h b/submodule.h
index 1277480ad..d11b4da40 100644
--- a/submodule.h
+++ b/submodule.h
@@ -49,6 +49,8 @@ extern int is_submodule_initialized(const char *path);
  * Otherwise the return error code is the same as of resolve_gitdir_gently.
  */
 extern int is_submodule_populated_gently(const char *path, int *return_error_code);
+extern void die_in_unpopulated_submodule(const struct index_state *istate,
+					 const char *prefix);
 extern int parse_submodule_update_strategy(const char *value,
 		struct submodule_update_strategy *dst);
 extern const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);
diff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh
index fd401ca60..0f1cb49ce 100755
--- a/t/t6134-pathspec-in-submodule.sh
+++ b/t/t6134-pathspec-in-submodule.sh
@@ -24,13 +24,9 @@ test_expect_success 'error message for path inside submodule' '
 	test_cmp expect actual
 '
 
-cat <<EOF >expect
-fatal: Pathspec '.' is in submodule 'sub'
-EOF
-
 test_expect_success 'error message for path inside submodule from within submodule' '
 	test_must_fail git -C sub add . 2>actual &&
-	test_cmp expect actual
+	test_i18ngrep "in unpopulated submodule" actual
 '
 
 test_done
-- 
2.13.0.rc2.291.g57267f2277-goog

[PATCH v2 3/6] pathspec: remove PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag

[Brandon Williams]

Since (ae8d08242 pathspec: pass directory indicator to
match_pathspec_item()) the path matching logic has been able to cope
with submodules without needing to strip off a trailing slash if a path
refers to a submodule.

Since the stripping the trailing slash is no longer necessary, remove
the PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag.  In addition, factor
out the logic which dies if a path decends into a submodule so that it
can still be used as a check after a pathspec struct has been
initialized.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/add.c          |  5 +++--
 builtin/check-ignore.c |  4 +++-
 pathspec.c             | 29 -----------------------------
 pathspec.h             | 14 +++-----------
 submodule.c            | 33 +++++++++++++++++++++++++++++++++
 submodule.h            |  2 ++
 6 files changed, 44 insertions(+), 43 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index ec58e3679..86770d6af 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -388,10 +388,11 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	 */
 	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_FULL |
-		       PATHSPEC_SYMLINK_LEADING_PATH |
-		       PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE,
+		       PATHSPEC_SYMLINK_LEADING_PATH,
 		       prefix, argv);
 
+	die_path_inside_submodule(&the_index, &pathspec);
+
 	if (add_new_files) {
 		int baselen;
 
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 1d73d3ca3..91040a4b0 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -4,6 +4,7 @@
 #include "quote.h"
 #include "pathspec.h"
 #include "parse-options.h"
+#include "submodule.h"
 
 static int quiet, verbose, stdin_paths, show_non_matching, no_index;
 static const char * const check_ignore_usage[] = {
@@ -87,10 +88,11 @@ static int check_ignore(struct dir_struct *dir,
 	parse_pathspec(&pathspec,
 		       PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP,
 		       PATHSPEC_SYMLINK_LEADING_PATH |
-		       PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE |
 		       PATHSPEC_KEEP_ORDER,
 		       prefix, argv);
 
+	die_path_inside_submodule(&the_index, &pathspec);
+
 	/*
 	 * look for pathspecs matching entries in the index, since these
 	 * should not be ignored, in order to be consistent with
diff --git a/pathspec.c b/pathspec.c
index 9b7634c5a..e42431278 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -398,32 +398,6 @@ static void strip_submodule_slash_cheap(struct pathspec_item *item)
 	}
 }
 
-static void strip_submodule_slash_expensive(struct pathspec_item *item)
-{
-	int i;
-
-	for (i = 0; i < active_nr; i++) {
-		struct cache_entry *ce = active_cache[i];
-		int ce_len = ce_namelen(ce);
-
-		if (!S_ISGITLINK(ce->ce_mode))
-			continue;
-
-		if (item->len <= ce_len || item->match[ce_len] != '/' ||
-		    memcmp(ce->name, item->match, ce_len))
-			continue;
-
-		if (item->len == ce_len + 1) {
-			/* strip trailing slash */
-			item->len--;
-			item->match[item->len] = '\0';
-		} else {
-			die(_("Pathspec '%s' is in submodule '%.*s'"),
-			    item->original, ce_len, ce->name);
-		}
-	}
-}
-
 /*
  * Perform the initialization of a pathspec_item based on a pathspec element.
  */
@@ -499,9 +473,6 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
 	if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
 		strip_submodule_slash_cheap(item);
 
-	if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
-		strip_submodule_slash_expensive(item);
-
 	if (magic & PATHSPEC_LITERAL) {
 		item->nowildcard_len = item->len;
 	} else {
diff --git a/pathspec.h b/pathspec.h
index 55e976972..3729efa85 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -62,23 +62,15 @@ struct pathspec {
 #define PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP (1<<3)
 /* die if a symlink is part of the given path's directory */
 #define PATHSPEC_SYMLINK_LEADING_PATH (1<<4)
-/*
- * This is like a combination of ..LEADING_PATH and .._SLASH_CHEAP
- * (but not the same): it strips the trailing slash if the given path
- * is a gitlink but also checks and dies if gitlink is part of the
- * leading path (i.e. the given path goes beyond a submodule). It's
- * safer than _SLASH_CHEAP and also more expensive.
- */
-#define PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE (1<<5)
-#define PATHSPEC_PREFIX_ORIGIN (1<<6)
-#define PATHSPEC_KEEP_ORDER (1<<7)
+#define PATHSPEC_PREFIX_ORIGIN (1<<5)
+#define PATHSPEC_KEEP_ORDER (1<<6)
 /*
  * For the callers that just need pure paths from somewhere else, not
  * from command line. Global --*-pathspecs options are ignored. No
  * magic is parsed in each pathspec either. If PATHSPEC_LITERAL is
  * allowed, then it will automatically set for every pathspec.
  */
-#define PATHSPEC_LITERAL_PATH (1<<8)
+#define PATHSPEC_LITERAL_PATH (1<<7)
 
 extern void parse_pathspec(struct pathspec *pathspec,
 			   unsigned magic_mask,
diff --git a/submodule.c b/submodule.c
index 885663c42..80851d044 100644
--- a/submodule.c
+++ b/submodule.c
@@ -312,6 +312,39 @@ void die_in_unpopulated_submodule(const struct index_state *istate,
 	}
 }
 
+/*
+ * Dies if any paths in the provided pathspec descends into a submodule
+ */
+void die_path_inside_submodule(const struct index_state *istate,
+			       const struct pathspec *ps)
+{
+	int i, j;
+
+	for (i = 0; i < istate->cache_nr; i++) {
+		struct cache_entry *ce = istate->cache[i];
+		int ce_len = ce_namelen(ce);
+
+		if (!S_ISGITLINK(ce->ce_mode))
+			continue;
+
+		for (j = 0; j < ps->nr ; j++) {
+			const struct pathspec_item *item = &ps->items[j];
+
+			if (item->len <= ce_len)
+				continue;
+			if (item->match[ce_len] != '/')
+				continue;
+			if (strncmp(ce->name, item->match, ce_len))
+				continue;
+			if (item->len == ce_len + 1)
+				continue;
+
+			die(_("Pathspec '%s' is in submodule '%.*s'"),
+			    item->original, ce_len, ce->name);
+		}
+	}
+}
+
 int parse_submodule_update_strategy(const char *value,
 		struct submodule_update_strategy *dst)
 {
diff --git a/submodule.h b/submodule.h
index d11b4da40..266d81f1c 100644
--- a/submodule.h
+++ b/submodule.h
@@ -51,6 +51,8 @@ extern int is_submodule_initialized(const char *path);
 extern int is_submodule_populated_gently(const char *path, int *return_error_code);
 extern void die_in_unpopulated_submodule(const struct index_state *istate,
 					 const char *prefix);
+extern void die_path_inside_submodule(const struct index_state *istate,
+				      const struct pathspec *ps);
 extern int parse_submodule_update_strategy(const char *value,
 		struct submodule_update_strategy *dst);
 extern const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);
-- 
2.13.0.rc2.291.g57267f2277-goog

[PATCH v2 4/6] ls-files: prevent prune_cache from overeagerly pruning submodules

[Brandon Williams]

Since (ae8d08242 pathspec: pass directory indicator to
match_pathspec_item()) the path matching logic has been able to cope
with submodules without needing to strip off a trailing slash if a path
refers to a submodule.

ls-files is the only caller of 'parse_pathspec()' which relies on the
behavior of the PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag because it
uses the result to construct a common prefix of all provided pathspecs
which is then used to prune the index of all entries which don't have
that prefix.  Since submodules entries in the index don't have a
trailing slash 'prune_cache()' will be overeager and prune a submodule
'sub' if the common prefix is 'sub/'.  To correct this behavior, only
prune entries which don't match up to, but not including, a trailing
slash of the common prefix.

This is in preparation to remove the
PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag in a later patch.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/ls-files.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index a6c70dbe9..1f3d47844 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -97,7 +97,7 @@ static void show_dir_entry(const char *tag, struct dir_entry *ent)
 {
 	int len = max_prefix_len;
 
-	if (len >= ent->len)
+	if (len > ent->len)
 		die("git ls-files: internal error - directory entry not superset of prefix");
 
 	if (!dir_path_match(ent, &pathspec, len, ps_matched))
@@ -238,7 +238,7 @@ static void show_ce_entry(const char *tag, const struct cache_entry *ce)
 		strbuf_addstr(&name, super_prefix);
 	strbuf_addstr(&name, ce->name);
 
-	if (len >= ce_namelen(ce))
+	if (len > ce_namelen(ce))
 		die("git ls-files: internal error - cache entry not superset of prefix");
 
 	if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&
@@ -403,6 +403,25 @@ static void prune_cache(const char *prefix, size_t prefixlen)
 	active_nr = last - pos;
 }
 
+static int get_common_prefix_len(const char *common_prefix)
+{
+	int common_prefix_len;
+
+	if (!common_prefix)
+		return 0;
+
+	common_prefix_len = strlen(common_prefix);
+
+	/*
+	 * If the prefix has a trailing slash, strip it so that submodules wont
+	 * be pruned from the index.
+	 */
+	if (common_prefix[common_prefix_len - 1] == '/')
+		common_prefix_len--;
+
+	return common_prefix_len;
+}
+
 /*
  * Read the tree specified with --with-tree option
  * (typically, HEAD) into stage #1 and then
@@ -624,8 +643,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		    "--error-unmatch");
 
 	parse_pathspec(&pathspec, 0,
-		       PATHSPEC_PREFER_CWD |
-		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
+		       PATHSPEC_PREFER_CWD,
 		       prefix, argv);
 
 	/*
@@ -637,7 +655,9 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		max_prefix = NULL;
 	else
 		max_prefix = common_prefix(&pathspec);
-	max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
+	max_prefix_len = get_common_prefix_len(max_prefix);
+
+	prune_cache(max_prefix, max_prefix_len);
 
 	/* Treat unmatching pathspec elements as errors */
 	if (pathspec.nr && error_unmatch)
@@ -651,7 +671,6 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	      show_killed || show_modified || show_resolve_undo))
 		show_cached = 1;
 
-	prune_cache(max_prefix, max_prefix_len);
 	if (with_tree) {
 		/*
 		 * Basic sanity check; show-stages and show-unmerged
-- 
2.13.0.rc2.291.g57267f2277-goog

[PATCH v2 5/6] pathspec: remove PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP

[Brandon Williams]

Since (ae8d08242 pathspec: pass directory indicator to
match_pathspec_item()) the path matching logic has been able to cope
with submodules without needing to strip off a trailing slash if a path
refers to a submodule.

Since stripping the slash is no longer necessary, remove the
PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/reset.c             |  1 -
 builtin/rm.c                |  3 +--
 builtin/submodule--helper.c |  3 +--
 pathspec.c                  | 15 ---------------
 pathspec.h                  | 10 ++++------
 5 files changed, 6 insertions(+), 26 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index fc3b906c4..5db2adc4c 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -236,7 +236,6 @@ static void parse_args(struct pathspec *pathspec,
 
 	parse_pathspec(pathspec, 0,
 		       PATHSPEC_PREFER_FULL |
-		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |
 		       (patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0),
 		       prefix, argv);
 }
diff --git a/builtin/rm.c b/builtin/rm.c
index fb79dcab1..7c323d012 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -271,8 +271,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 		die(_("index file corrupt"));
 
 	parse_pathspec(&pathspec, 0,
-		       PATHSPEC_PREFER_CWD |
-		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
+		       PATHSPEC_PREFER_CWD,
 		       prefix, argv);
 	refresh_index(&the_index, REFRESH_QUIET, &pathspec, NULL, NULL);
 
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 566a5b6a6..8cc648d85 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -233,8 +233,7 @@ static int module_list_compute(int argc, const char **argv,
 	int i, result = 0;
 	char *ps_matched = NULL;
 	parse_pathspec(pathspec, 0,
-		       PATHSPEC_PREFER_FULL |
-		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
+		       PATHSPEC_PREFER_FULL,
 		       prefix, argv);
 
 	if (pathspec->nr)
diff --git a/pathspec.c b/pathspec.c
index e42431278..1e5df2316 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -386,18 +386,6 @@ static const char *parse_element_magic(unsigned *magic, int *prefix_len,
 		return parse_short_magic(magic, elem);
 }
 
-static void strip_submodule_slash_cheap(struct pathspec_item *item)
-{
-	if (item->len >= 1 && item->match[item->len - 1] == '/') {
-		int i = cache_name_pos(item->match, item->len - 1);
-
-		if (i >= 0 && S_ISGITLINK(active_cache[i]->ce_mode)) {
-			item->len--;
-			item->match[item->len] = '\0';
-		}
-	}
-}
-
 /*
  * Perform the initialization of a pathspec_item based on a pathspec element.
  */
@@ -470,9 +458,6 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
 		item->original = xstrdup(elt);
 	}
 
-	if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
-		strip_submodule_slash_cheap(item);
-
 	if (magic & PATHSPEC_LITERAL) {
 		item->nowildcard_len = item->len;
 	} else {
diff --git a/pathspec.h b/pathspec.h
index 3729efa85..6671b9577 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -58,19 +58,17 @@ struct pathspec {
 #define PATHSPEC_PREFER_CWD (1<<0) /* No args means match cwd */
 #define PATHSPEC_PREFER_FULL (1<<1) /* No args means match everything */
 #define PATHSPEC_MAXDEPTH_VALID (1<<2) /* max_depth field is valid */
-/* strip the trailing slash if the given path is a gitlink */
-#define PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP (1<<3)
 /* die if a symlink is part of the given path's directory */
-#define PATHSPEC_SYMLINK_LEADING_PATH (1<<4)
-#define PATHSPEC_PREFIX_ORIGIN (1<<5)
-#define PATHSPEC_KEEP_ORDER (1<<6)
+#define PATHSPEC_SYMLINK_LEADING_PATH (1<<3)
+#define PATHSPEC_PREFIX_ORIGIN (1<<4)
+#define PATHSPEC_KEEP_ORDER (1<<5)
 /*
  * For the callers that just need pure paths from somewhere else, not
  * from command line. Global --*-pathspecs options are ignored. No
  * magic is parsed in each pathspec either. If PATHSPEC_LITERAL is
  * allowed, then it will automatically set for every pathspec.
  */
-#define PATHSPEC_LITERAL_PATH (1<<7)
+#define PATHSPEC_LITERAL_PATH (1<<6)
 
 extern void parse_pathspec(struct pathspec *pathspec,
 			   unsigned magic_mask,
-- 
2.13.0.rc2.291.g57267f2277-goog

[PATCH v2 6/6] pathspec: convert find_pathspecs_matching_against_index to take an index

[Brandon Williams]

Convert find_pathspecs_matching_against_index to take an index
parameter.

In addition mark pathspec.c with NO_THE_INDEX_COMPATIBILITY_MACROS now
that it doesn't use any cache macros or reference 'the_index'.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/add.c          |  4 ++--
 builtin/check-ignore.c |  2 +-
 pathspec.c             | 11 +++++++----
 pathspec.h             |  7 +++++--
 4 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 86770d6af..0365a5209 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -136,7 +136,7 @@ static char *prune_directory(struct dir_struct *dir, struct pathspec *pathspec,
 			*dst++ = entry;
 	}
 	dir->nr = dst - dir->entries;
-	add_pathspec_matches_against_index(pathspec, seen);
+	add_pathspec_matches_against_index(pathspec, &the_index, seen);
 	return seen;
 }
 
@@ -418,7 +418,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		int i;
 
 		if (!seen)
-			seen = find_pathspecs_matching_against_index(&pathspec);
+			seen = find_pathspecs_matching_against_index(&pathspec, &the_index);
 
 		/*
 		 * file_exists() assumes exact match
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 91040a4b0..7629018a5 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -98,7 +98,7 @@ static int check_ignore(struct dir_struct *dir,
 	 * should not be ignored, in order to be consistent with
 	 * 'git status', 'git add' etc.
 	 */
-	seen = find_pathspecs_matching_against_index(&pathspec);
+	seen = find_pathspecs_matching_against_index(&pathspec, &the_index);
 	for (i = 0; i < pathspec.nr; i++) {
 		full_path = pathspec.items[i].match;
 		exclude = NULL;
diff --git a/pathspec.c b/pathspec.c
index 1e5df2316..828405021 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -1,3 +1,4 @@
+#define NO_THE_INDEX_COMPATIBILITY_MACROS
 #include "cache.h"
 #include "dir.h"
 #include "pathspec.h"
@@ -17,6 +18,7 @@
  * to use find_pathspecs_matching_against_index() instead.
  */
 void add_pathspec_matches_against_index(const struct pathspec *pathspec,
+					const struct index_state *istate,
 					char *seen)
 {
 	int num_unmatched = 0, i;
@@ -32,8 +34,8 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,
 			num_unmatched++;
 	if (!num_unmatched)
 		return;
-	for (i = 0; i < active_nr; i++) {
-		const struct cache_entry *ce = active_cache[i];
+	for (i = 0; i < istate->cache_nr; i++) {
+		const struct cache_entry *ce = istate->cache[i];
 		ce_path_match(ce, pathspec, seen);
 	}
 }
@@ -46,10 +48,11 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,
  * nature of the "closest" (i.e. most specific) matches which each of the
  * given pathspecs achieves against all items in the index.
  */
-char *find_pathspecs_matching_against_index(const struct pathspec *pathspec)
+char *find_pathspecs_matching_against_index(const struct pathspec *pathspec,
+					    const struct index_state *istate)
 {
 	char *seen = xcalloc(pathspec->nr, 1);
-	add_pathspec_matches_against_index(pathspec, seen);
+	add_pathspec_matches_against_index(pathspec, istate, seen);
 	return seen;
 }
 
diff --git a/pathspec.h b/pathspec.h
index 6671b9577..60e650040 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -96,7 +96,10 @@ static inline int ps_strcmp(const struct pathspec_item *item,
 		return strcmp(s1, s2);
 }
 
-extern char *find_pathspecs_matching_against_index(const struct pathspec *pathspec);
-extern void add_pathspec_matches_against_index(const struct pathspec *pathspec, char *seen);
+extern void add_pathspec_matches_against_index(const struct pathspec *pathspec,
+					       const struct index_state *istate,
+					       char *seen);
+extern char *find_pathspecs_matching_against_index(const struct pathspec *pathspec,
+						   const struct index_state *istate);
 
 #endif /* PATHSPEC_H */
-- 
2.13.0.rc2.291.g57267f2277-goog

Re: [PATCH v2 0/6] convert pathspec.c to take an index parameter

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> The main difference in v2 is that instead of piping through an index_state
> struct into parse_pathspec, I ripped out the logic that needed to access the
> index and either removed it completely if it wasn't needed anymore (stripping
> submodule slash) or factored it out into its own function which can be called
> after initializing a pathspec object (dying if a path descends into a
> submodule).
>
> Brandon Williams (6):
>   pathspec: provide a more descriptive die message
>   submodule: add die_in_unpopulated_submodule function
>   pathspec: remove PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag
>   ls-files: prevent prune_cache from overeagerly pruning submodules
>   pathspec: remove PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP
>   pathspec: convert find_pathspecs_matching_against_index to take an
>     index

This was (perhaps surprisingly) quite a pleasant read, even compared
to the other approach ;-).

Re: [PATCH v2 0/6] convert pathspec.c to take an index parameter

[Brandon Williams]

On 05/12, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > The main difference in v2 is that instead of piping through an index_state
> > struct into parse_pathspec, I ripped out the logic that needed to access the
> > index and either removed it completely if it wasn't needed anymore (stripping
> > submodule slash) or factored it out into its own function which can be called
> > after initializing a pathspec object (dying if a path descends into a
> > submodule).
> >
> > Brandon Williams (6):
> >   pathspec: provide a more descriptive die message
> >   submodule: add die_in_unpopulated_submodule function
> >   pathspec: remove PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag
> >   ls-files: prevent prune_cache from overeagerly pruning submodules
> >   pathspec: remove PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP
> >   pathspec: convert find_pathspecs_matching_against_index to take an
> >     index
> 
> This was (perhaps surprisingly) quite a pleasant read, even compared
> to the other approach ;-).

Glad you think so! Thanks!

-- 
Brandon Williams