diff mbox series

[v3,5/8] sparse-checkout: create helper methods

Message ID 5d28570c82af19b4bda4253e72ace3760dfe2606.1629206603.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Sparse index: delete ignored files outside sparse cone | expand

Commit Message

Derrick Stolee Aug. 17, 2021, 1:23 p.m. UTC
From: Derrick Stolee <dstolee@microsoft.com>

As we integrate the sparse index into more builtins, we occasionally
need to check the sparse-checkout patterns to see if a path is within
the sparse-checkout cone. Create some helper methods that help
initialize the patterns and check for pattern matching to make this
easier.

The existing callers of commands like get_sparse_checkout_patterns() use
a custom 'struct pattern_list' that is not necessarily the one in the
'struct index_state', so there are not many previous uses that could
adopt these helpers. There are just two in builtin/add.c and
sparse-index.c that can use path_in_sparse_checkout().

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/add.c  |  8 ++------
 dir.c          | 33 +++++++++++++++++++++++++++++++++
 dir.h          |  6 ++++++
 sparse-index.c | 12 +++---------
 4 files changed, 44 insertions(+), 15 deletions(-)

Comments

Johannes Schindelin Aug. 19, 2021, 8:07 a.m. UTC | #1
Hi Stolee,

On Tue, 17 Aug 2021, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> As we integrate the sparse index into more builtins, we occasionally
> need to check the sparse-checkout patterns to see if a path is within
> the sparse-checkout cone. Create some helper methods that help
> initialize the patterns and check for pattern matching to make this
> easier.
>
> The existing callers of commands like get_sparse_checkout_patterns() use
> a custom 'struct pattern_list' that is not necessarily the one in the
> 'struct index_state', so there are not many previous uses that could
> adopt these helpers. There are just two in builtin/add.c and
> sparse-index.c that can use path_in_sparse_checkout().

Very good!

