[v2,2/3] sparse-index: add ensure_correct_sparsity function

Commit Message

Victoria Dye Oct. 21, 2021, 8:48 p.m. UTC

From: Victoria Dye <[e-mail]>

The purpose of the `ensure_correct_sparsity` function is to provide a means
of aligning the in-core index with the sparsity required by the repository
settings and other properties of the index. The function will first attempt
to convert the index to sparse, now with a "SPARSE_INDEX_VERIFY_ALLOWED"
flag that forces the function to return a nonzero value if repository
settings do not allow use of a sparse index. If a nonzero value is returned,
the index is expanded to full with `ensure_full_index`.

Helped-by: Junio C Hamano <[e-mail]>
Signed-off-by: Victoria Dye <[e-mail]>
---
 sparse-index.c | 42 +++++++++++++++++++++++++++++++++++++++---
 sparse-index.h |  2 ++
 2 files changed, 41 insertions(+), 3 deletions(-)

Comments

Junio C Hamano Oct. 21, 2021, 10:20 p.m. UTC | #1

"Victoria Dye via GitGitGadget" <[e-mail]> writes:

> -int convert_to_sparse(struct index_state *istate, int flags)
> +static int can_convert_to_sparse(struct index_state *istate, int flags)
>  {
>  	int test_env;

May not be a problem with this patch, but this variable does not
need to be in this large a scope.

> -	if (istate->sparse_index || !istate->cache_nr ||
> -	    !core_apply_sparse_checkout || !core_sparse_checkout_cone)
> +	if (!core_apply_sparse_checkout || !core_sparse_checkout_cone)
>  		return 0;
>  
>  	if (!istate->repo)
> @@ -187,6 +186,30 @@ int convert_to_sparse(struct index_state *istate, int flags)
>  	if (cache_tree_update(istate, WRITE_TREE_MISSING_OK))
>  		return 0;
>  
> +	return 1;
> +}
> +
> +int convert_to_sparse(struct index_state *istate, int flags)
> +{
> +	int verify = flags & SPARSE_INDEX_VERIFY_ALLOWED;
> +
> +	/*
> +	 * If validating with strict checks against whether the sparse index is
> +	 * allowed, we want to check `can_convert_to_sparse` *before* exiting
> +	 * early due to an already sparse or empty index.
> +	 *
> +	 * If not performing strict validation, the order is reversed to avoid
> +	 * the more expensive checks in `can_convert_to_sparse` whenver possible.
> +	 */
> +	if (verify) {
> +		if (!can_convert_to_sparse(istate, flags))
> +			return -1;
> +		else if (istate->sparse_index || !istate->cache_nr)
> +			return 0;
> +	} else if (istate->sparse_index || !istate->cache_nr ||
> +		   !can_convert_to_sparse(istate, flags))
> +		return 0;
> +
>  	remove_fsmonitor(istate);
>  
>  	trace2_region_enter("index", "convert_to_sparse", istate->repo);
> @@ -313,6 +336,19 @@ void ensure_full_index(struct index_state *istate)
>  	trace2_region_leave("index", "ensure_full_index", istate->repo);
>  }
>  
> +void ensure_correct_sparsity(struct index_state *istate)
> +{
> +	/*
> +	 * First check whether the index can be converted to sparse by attempting
> +	 * to convert it with the SPARSE_INDEX_VERIFY_ALLOWED flag. If the
> +	 * SPARSE_INDEX_VERIFY_ALLOWED checks indicate that the index cannot
> +	 * be converted because repository settings and/or index properties
> +	 * do not allow it, expand the index to full.
> +	 */

The logic may be OK, but the need to give this long description is a
sign that the meaning of the value returned from the function is not
clear from the name of the function.

> +	if (convert_to_sparse(istate, SPARSE_INDEX_VERIFY_ALLOWED))
> +		ensure_full_index(istate);

It might make it more straight-forward to 

 (1) drop the "if (verify)" part in convert_to_sparse(), which would
     mean that for all callers convert_to_sparse() will retain the
     same behaviour as before;

 (2) make a call to can_convert_to_sparse() here, and if that
     returns true, make a call to ensure_full_index(); this would
     behave identically to what this patch does when can_convert is
     false; and

 (3) correct the can_convert_to_sparse() function to not blow away
     the cache_tree unconditionally and recompute, so that calling
     it twice in a row will not be costly.

Victoria Dye Oct. 27, 2021, 5:21 p.m. UTC | #2

Junio C Hamano wrote:
> "Victoria Dye via GitGitGadget" <[e-mail]> writes:
> >> @@ -313,6 +336,19 @@ void ensure_full_index(struct index_state *istate)
>>  	trace2_region_leave("index", "ensure_full_index", istate->repo);
>>  }
>>  
>> +void ensure_correct_sparsity(struct index_state *istate)
>> +{
>> +	/*
>> +	 * First check whether the index can be converted to sparse by attempting
>> +	 * to convert it with the SPARSE_INDEX_VERIFY_ALLOWED flag. If the
>> +	 * SPARSE_INDEX_VERIFY_ALLOWED checks indicate that the index cannot
>> +	 * be converted because repository settings and/or index properties
>> +	 * do not allow it, expand the index to full.
>> +	 */
> 
> The logic may be OK, but the need to give this long description is a
> sign that the meaning of the value returned from the function is not
> clear from the name of the function.
> 
>> +	if (convert_to_sparse(istate, SPARSE_INDEX_VERIFY_ALLOWED))
>> +		ensure_full_index(istate);
> 
> It might make it more straight-forward to 
> 
>  (1) drop the "if (verify)" part in convert_to_sparse(), which would
>      mean that for all callers convert_to_sparse() will retain the
>      same behaviour as before;
> 
>  (2) make a call to can_convert_to_sparse() here, and if that
>      returns true, make a call to ensure_full_index(); this would
>      behave identically to what this patch does when can_convert is
>      false; and
> 
>  (3) correct the can_convert_to_sparse() function to not blow away
>      the cache_tree unconditionally and recompute, so that calling
>      it twice in a row will not be costly.
> 

Agreed, this approach is a lot easier to follow. I went a bit overboard
trying to handle *all* possible cases where `convert_to_sparse` returns
before converting, but the primary goal of this series is updating the
behavior when config settings change. I'll include the suggested
restructuring in my next re-roll. Thanks!

Patch

diff --git a/sparse-index.c b/sparse-index.c
index 7b7ff79e044..4273453e078 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -122,11 +122,10 @@  static int index_has_unmerged_entries(struct index_state *istate)
 	return 0;
 }
 
