convert pathspec.c to take an index parameter

PATCH Superseded Brandon Williams 3 participants 29 messages May 9, 2017 – May 12, 2017

Share & tools

Brandon Williams

author

2017-05-09 19:18 UTC #0 lore raw

cover convert pathspec.c to take an index parameter

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(-)

Signature · 1 line

2.13.0.rc2.291.g57267f2277-goog

Brandon Williams

author

2017-05-09 19:18 UTC #1 lore raw

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

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 <[email protected]>

---
 pathspec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

pathspec.c +1 -1

index 303efda83..61b5b1273 100644
@@ -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");
 	}
 }

Signature · 1 line

2.13.0.rc2.291.g57267f2277-goog

Brandon Williams

author

2017-05-09 19:18 UTC #2 lore raw

PATCH 2/8 submodule: add die_in_unpopulated_submodule function

In reply to 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 <[email protected]>

---
 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(-)

builtin/add.c +3 -0

index 9f53f020d..ec58e3679 100644
@@ -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.

pathspec.c +0 ~~-29~~

index 61b5b1273..e37256c3b 100644
@@ -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");
 	}
 }

submodule.c +30 -0

index 7c3c4b17f..9e0502f25 100644
@@ -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)
 {

submodule.h +2 -0

index 8a8bc49dc..f4fe6374d 100644
@@ -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);

t/t6134-pathspec-in-submodule.sh +1 -5

index fd401ca60..0f1cb49ce 100755
@@ -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

Signature · 1 line

2.13.0.rc2.291.g57267f2277-goog

Brandon Williams

author

2017-05-09 19:18 UTC #3 lore raw

PATCH 3/8 pathspec: change PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag

In reply to 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 <[email protected]>

---
 builtin/add.c          |  3 ++-
 builtin/check-ignore.c |  3 ++-
 pathspec.c             | 32 ++++++++++++++++----------------
 pathspec.h             |  9 +++------
 4 files changed, 23 insertions(+), 24 deletions(-)

builtin/add.c +2 -1

index ec58e3679..2aa9aeab9 100644
@@ -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) {

builtin/check-ignore.c +2 -1

index 1d73d3ca3..73237b2b1 100644
@@ -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);

pathspec.c +16 ~~-16~~

index e37256c3b..f37d5769d 100644
@@ -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;

pathspec.h +3 -6

index 55e976972..93a819cbf 100644
@@ -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)
 /*

Signature · 1 line

2.13.0.rc2.291.g57267f2277-goog

Junio C Hamano

2017-05-10 05:52 UTC #4 lore raw

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

Quoted 11 lines · 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.

Quoted 5 lines

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.

Brandon Williams

author

2017-05-10 16:16 UTC #5 lore raw

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

Quoted 19 lines · Junio C Hamano

Quoted 11 lines · 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.
Quoted 5 lines
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.

Quoted 6 lines

"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.

Signature · 1 line

Brandon Williams

Brandon Williams

author

2017-05-09 19:18 UTC #6 lore raw

PATCH 4/8 pathspec: rename PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP

In reply to 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 <[email protected]>

---
 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(-)

builtin/add.c +1 -1

index 2aa9aeab9..51d7a5506 100644
@@ -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);

builtin/check-ignore.c +1 -1

index 73237b2b1..694e4c61b 100644
@@ -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);

builtin/ls-files.c +1 -1

index d449e46db..e9dee2e41 100644
@@ -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);
 
 	/*

builtin/reset.c +1 -1

index fc3b906c4..529f2f9be 100644
@@ -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);
 }

builtin/rm.c +1 -1

index fb79dcab1..8fe12d0a7 100644
@@ -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);

builtin/submodule--helper.c +1 -1

index 85aafe46a..69149b557 100644
@@ -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)

pathspec.c +3 -3

index f37d5769d..a1297ca02 100644
@@ -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;

pathspec.h +1 -1

index 93a819cbf..dd6456d5d 100644
@@ -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)
 /*

Signature · 1 line

2.13.0.rc2.291.g57267f2277-goog

Junio C Hamano

2017-05-10 05:53 UTC #7 lore raw

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

Quoted 3 lines · 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.

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

Brandon Williams

author

2017-05-09 19:18 UTC #8 lore raw

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

In reply to Brandon Williams

Signed-off-by Brandon Williams <[email protected]>

---
 pathspec.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

pathspec.c +5 -4

index a1297ca02..cff069536 100644
@@ -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;

Signature · 1 line

2.13.0.rc2.291.g57267f2277-goog

Junio C Hamano

2017-05-10 05:55 UTC #9 lore raw

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

Quoted 4 lines · Brandon Williams

Signed-off-by Brandon Williams <[email protected]>
---
 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.

Brandon Williams

author

2017-05-09 19:18 UTC #10 lore raw

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

In reply to Brandon Williams

Signed-off-by Brandon Williams <[email protected]>

---
 builtin/add.c          |  4 ++--
 builtin/check-ignore.c |  2 +-
 pathspec.c             | 10 ++++++----
 pathspec.h             |  7 +++++--
 4 files changed, 14 insertions(+), 9 deletions(-)

builtin/add.c +2 -2

index 51d7a5506..4e3bf20c2 100644
@@ -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

builtin/check-ignore.c +1 -1

index 694e4c61b..446b76dcf 100644
@@ -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;

pathspec.c +6 -4

index cff069536..bbd71b48b 100644
@@ -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;
 }

pathspec.h +5 -2

index dd6456d5d..dfb22440a 100644
@@ -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 */