>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  builtin/add.c  |  8 ++------
>  dir.c          | 33 +++++++++++++++++++++++++++++++++
>  dir.h          |  6 ++++++
>  sparse-index.c | 12 +++---------
>  4 files changed, 44 insertions(+), 15 deletions(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 17528e8f922..f675bdeae4a 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -190,8 +190,6 @@ static int refresh(int verbose, const struct pathspec *pathspec)
>  	struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;
>  	int flags = REFRESH_IGNORE_SKIP_WORKTREE |
>  		    (verbose ? REFRESH_IN_PORCELAIN : REFRESH_QUIET);
> -	struct pattern_list pl = { 0 };
> -	int sparse_checkout_enabled = !get_sparse_checkout_patterns(&pl);
>
>  	seen = xcalloc(pathspec->nr, 1);
>  	refresh_index(&the_index, flags, pathspec, seen,
> @@ -199,12 +197,10 @@ static int refresh(int verbose, const struct pathspec *pathspec)
>  	for (i = 0; i < pathspec->nr; i++) {
>  		if (!seen[i]) {
>  			const char *path = pathspec->items[i].original;
> -			int dtype = DT_REG;
>
>  			if (matches_skip_worktree(pathspec, i, &skip_worktree_seen) ||
> -			    (sparse_checkout_enabled &&
> -			     !path_matches_pattern_list(path, strlen(path), NULL,
> -							&dtype, &pl, &the_index))) {
> +			    (core_apply_sparse_checkout &&

Do we need to test for `core_apply_sparse_checkout` here? Or does the `if
(!istate->sparse_checkout_patterns) return MATCHED;` early return in
`path_in_sparse_checkout()` suffice to catch this?

The remainder of the patch looks good to me.

Thank you,
Dscho

> +			     path_in_sparse_checkout(path, &the_index) == NOT_MATCHED)) {
>  				string_list_append(&only_match_skip_worktree,
>  						   pathspec->items[i].original);
>  			} else {
> diff --git a/dir.c b/dir.c
> index 03c4d212672..6fd4f2e0f27 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1439,6 +1439,39 @@ done:
>  	return result;
>  }
>
> +int init_sparse_checkout_patterns(struct index_state *istate)
> +{
> +	if (!core_apply_sparse_checkout ||
> +	    istate->sparse_checkout_patterns)
> +		return 0;
> +
> +	CALLOC_ARRAY(istate->sparse_checkout_patterns, 1);
> +
> +	if (get_sparse_checkout_patterns(istate->sparse_checkout_patterns) < 0) {
> +		FREE_AND_NULL(istate->sparse_checkout_patterns);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +int path_in_sparse_checkout(const char *path,
> +			    struct index_state *istate)
> +{
> +	const char *base;
> +	int dtype = DT_REG;
> +	init_sparse_checkout_patterns(istate);
> +
> +	if (!istate->sparse_checkout_patterns)
> +		return MATCHED;
> +
> +	base = strrchr(path, '/');
> +	return path_matches_pattern_list(path, strlen(path), base ? base + 1 : path,
> +					 &dtype,
> +					 istate->sparse_checkout_patterns,
> +					 istate) > 0;
> +}
> +
>  static struct path_pattern *last_matching_pattern_from_lists(
>  		struct dir_struct *dir, struct index_state *istate,
>  		const char *pathname, int pathlen,
> diff --git a/dir.h b/dir.h
> index b3e1a54a971..b899ee43d81 100644
> --- a/dir.h
> +++ b/dir.h
> @@ -394,6 +394,12 @@ enum pattern_match_result path_matches_pattern_list(const char *pathname,
>  				const char *basename, int *dtype,
>  				struct pattern_list *pl,
>  				struct index_state *istate);
> +
> +int init_sparse_checkout_patterns(struct index_state *state);
> +
> +int path_in_sparse_checkout(const char *path,
> +			    struct index_state *istate);
> +
>  struct dir_entry *dir_add_ignored(struct dir_struct *dir,
>  				  struct index_state *istate,
>  				  const char *pathname, int len);
> diff --git a/sparse-index.c b/sparse-index.c
> index b6e90417556..2efc9fd4910 100644
> --- a/sparse-index.c
> +++ b/sparse-index.c
> @@ -34,17 +34,14 @@ static int convert_to_sparse_rec(struct index_state *istate,
>  	int i, can_convert = 1;
>  	int start_converted = num_converted;
>  	enum pattern_match_result match;
> -	int dtype = DT_UNKNOWN;
>  	struct strbuf child_path = STRBUF_INIT;
> -	struct pattern_list *pl = istate->sparse_checkout_patterns;
>
>  	/*
>  	 * Is the current path outside of the sparse cone?
>  	 * Then check if the region can be replaced by a sparse
>  	 * directory entry (everything is sparse and merged).
>  	 */
> -	match = path_matches_pattern_list(ct_path, ct_pathlen,
> -					  NULL, &dtype, pl, istate);
> +	match = path_in_sparse_checkout(ct_path, istate);
>  	if (match != NOT_MATCHED)
>  		can_convert = 0;
>
> @@ -153,11 +150,8 @@ int convert_to_sparse(struct index_state *istate)
>  	if (!istate->repo->settings.sparse_index)
>  		return 0;
>
> -	if (!istate->sparse_checkout_patterns) {
> -		istate->sparse_checkout_patterns = xcalloc(1, sizeof(struct pattern_list));
> -		if (get_sparse_checkout_patterns(istate->sparse_checkout_patterns) < 0)
> -			return 0;
> -	}
> +	if (init_sparse_checkout_patterns(istate) < 0)
> +		return 0;
>
>  	/*
>  	 * We need cone-mode patterns to use sparse-index. If a user edits
> --
> gitgitgadget
>
>
Derrick Stolee Aug. 20, 2021, 3:30 p.m. UTC | #2
On 8/19/2021 4:07 AM, Johannes Schindelin wrote:
...
>>  			if (matches_skip_worktree(pathspec, i, &skip_worktree_seen) ||
>> -			    (sparse_checkout_enabled &&
>> -			     !path_matches_pattern_list(path, strlen(path), NULL,
>> -							&dtype, &pl, &the_index))) {
>> +			    (core_apply_sparse_checkout &&
> 
> Do we need to test for `core_apply_sparse_checkout` here? Or does the `if
> (!istate->sparse_checkout_patterns) return MATCHED;` early return in
> `path_in_sparse_checkout()` suffice to catch this?
> 
> The remainder of the patch looks good to me.
> 
> Thank you,
> Dscho
> 
>> +			     path_in_sparse_checkout(path, &the_index) == NOT_MATCHED)) {

Thank you for pointing out this. This is actually a stale change from an earlier
version where path_in_sparse_checkout returned an 'enum pattern_match_result'
but now casts down to an 'int', meaning '0' is not in the sparse-checkout and '1'
is that it _is_ in the sparse-checkout.

>> +int path_in_sparse_checkout(const char *path,
>> +			    struct index_state *istate)
>> +{
>> +	const char *base;
>> +	int dtype = DT_REG;
>> +	init_sparse_checkout_patterns(istate);
>> +
>> +	if (!istate->sparse_checkout_patterns)
>> +		return MATCHED;

So here, if we do not have sparse-checkout patterns (for example, if
core_apply_sparse_checkout is false), then this returns MATCHED (== 1).

To be extra clear, this should just be 'return 1;'.

>> +	base = strrchr(path, '/');
>> +	return path_matches_pattern_list(path, strlen(path), base ? base + 1 : path,
>> +					 &dtype,
>> +					 istate->sparse_checkout_patterns,
>> +					 istate) > 0;

Here, we are selecting the portion of 'enum pattern_match_result' that
we care about (MATCHED and MATCHED_RECURSIVE). The UNMATCHED (0) and
UNDECIDED (-1) are the other possibilities, but file paths will not
return UNDECIDED, that is instead for directories in non-cone mode
patterns.

Thanks,
-Stolee
diff mbox series

Patch

diff --git a/builtin/add.c b/builtin/add.c
index 17528e8f922..f675bdeae4a 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -190,8 +190,6 @@  static int refresh(int verbose, const struct pathspec *pathspec)
 	struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;
 	int flags = REFRESH_IGNORE_SKIP_WORKTREE |
 		    (verbose ? REFRESH_IN_PORCELAIN : REFRESH_QUIET);
-	struct pattern_list pl = { 0 };
-	int sparse_checkout_enabled = !get_sparse_checkout_patterns(&pl);
 
 	seen = xcalloc(pathspec->nr, 1);
 	refresh_index(&the_index, flags, pathspec, seen,
@@ -199,12 +197,10 @@  static int refresh(int verbose, const struct pathspec *pathspec)
 	for (i = 0; i < pathspec->nr; i++) {
 		if (!seen[i]) {
 			const char *path = pathspec->items[i].original;
-			int dtype = DT_REG;
 
 			if (matches_skip_worktree(pathspec, i, &skip_worktree_seen) ||
-			    (sparse_checkout_enabled &&
-			     !path_matches_pattern_list(path, strlen(path), NULL,
-							&dtype, &pl, &the_index))) {
+			    (core_apply_sparse_checkout &&
+			     path_in_sparse_checkout(path, &the_index) == NOT_MATCHED)) {
 				string_list_append(&only_match_skip_worktree,
 						   pathspec->items[i].original);
 			} else {
diff --git a/dir.c b/dir.c
index 03c4d212672..6fd4f2e0f27 100644
--- a/dir.c
+++ b/dir.c
@@ -1439,6 +1439,39 @@  done:
 	return result;
 }
 
+int init_sparse_checkout_patterns(struct index_state *istate)
+{
+	if (!core_apply_sparse_checkout ||
+	    istate->sparse_checkout_patterns)
+		return 0;
+
+	CALLOC_ARRAY(istate->sparse_checkout_patterns, 1);
+
+	if (get_sparse_checkout_patterns(istate->sparse_checkout_patterns) < 0) {
+		FREE_AND_NULL(istate->sparse_checkout_patterns);
+		return -1;
+	}
+
+	return 0;
+}
+
+int path_in_sparse_checkout(const char *path,
+			    struct index_state *istate)
+{
+	const char *base;
+	int dtype = DT_REG;
+	init_sparse_checkout_patterns(istate);
+
+	if (!istate->sparse_checkout_patterns)
+		return MATCHED;
+
+	base = strrchr(path, '/');
+	return path_matches_pattern_list(path, strlen(path), base ? base + 1 : path,
+					 &dtype,
+					 istate->sparse_checkout_patterns,
+					 istate) > 0;
+}
+
 static struct path_pattern *last_matching_pattern_from_lists(
 		struct dir_struct *dir, struct index_state *istate,
 		const char *pathname, int pathlen,
diff --git a/dir.h b/dir.h
index b3e1a54a971..b899ee43d81 100644
--- a/dir.h
+++ b/dir.h
@@ -394,6 +394,12 @@  enum pattern_match_result path_matches_pattern_list(const char *pathname,
 				const char *basename, int *dtype,
 				struct pattern_list *pl,
 				struct index_state *istate);
+
+int init_sparse_checkout_patterns(struct index_state *state);
+
+int path_in_sparse_checkout(const char *path,
+			    struct index_state *istate);
+
 struct dir_entry *dir_add_ignored(struct dir_struct *dir,
 				  struct index_state *istate,
 				  const char *pathname, int len);
diff --git a/sparse-index.c b/sparse-index.c
index b6e90417556..2efc9fd4910 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -34,17 +34,14 @@  static int convert_to_sparse_rec(struct index_state *istate,
 	int i, can_convert = 1;
 	int start_converted = num_converted;
 	enum pattern_match_result match;
-	int dtype = DT_UNKNOWN;
 	struct strbuf child_path = STRBUF_INIT;
-	struct pattern_list *pl = istate->sparse_checkout_patterns;
 
 	/*
 	 * Is the current path outside of the sparse cone?
 	 * Then check if the region can be replaced by a sparse
 	 * directory entry (everything is sparse and merged).
 	 */
-	match = path_matches_pattern_list(ct_path, ct_pathlen,
-					  NULL, &dtype, pl, istate);
+	match = path_in_sparse_checkout(ct_path, istate);
 	if (match != NOT_MATCHED)
 		can_convert = 0;
 
@@ -153,11 +150,8 @@  int convert_to_sparse(struct index_state *istate)
 	if (!istate->repo->settings.sparse_index)
 		return 0;
 
-	if (!istate->sparse_checkout_patterns) {
-		istate->sparse_checkout_patterns = xcalloc(1, sizeof(struct pattern_list));
-		if (get_sparse_checkout_patterns(istate->sparse_checkout_patterns) < 0)
-			return 0;
-	}
+	if (init_sparse_checkout_patterns(istate) < 0)
+		return 0;
 
 	/*
 	 * We need cone-mode patterns to use sparse-index. If a user edits