[PATCHv2] unpack-trees: move checkout state into check_updates

Stefan Beller· Dec 29, 2016, 19:43 UTC · lore

The checkout state was introduced via 16da134b1f9 (read-trees: refactor the unpack_trees() part, 2006-07-30). An attempt to refactor the checkout state was done in b56aa5b268e (unpack-trees: pass checkout state explicitly to check_updates(), 2016-09-13), but we can go even further.

The `struct checkout state` is not used in unpack_trees apart from initializing it, so move it into the function that makes use of it, which is `check_updates`.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Rene wrote:

> we could use "index" instead of "&o->result".  Not sure if it's worth a patch, though.

done in this iteration of the patch. I also reordered the variables at the beginning of the function for readability.

Thanks, Stefan

 unpack-trees.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 7a6df99d10..eb44f50e65 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -218,14 +218,18 @@ static void unlink_entry(const struct cache_entry *ce)
 	schedule_dir_for_removal(ce->name, ce_namelen(ce));
 }
 
-static int check_updates(struct unpack_trees_options *o,
-			 const struct checkout *state)
+static int check_updates(struct unpack_trees_options *o)
 {
 	unsigned cnt = 0, total = 0;
 	struct progress *progress = NULL;
 	struct index_state *index = &o->result;
-	int i;
-	int errs = 0;
+	struct checkout state = CHECKOUT_INIT;
+	int i, errs = 0;
+
+	state.force = 1;
+	state.quiet = 1;
+	state.refresh_cache = 1;
+	state.istate = index;
 
 	if (o->update && o->verbose_update) {
 		for (total = cnt = 0; cnt < index->cache_nr; cnt++) {
@@ -240,7 +244,7 @@ static int check_updates(struct unpack_trees_options *o,
 	}
 
 	if (o->update)
-		git_attr_set_direction(GIT_ATTR_CHECKOUT, &o->result);
+		git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
 	for (i = 0; i < index->cache_nr; i++) {
 		const struct cache_entry *ce = index->cache[i];
 
@@ -251,7 +255,7 @@ static int check_updates(struct unpack_trees_options *o,
 			continue;
 		}
 	}
-	remove_marked_cache_entries(&o->result);
+	remove_marked_cache_entries(index);
 	remove_scheduled_dirs();
 
 	for (i = 0; i < index->cache_nr; i++) {
@@ -264,7 +268,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,14 +1098,9 @@ 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 = CHECKOUT_INIT;
 
 	if (len > MAX_UNPACK_TREES)
 		die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
-	state.force = 1;
-	state.quiet = 1;
-	state.refresh_cache = 1;
-	state.istate = &o->result;
 
 	memset(&el, 0, sizeof(el));
 	if (!core_apply_sparse_checkout || !o->update)
@@ -1238,7 +1237,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	}
 
 	o->src_index = NULL;
-	ret = check_updates(o, &state) ? (-2) : 0;
+	ret = check_updates(o) ? (-2) : 0;
 	if (o->dst_index) {
 		if (!ret) {
 			if (!o->result.cache_tree)

-- 
2.11.0.259.ga95e92af08.dirty

Junio C Hamano· Jan 1, 2017, 01:26 UTC · re: Stefan Beller · lore

Re: [PATCHv2] unpack-trees: move checkout state into check_updates

Stefan Beller <sbeller@google.com> writes:

Show 12 quoted lines

> The checkout state was introduced via 16da134b1f9
> (read-trees: refactor the unpack_trees() part, 2006-07-30). An attempt to
> refactor the checkout state was done in b56aa5b268e (unpack-trees: pass
> checkout state explicitly to check_updates(), 2016-09-13), but we can
> go even further.
>
> The `struct checkout state` is not used in unpack_trees apart from
> initializing it, so move it into the function that makes use of it,
> which is `check_updates`.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

I'd add René's Reviewed-by: here.

Show 16 quoted lines

> -static int check_updates(struct unpack_trees_options *o,
> -			 const struct checkout *state)
> +static int check_updates(struct unpack_trees_options *o)
>  {
>  	unsigned cnt = 0, total = 0;
>  	struct progress *progress = NULL;
>  	struct index_state *index = &o->result;
> -	int i;
> -	int errs = 0;
> +	struct checkout state = CHECKOUT_INIT;
> +	int i, errs = 0;
> +
> +	state.force = 1;
> +	state.quiet = 1;
> +	state.refresh_cache = 1;
> +	state.istate = index;

I think moving heavier and initialized variables earlier and more lightweight and ephemeral ones like "i" later does make it easier to follow. "errs" has the significance and the lifetime similar to cnt/total, and logically should be higher, though. It is not a big enough deal to reroll (but as your futzing of the variable definition order was not a big enough deal to do in this patch, either, so...).

Queued.  Thanks.