Signature · 1 line

2.13.0.rc2.291.g57267f2277-goog

Brandon Williams

author

2017-05-09 19:18 UTC #11 lore raw

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

In reply to Brandon Williams

Signed-off-by Brandon Williams <[email protected]>

---
 pathspec.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

pathspec.c +7 -4

index bbd71b48b..c7ed8b3fb 100644
@@ -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++;
 	}

Signature · 1 line

2.13.0.rc2.291.g57267f2277-goog

Brandon Williams

author

2017-05-09 19:18 UTC #12 lore raw

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

In reply to 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 <[email protected]>

---
 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(-)

archive.c +2 -2

index 60b889198..ce9b30f2e 100644
@@ -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;

builtin/add.c +2 -2

index 4e3bf20c2..23606db39 100644
@@ -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 |

builtin/blame.c +1 -1

index f7aa95f4b..e37837c17 100644
@@ -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);

builtin/check-ignore.c +1 -1

index 446b76dcf..90169e79a 100644
@@ -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 |

builtin/checkout.c +1 -1

index bfa5419f3..cb8ed09f6 100644
@@ -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);

builtin/clean.c +1 -1

index d861f836a..ebc980b4f 100644
@@ -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);

builtin/commit.c +2 -2

index 4e288bc51..014193728 100644
@@ -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);

builtin/grep.c +1 -1

index 65070c52f..d330a85cb 100644
@@ -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);

builtin/ls-files.c +2 -2

index e9dee2e41..f5d81ab1d 100644
@@ -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);

builtin/ls-tree.c +3 -2

index d7ebeb4ce..1d1083ed7 100644
@@ -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++)

builtin/rerere.c +1 -1

index 1bf72423b..749599a9c 100644
@@ -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);
 	}

builtin/reset.c +1 -1

index 529f2f9be..f9da86622 100644
@@ -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),

builtin/rm.c +1 -1

index 8fe12d0a7..cffd27c20 100644
@@ -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);

builtin/submodule--helper.c +1 -1

index 69149b557..b06d62d80 100644
@@ -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);

builtin/update-index.c +1 -1

index ebfc09faa..0aaf7e0da 100644
@@ -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);

line-log.c +1 -1

index a23b91047..4c3232639 100644
@@ -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);

pathspec.c +10 -3

index c7ed8b3fb..b9c87b8d9 100644
@@ -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++;
 	}

pathspec.h +1 -0

index dfb22440a..4d1007572 100644
@@ -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,

revision.c +3 -2

index 7ff61ff5f..6efdb28ac 100644
@@ -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);
 	}

submodule.c +1 -1

index 9e0502f25..ba9db9714 100644
@@ -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);

tree-diff.c +1 -1

index e164e532b..58c66b5bc 100644
@@ -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);

Signature · 1 line

2.13.0.rc2.291.g57267f2277-goog

Junio C Hamano

2017-05-10 06:04 UTC #13 lore raw

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

Quoted 8 lines · 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.

Quoted 4 lines

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 <[email protected]>

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...

Brandon Williams

author

2017-05-10 17:02 UTC #14 lore raw

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

Quoted 22 lines · Junio C Hamano

Quoted 8 lines · 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.
Quoted 4 lines
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 <[email protected]>
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.

Quoted 8 lines

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.

Signature · 1 line

Brandon Williams

Junio C Hamano

2017-05-11 01:48 UTC #15 lore raw

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

Quoted 10 lines · Brandon Williams

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.

Quoted 2 lines

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.

Johannes Sixt

2017-05-11 05:04 UTC #16 lore raw

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

Am 11.05.2017 um 03:48 schrieb Junio C Hamano:

Quoted 7 lines

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

Junio C Hamano

2017-05-11 05:13 UTC #17 lore raw

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

Quoted 9 lines · Johannes Sixt

Am 11.05.2017 um 03:48 schrieb Junio C Hamano:
Quoted 7 lines
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.

Quoted 4 lines

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.

Brandon Williams

author

2017-05-11 17:36 UTC #18 lore raw

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

In reply to Junio C Hamano

Quoted 15 lines · Junio C Hamano

Quoted 10 lines · Brandon Williams
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.
Quoted 2 lines
So there's another option here if you don't feel quite right about piping through an index into parse_pathspec just yet.

Quoted 6 lines

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.

Quoted 7 lines

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.

Signature · 1 line

Brandon Williams

Junio C Hamano

