threads / patch / 44077

patchunpack-trees: pass checkout state explicitly to check_updates()

Subject: [PATCH] unpack-trees: pass checkout state explicitly to check_updates()

## tl;dr

2 messages between Sep 13, 2016 and Sep 13, 2016. Diffs are folded; open one to read it.

replies: 1people: 2as markdown or json

René Scharfe· Sep 13, 2016, 17:37 UTC · lore

Add a parameter for the struct checkout variable to check_updates() instead of using a static global variable. Passing it explicitly makes object ownership and usage more easily apparent. And we get rid of a static variable; those can be problematic in library-like code.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 unpack-trees.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Show changes to unpack-trees.c +5 −4

diff --git a/unpack-trees.c b/unpack-trees.c
index 11c37fb..74d6dd4 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -218,8 +218,8 @@ static void unlink_entry(const struct cache_entry *ce)
 	schedule_dir_for_removal(ce->name, ce_namelen(ce));
 }
 
-static struct checkout state;
-static int check_updates(struct unpack_trees_options *o)
+static int check_updates(struct unpack_trees_options *o,
+			 const struct checkout *state)
 {
 	unsigned cnt = 0, total = 0;
 	struct progress *progress = NULL;
@@ -264,7 +264,7 @@ static int check_updates(struct unpack_trees_options *o)
 			display_progress(progress, ++cnt);
 			ce->ce_flags &= ~CE_UPDATE;
 			if (o->update && !o->dry_run) {
-				errs |= checkout_entry(ce, &state, NULL);
+				errs |= checkout_entry(ce, state, NULL);
 			}
 		}
 	}
@@ -1094,6 +1094,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	int i, ret;
 	static struct cache_entry *dfc;
 	struct exclude_list el;
+	struct checkout state;
 
 	if (len > MAX_UNPACK_TREES)
 		die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
@@ -1239,7 +1240,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	}
 
 	o->src_index = NULL;
-	ret = check_updates(o) ? (-2) : 0;
+	ret = check_updates(o, &state) ? (-2) : 0;
 	if (o->dst_index) {
 		if (!ret) {
 			if (!o->result.cache_tree)

-- 
2.10.0

Junio C Hamano· Sep 13, 2016, 23:25 UTC · re: René Scharfe · lore

Re: [PATCH] unpack-trees: pass checkout state explicitly to check_updates()

René Scharfe <l.s.r@web.de> writes:

Show 14 quoted lines

> Add a parameter for the struct checkout variable to check_updates()
> instead of using a static global variable.  Passing it explicitly makes
> object ownership and usage more easily apparent.  And we get rid of a
> static variable; those can be problematic in library-like code.
> ...
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 11c37fb..74d6dd4 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -218,8 +218,8 @@ static void unlink_entry(const struct cache_entry *ce)
>  	schedule_dir_for_removal(ce->name, ce_namelen(ce));
>  }
>  
> -static struct checkout state;

Show 6 quoted lines

> ...
> @@ -1094,6 +1094,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>  	int i, ret;
>  	static struct cache_entry *dfc;
>  	struct exclude_list el;
> +	struct checkout state;

Does the distinction between this thing in BSS implicitly cleared and the new one on stack that does not seem to have any initialization matter?

	... goes and looks ...

OK, after this hunk we clear and set up everything in state, so there is no difference in behaviour. Just we got rid of an unnecessary file-scope global.

Nice.  Thanks.

Show 11 quoted lines

>  	if (len > MAX_UNPACK_TREES)
>  		die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
> @@ -1239,7 +1240,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>  	}
>  
>  	o->src_index = NULL;
> -	ret = check_updates(o) ? (-2) : 0;
> +	ret = check_updates(o, &state) ? (-2) : 0;
>  	if (o->dst_index) {
>  		if (!ret) {
>  			if (!o->result.cache_tree)