-int convert_to_sparse(struct index_state *istate, int flags)
+static int can_convert_to_sparse(struct index_state *istate, int flags)
 {
 	int test_env;
-	if (istate->sparse_index || !istate->cache_nr ||
-	    !core_apply_sparse_checkout || !core_sparse_checkout_cone)
+	if (!core_apply_sparse_checkout || !core_sparse_checkout_cone)
 		return 0;
 
 	if (!istate->repo)
@@ -187,6 +186,30 @@  int convert_to_sparse(struct index_state *istate, int flags)
 	if (cache_tree_update(istate, WRITE_TREE_MISSING_OK))
 		return 0;
 
+	return 1;
+}
+
+int convert_to_sparse(struct index_state *istate, int flags)
+{
+	int verify = flags & SPARSE_INDEX_VERIFY_ALLOWED;
+
+	/*
+	 * If validating with strict checks against whether the sparse index is
+	 * allowed, we want to check `can_convert_to_sparse` *before* exiting
+	 * early due to an already sparse or empty index.
+	 *
+	 * If not performing strict validation, the order is reversed to avoid
+	 * the more expensive checks in `can_convert_to_sparse` whenver possible.
+	 */
+	if (verify) {
+		if (!can_convert_to_sparse(istate, flags))
+			return -1;
+		else if (istate->sparse_index || !istate->cache_nr)
+			return 0;
+	} else if (istate->sparse_index || !istate->cache_nr ||
+		   !can_convert_to_sparse(istate, flags))
+		return 0;
+
 	remove_fsmonitor(istate);
 
 	trace2_region_enter("index", "convert_to_sparse", istate->repo);
@@ -313,6 +336,19 @@  void ensure_full_index(struct index_state *istate)
 	trace2_region_leave("index", "ensure_full_index", istate->repo);
 }
 
+void ensure_correct_sparsity(struct index_state *istate)
+{
+	/*
+	 * First check whether the index can be converted to sparse by attempting
+	 * to convert it with the SPARSE_INDEX_VERIFY_ALLOWED flag. If the
+	 * SPARSE_INDEX_VERIFY_ALLOWED checks indicate that the index cannot
+	 * be converted because repository settings and/or index properties
+	 * do not allow it, expand the index to full.
+	 */
+	if (convert_to_sparse(istate, SPARSE_INDEX_VERIFY_ALLOWED))
+		ensure_full_index(istate);
+}
+
 /*
  * This static global helps avoid infinite recursion between
  * expand_to_path() and index_file_exists().
diff --git a/sparse-index.h b/sparse-index.h
index 9f3d7bc7faf..b61754f1f76 100644
--- a/sparse-index.h
+++ b/sparse-index.h
@@ -3,7 +3,9 @@ 
 
 struct index_state;
 #define SPARSE_INDEX_MEMORY_ONLY (1 << 0)
+#define SPARSE_INDEX_VERIFY_ALLOWED (1 << 1)
 int convert_to_sparse(struct index_state *istate, int flags);
+void ensure_correct_sparsity(struct index_state *istate);
 
 /*
  * Some places in the codebase expect to search for a specific path.