diff mbox series

[05/11] sparse-checkout: avoid using internal API of unpack-trees

Message ID 508837fc182b0adba96c3fcbd468f47f8f0a0aef.1677143700.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Clarify API for dir.[ch] and unpack-trees.[ch] -- mark relevant fields as internal | expand

Commit Message

Elijah Newren Feb. 23, 2023, 9:14 a.m. UTC 
From: Elijah Newren <newren@gmail.com>

struct unpack_trees_options has the following field and comment:

	struct pattern_list *pl; /* for internal use */

Despite the internal-use comment, commit e091228e17 ("sparse-checkout:
update working directory in-process", 2019-11-21) starting setting this
field from an external caller.  At the time, the only way around that
would have been to modify unpack_trees() to take an extra pattern_list
argument, and there's a lot of callers of that function.  However, when
we split update_sparsity() off as a separate function, with
sparse-checkout being the sole caller, the need to update other callers
went away.  Fix this API problem by adding a pattern_list argument to
update_sparsity() and stop setting the internal o.pl field directly.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/sparse-checkout.c |  3 +--
 unpack-trees.c            | 17 ++++++++++-------
 unpack-trees.h            |  3 ++-
 3 files changed, 13 insertions(+), 10 deletions(-)

Comments

Jonathan Tan Feb. 24, 2023, 10:37 p.m. UTC | #1 
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index c3738154918..4b7390ce367 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -219,14 +219,13 @@ static int update_working_directory(struct pattern_list *pl)
>  	o.dst_index = r->index;
>  	index_state_init(&o.result, r);
>  	o.skip_sparse_checkout = 0;
> -	o.pl = pl;
>  
>  	setup_work_tree();
>  
>  	repo_hold_locked_index(r, &lock_file, LOCK_DIE_ON_ERROR);
>  
>  	setup_unpack_trees_porcelain(&o, "sparse-checkout");
> -	result = update_sparsity(&o);
> +	result = update_sparsity(&o, pl);

This makes sense - setup_unpack_trees_porcelain() does not use o.pl, so
there is no logic change.

> @@ -2111,11 +2111,12 @@ enum update_sparsity_result update_sparsity(struct unpack_trees_options *o)
>  	trace_performance_enter();
>  
>  	/* If we weren't given patterns, use the recorded ones */
> -	if (!o->pl) {
> -		memset(&pl, 0, sizeof(pl));
> +	if (!pl) {
>  		free_pattern_list = 1;
> -		populate_from_existing_patterns(o, &pl);
> +		pl = xcalloc(1, sizeof(*pl));
> +		populate_from_existing_patterns(o, pl);
>  	}
> +	o->pl = pl;
>  
>  	/* Expand sparse directories as needed */
>  	expand_index(o->src_index, o->pl);
> @@ -2147,8 +2148,10 @@ enum update_sparsity_result update_sparsity(struct unpack_trees_options *o)
>  
>  	display_warning_msgs(o);
>  	o->show_all_errors = old_show_all_errors;
> -	if (free_pattern_list)
> -		clear_pattern_list(&pl);
> +	if (free_pattern_list) {
> +		clear_pattern_list(pl);
> +		free(pl);
> +	}

When free_pattern_list is true, we free "pl" which was previously
assigned to "o->pl". Do we thus need to also clear "o->pl"?
Elijah Newren Feb. 25, 2023, 12:33 a.m. UTC | #2 
On Fri, Feb 24, 2023 at 2:37 PM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> > diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> > index c3738154918..4b7390ce367 100644
> > --- a/builtin/sparse-checkout.c
> > +++ b/builtin/sparse-checkout.c
> > @@ -219,14 +219,13 @@ static int update_working_directory(struct pattern_list *pl)
> >       o.dst_index = r->index;
> >       index_state_init(&o.result, r);
> >       o.skip_sparse_checkout = 0;
> > -     o.pl = pl;
> >
> >       setup_work_tree();
> >
> >       repo_hold_locked_index(r, &lock_file, LOCK_DIE_ON_ERROR);
> >
> >       setup_unpack_trees_porcelain(&o, "sparse-checkout");
> > -     result = update_sparsity(&o);
> > +     result = update_sparsity(&o, pl);
>
> This makes sense - setup_unpack_trees_porcelain() does not use o.pl, so
> there is no logic change.
>
> > @@ -2111,11 +2111,12 @@ enum update_sparsity_result update_sparsity(struct unpack_trees_options *o)
> >       trace_performance_enter();
> >
> >       /* If we weren't given patterns, use the recorded ones */
> > -     if (!o->pl) {
> > -             memset(&pl, 0, sizeof(pl));
> > +     if (!pl) {
> >               free_pattern_list = 1;
> > -             populate_from_existing_patterns(o, &pl);
> > +             pl = xcalloc(1, sizeof(*pl));
> > +             populate_from_existing_patterns(o, pl);
> >       }
> > +     o->pl = pl;
> >
> >       /* Expand sparse directories as needed */
> >       expand_index(o->src_index, o->pl);
> > @@ -2147,8 +2148,10 @@ enum update_sparsity_result update_sparsity(struct unpack_trees_options *o)
> >
> >       display_warning_msgs(o);
> >       o->show_all_errors = old_show_all_errors;
> > -     if (free_pattern_list)
> > -             clear_pattern_list(&pl);
> > +     if (free_pattern_list) {
> > +             clear_pattern_list(pl);
> > +             free(pl);
> > +     }
>
> When free_pattern_list is true, we free "pl" which was previously
> assigned to "o->pl". Do we thus need to also clear "o->pl"?

Yeah, I don't think the existing code will ever attempt to use o->pl
again under these circumstances, but setting it to NULL for
future-proofing makes sense.  I'll make that tweak; thanks for reading
carefully!
diff mbox series

Patch

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index c3738154918..4b7390ce367 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -219,14 +219,13 @@  static int update_working_directory(struct pattern_list *pl)
 	o.dst_index = r->index;
 	index_state_init(&o.result, r);
 	o.skip_sparse_checkout = 0;
-	o.pl = pl;
 
 	setup_work_tree();
 
 	repo_hold_locked_index(r, &lock_file, LOCK_DIE_ON_ERROR);
 
 	setup_unpack_trees_porcelain(&o, "sparse-checkout");
-	result = update_sparsity(&o);
+	result = update_sparsity(&o, pl);
 	clear_unpack_trees_porcelain(&o);
 
 	if (result == UPDATE_SPARSITY_WARNINGS)
diff --git a/unpack-trees.c b/unpack-trees.c
index 0887d157df4..d9c9f330233 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2091,10 +2091,10 @@  return_failed:
  *
  * CE_NEW_SKIP_WORKTREE is used internally.
  */
-enum update_sparsity_result update_sparsity(struct unpack_trees_options *o)
+enum update_sparsity_result update_sparsity(struct unpack_trees_options *o,
+					    struct pattern_list *pl)
 {
 	enum update_sparsity_result ret = UPDATE_SPARSITY_SUCCESS;
-	struct pattern_list pl;
 	int i;
 	unsigned old_show_all_errors;
 	int free_pattern_list = 0;
@@ -2111,11 +2111,12 @@  enum update_sparsity_result update_sparsity(struct unpack_trees_options *o)
 	trace_performance_enter();
 
 	/* If we weren't given patterns, use the recorded ones */
-	if (!o->pl) {
-		memset(&pl, 0, sizeof(pl));
+	if (!pl) {
 		free_pattern_list = 1;
-		populate_from_existing_patterns(o, &pl);
+		pl = xcalloc(1, sizeof(*pl));
+		populate_from_existing_patterns(o, pl);
 	}
+	o->pl = pl;
 
 	/* Expand sparse directories as needed */
 	expand_index(o->src_index, o->pl);
@@ -2147,8 +2148,10 @@  enum update_sparsity_result update_sparsity(struct unpack_trees_options *o)
 
 	display_warning_msgs(o);
 	o->show_all_errors = old_show_all_errors;
-	if (free_pattern_list)
-		clear_pattern_list(&pl);
+	if (free_pattern_list) {
+		clear_pattern_list(pl);
+		free(pl);
+	}
 	trace_performance_leave("update_sparsity");
 	return ret;
 }
diff --git a/unpack-trees.h b/unpack-trees.h
index 3a7b3e5f007..f3a6e4f90ef 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -112,7 +112,8 @@  enum update_sparsity_result {
 	UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES = -2
 };
 
-enum update_sparsity_result update_sparsity(struct unpack_trees_options *options);
+enum update_sparsity_result update_sparsity(struct unpack_trees_options *options,
+					    struct pattern_list *pl);
 
 int verify_uptodate(const struct cache_entry *ce,
 		    struct unpack_trees_options *o);