2017-05-12 00:54 UTC #19 lore raw

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

Quoted 3 lines · Brandon Williams

... 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.

Brandon Williams

author

2017-05-11 22:04 UTC #20 lore raw

v2 cover convert pathspec.c to take an index parameter

In reply to 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(-)

Signature · 1 line

2.13.0.rc2.291.g57267f2277-goog

Brandon Williams

author

2017-05-11 22:05 UTC #21 lore raw

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

Signed-off-by Brandon Williams <[email protected]>

---
 pathspec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

pathspec.c +1 -1

index 50f76fff4..904cfb739 100644
@@ -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");
 	}
 }

Signature · 1 line

2.13.0.rc2.291.g57267f2277-goog

Brandon Williams

author

2017-05-11 22:04 UTC #22 lore raw

PATCH v2 2/6 submodule: add die_in_unpopulated_submodule function

In reply to Brandon Williams

Signed-off-by Brandon Williams <[email protected]>

---
 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(-)

builtin/add.c +3 -0

index 9f53f020d..ec58e3679 100644
@@ -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.

pathspec.c +0 ~~-29~~

index 904cfb739..9b7634c5a 100644
@@ -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");
 	}
 }

submodule.c +30 -0

index d3299e29c..885663c42 100644
@@ -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)
 {

submodule.h +2 -0

index 1277480ad..d11b4da40 100644
@@ -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);

t/t6134-pathspec-in-submodule.sh +1 -5

index fd401ca60..0f1cb49ce 100755
@@ -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

Signature · 1 line

2.13.0.rc2.291.g57267f2277-goog

Brandon Williams

author

2017-05-11 22:04 UTC #23 lore raw

PATCH v2 3/6 pathspec: remove PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag

In reply to 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 <[email protected]>

---
 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(-)

builtin/add.c +3 -2

index ec58e3679..86770d6af 100644
@@ -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;

builtin/check-ignore.c +3 -1

index 1d73d3ca3..91040a4b0 100644
@@ -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

pathspec.c +0 ~~-29~~

index 9b7634c5a..e42431278 100644
@@ -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 {

pathspec.h +3 ~~-11~~

index 55e976972..3729efa85 100644
@@ -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,

submodule.c +33 -0

index 885663c42..80851d044 100644
@@ -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)
 {

submodule.h +2 -0

index d11b4da40..266d81f1c 100644
@@ -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);

Signature · 1 line

2.13.0.rc2.291.g57267f2277-goog

Brandon Williams

author

2017-05-11 22:04 UTC #24 lore raw

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

In reply to Brandon Williams

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 <[email protected]>

---
 builtin/ls-files.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

builtin/ls-files.c +25 -6

index a6c70dbe9..1f3d47844 100644
@@ -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

Signature · 1 line

2.13.0.rc2.291.g57267f2277-goog

Brandon Williams

author

2017-05-11 22:05 UTC #25 lore raw

PATCH v2 5/6 pathspec: remove PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP

In reply to Brandon Williams

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

Signed-off-by Brandon Williams <[email protected]>

---
 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(-)

builtin/reset.c +0 -1

index fc3b906c4..5db2adc4c 100644
@@ -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);
 }

builtin/rm.c +1 -2

index fb79dcab1..7c323d012 100644
@@ -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);

builtin/submodule--helper.c +1 -2

index 566a5b6a6..8cc648d85 100644
@@ -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)

pathspec.c +0 ~~-15~~

index e42431278..1e5df2316 100644
@@ -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 {

pathspec.h +4 -6

index 3729efa85..6671b9577 100644
@@ -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,

Signature · 1 line

2.13.0.rc2.291.g57267f2277-goog

Brandon Williams

author

2017-05-11 22:05 UTC #26 lore raw

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

In reply to 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 <[email protected]>

---
 builtin/add.c          |  4 ++--
 builtin/check-ignore.c |  2 +-
 pathspec.c             | 11 +++++++----
 pathspec.h             |  7 +++++--
 4 files changed, 15 insertions(+), 9 deletions(-)

builtin/add.c +2 -2

index 86770d6af..0365a5209 100644
@@ -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

builtin/check-ignore.c +1 -1

index 91040a4b0..7629018a5 100644
@@ -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;

pathspec.c +7 -4

index 1e5df2316..828405021 100644
@@ -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;
 }

pathspec.h +5 -2

index 6671b9577..60e650040 100644
@@ -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 */

Signature · 1 line

2.13.0.rc2.291.g57267f2277-goog

Junio C Hamano

2017-05-12 18:45 UTC #27 lore raw

In reply to Brandon Williams

Quoted 7 lines · 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).

Quoted 8 lines

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 ;-).

Brandon Williams

author

2017-05-12 18:44 UTC #28 lore raw

Quoted 18 lines · Junio C Hamano

Quoted 7 lines · 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).
Quoted 8 lines
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

Quoted 2 lines

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

Glad you think so! Thanks!

Signature · 1 line

Brandon Williams