diff mbox series

[v2,04/18] unpack-trees: simplify pattern_list freeing

Message ID 65772dd46df12d79a41e7ed2d6e1fc197b80d379.1584813609.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Sparse checkout improvements -- improved sparsity updating | expand

Commit Message

Linus Arver via GitGitGadget March 21, 2020, 5:59 p.m. UTC 
From: Elijah Newren <newren@gmail.com>

commit e091228e17 ("sparse-checkout: update working directory
in-process", 2019-11-21) allowed passing a pre-defined set of patterns
to unpack_trees().  However, if o->pl was NULL, it would still read the
existing patterns and use those.  If those patterns were read into a
data structure that was allocated, naturally they needed to be free'd.
However, despite the same function being responsible for knowing about
both the allocation and the free'ing, the logic for tracking whether to
free the pattern_list was hoisted to an outer function with an
additional flag in unpack_trees_options.  Put the logic back in the
relevant function and discard the now unnecessary flag.

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

Comments

Derrick Stolee March 23, 2020, 3:57 p.m. UTC | #1 
On 3/21/2020 1:59 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> commit e091228e17 ("sparse-checkout: update working directory
> in-process", 2019-11-21) allowed passing a pre-defined set of patterns
> to unpack_trees().  However, if o->pl was NULL, it would still read the
> existing patterns and use those.  If those patterns were read into a
> data structure that was allocated, naturally they needed to be free'd.
> However, despite the same function being responsible for knowing about
> both the allocation and the free'ing, the logic for tracking whether to
> free the pattern_list was hoisted to an outer function with an
> additional flag in unpack_trees_options.  Put the logic back in the
> relevant function and discard the now unnecessary flag.

Your approach here is much cleaner. Thanks!

> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  builtin/sparse-checkout.c | 1 -
>  unpack-trees.c            | 6 ++++--
>  unpack-trees.h            | 3 +--
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index 740da4b6d54..d102a9697fd 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -122,7 +122,6 @@ static int update_working_directory(struct pattern_list *pl)
>  	o.dst_index = r->index;
>  	o.skip_sparse_checkout = 0;
>  	o.pl = pl;
> -	o.keep_pattern_list = !!pl;
>  
>  	resolve_undo_clear_index(r->index);
>  	setup_work_tree();
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 3af2e126abf..d2863fa0310 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1503,6 +1503,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>  	int i, ret;
>  	static struct cache_entry *dfc;
>  	struct pattern_list pl;
> +	int free_pattern_list = 0;
>  
>  	if (len > MAX_UNPACK_TREES)
>  		die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
> @@ -1519,6 +1520,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>  		else
>  			o->pl = &pl;
>  		free(sparse);
> +		free_pattern_list = 1;
>  	}
>  
>  	memset(&o->result, 0, sizeof(o->result));
> @@ -1686,9 +1688,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>  	o->src_index = NULL;
>  
>  done:
> -	trace_performance_leave("unpack_trees");
> -	if (!o->keep_pattern_list)
> +	if (free_pattern_list)
>  		clear_pattern_list(&pl);
> +	trace_performance_leave("unpack_trees");
>  	return ret;
>  
>  return_failed:
> diff --git a/unpack-trees.h b/unpack-trees.h
> index 6d7c7b6c2e0..d3516267f36 100644
> --- a/unpack-trees.h
> +++ b/unpack-trees.h
> @@ -58,8 +58,7 @@ struct unpack_trees_options {
>  		     quiet,
>  		     exiting_early,
>  		     show_all_errors,
> -		     dry_run,
> -		     keep_pattern_list;
> +		     dry_run;
>  	const char *prefix;
>  	int cache_bottom;
>  	struct dir_struct *dir;
>
diff mbox series

Patch

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 740da4b6d54..d102a9697fd 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -122,7 +122,6 @@  static int update_working_directory(struct pattern_list *pl)
 	o.dst_index = r->index;
 	o.skip_sparse_checkout = 0;
 	o.pl = pl;
-	o.keep_pattern_list = !!pl;
 
 	resolve_undo_clear_index(r->index);
 	setup_work_tree();
diff --git a/unpack-trees.c b/unpack-trees.c
index 3af2e126abf..d2863fa0310 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1503,6 +1503,7 @@  int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	int i, ret;
 	static struct cache_entry *dfc;
 	struct pattern_list pl;
+	int free_pattern_list = 0;
 
 	if (len > MAX_UNPACK_TREES)
 		die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
@@ -1519,6 +1520,7 @@  int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 		else
 			o->pl = &pl;
 		free(sparse);
+		free_pattern_list = 1;
 	}
 
 	memset(&o->result, 0, sizeof(o->result));
@@ -1686,9 +1688,9 @@  int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	o->src_index = NULL;
 
 done:
-	trace_performance_leave("unpack_trees");
-	if (!o->keep_pattern_list)
+	if (free_pattern_list)
 		clear_pattern_list(&pl);
+	trace_performance_leave("unpack_trees");
 	return ret;
 
 return_failed:
diff --git a/unpack-trees.h b/unpack-trees.h
index 6d7c7b6c2e0..d3516267f36 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -58,8 +58,7 @@  struct unpack_trees_options {
 		     quiet,
 		     exiting_early,
 		     show_all_errors,
-		     dry_run,
-		     keep_pattern_list;
+		     dry_run;
 	const char *prefix;
 	int cache_bottom;
 	struct dir_struct *dir;