# [PATCH 0/5] refactor unpack-trees 9 messages from 2017-01-06 to 2017-01-09. Participants: Stefan Beller, Jeff King. Thread: https://gitlist.dev/t/44828 ## Stefan Beller, 2017-01-06 21:03 Subject: [PATCH 0/5] refactor unpack-trees Message-ID: <20170106210330.31761-1-sbeller@google.com> URL: https://gitlist.dev/e/20170106210330.31761-1-sbeller%40google.com In-Reply-To: ``` unpack-trees is a central file needed for the understanding of working tree manipulation. To help with the understanding refactor the code to be more readable. The first patch was a standalone patch 8 days ago; now incorporated into this series as a v3, reducing the scope of the checkout state. The second patch removes a single continue statement; it needed some digging to explain, but looks trivial. The last 3 patches shorten the check_updates function by adding more functions. If we ever want to parallelize file IO then these smaller functions would be the scope to do it, keeping the check_updates as a high level function guiding through the steps what is happening during a working tree update. Thanks, Stefan Stefan Beller (5): unpack-trees: move checkout state into check_updates unpack-trees: remove unneeded continue unpack-trees: factor progress setup out of check_updates unpack-trees: factor file removal out of check_updates unpack-trees: factor working tree update out of check_updates unpack-trees.c | 96 ++++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 64 insertions(+), 32 deletions(-) -- 2.11.0.31.g919a8d0.dirty ``` ## Stefan Beller, 2017-01-06 21:03 Subject: [PATCHv3 1/5] unpack-trees: move checkout state into check_updates Message-ID: <20170106210330.31761-2-sbeller@google.com> URL: https://gitlist.dev/e/20170106210330.31761-2-sbeller%40google.com In-Reply-To: <20170106210330.31761-1-sbeller@google.com> ``` 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`. Reviewed-by: RenĂ© Scharfe Signed-off-by: Stefan Beller --- unpack-trees.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/unpack-trees.c b/unpack-trees.c index bc56195e27..55c75b4d6a 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -237,77 +237,82 @@ static void display_error_msgs(struct unpack_trees_options *o) } /* * Unlink the last component and schedule the leading directories for * removal, such that empty directories get removed. */ static void unlink_entry(const struct cache_entry *ce) { if (!check_leading_path(ce->name, ce_namelen(ce))) return; if (remove_or_warn(ce->ce_mode, ce->name)) return; 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; + int i, errs = 0; + struct progress *progress = NULL; struct index_state *index = &o->result; - int i; - int errs = 0; + struct checkout state = CHECKOUT_INIT; + + 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++) { const struct cache_entry *ce = index->cache[cnt]; if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE)) total++; } progress = start_progress_delay(_("Checking out files"), total, 50, 1); cnt = 0; } 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]; if (ce->ce_flags & CE_WT_REMOVE) { display_progress(progress, ++cnt); if (o->update && !o->dry_run) unlink_entry(ce); continue; } } - remove_marked_cache_entries(&o->result); + remove_marked_cache_entries(index); remove_scheduled_dirs(); for (i = 0; i < index->cache_nr; i++) { struct cache_entry *ce = index->cache[i]; if (ce->ce_flags & CE_UPDATE) { if (ce->ce_flags & CE_WT_REMOVE) die("BUG: both update and delete flags are set on %s", ce->name); 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); } } } stop_progress(&progress); if (o->update) git_attr_set_direction(GIT_ATTR_CHECKIN, NULL); return errs != 0; } static int verify_uptodate_sparse(const struct cache_entry *ce, struct unpack_trees_options *o); static int verify_absent_sparse(const struct cache_entry *ce, enum unpack_trees_error_types, struct unpack_trees_options *o); @@ -1113,38 +1118,33 @@ static void mark_new_skip_worktree(struct exclude_list *el, static int verify_absent(const struct cache_entry *, enum unpack_trees_error_types, struct unpack_trees_options *); /* * N-way merge "len" trees. Returns 0 on success, -1 on failure to manipulate the * resulting index, -2 on failure to reflect the changes to the work tree. * * CE_ADDED, CE_UNPACKED and CE_NEW_SKIP_WORKTREE are used internally */ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options *o) { 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) o->skip_sparse_checkout = 1; if (!o->skip_sparse_checkout) { char *sparse = git_pathdup("info/sparse-checkout"); if (add_excludes_from_file_to_list(sparse, "", 0, &el, 0) < 0) o->skip_sparse_checkout = 1; else o->el = ⪙ free(sparse); } memset(&o->result, 0, sizeof(o->result)); o->result.initialized = 1; @@ -1257,31 +1257,31 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options if (ret < 0) goto return_failed; /* * Sparse checkout is meant to narrow down checkout area * but it does not make sense to narrow down to empty working * tree. This is usually a mistake in sparse checkout rules. * Do not allow users to do that. */ if (o->result.cache_nr && empty_worktree) { ret = unpack_failed(o, "Sparse checkout leaves no entry on working directory"); goto done; } } 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) o->result.cache_tree = cache_tree(); if (!cache_tree_fully_valid(o->result.cache_tree)) cache_tree_update(&o->result, WRITE_TREE_SILENT | WRITE_TREE_REPAIR); } discard_index(o->dst_index); *o->dst_index = o->result; } else { discard_index(&o->result); } -- 2.11.0.31.g919a8d0.dirty ``` ## Stefan Beller, 2017-01-06 21:03 Subject: Re: [PATCHv2] unpack-trees: move checkout state into check_updates Message-ID: URL: https://gitlist.dev/e/CAGZ79kaM%3DUosm7KAvAP-8w59Tyfc7LZiV3ZOr%3DPZnBgMCzr2AA%40mail.gmail.com In-Reply-To: ``` On Sat, Dec 31, 2016 at 5:26 PM, Junio C Hamano wrote: >> Signed-off-by: Stefan Beller >> --- > > I'd add RenĂ©'s Reviewed-by: here. done > > 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...). I will send out a series, that is based on this patch shortly; as I fuzzed again with the small variables, that series doesn't apply on this version but the version to be sent out shortly. > > Queued. Thanks. please replace with the following series. ``` ## Stefan Beller, 2017-01-06 21:03 Subject: [PATCH 5/5] unpack-trees: factor working tree update out of check_updates Message-ID: <20170106210330.31761-6-sbeller@google.com> URL: https://gitlist.dev/e/20170106210330.31761-6-sbeller%40google.com In-Reply-To: <20170106210330.31761-1-sbeller@google.com> ``` This makes check_updates shorter and easier to understand. Signed-off-by: Stefan Beller --- unpack-trees.c | 40 ++++++++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/unpack-trees.c b/unpack-trees.c index b954ec1233..b40c069b1b 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -275,67 +275,79 @@ static struct progress *get_progress(struct unpack_trees_options *o) struct index_state *index = &o->result; if (!o->update || !o->verbose_update) return NULL; for (; cnt < index->cache_nr; cnt++) { const struct cache_entry *ce = index->cache[cnt]; if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE)) total++; } return start_progress_delay(_("Checking out files"), total, 50, 1); } -static int check_updates(struct unpack_trees_options *o) +static int update_working_tree_files(struct unpack_trees_options *o, + struct progress *progress, + unsigned start_cnt) { - unsigned cnt = 0; + unsigned cnt = start_cnt; int i, errs = 0; - struct progress *progress = NULL; struct index_state *index = &o->result; struct checkout state = CHECKOUT_INIT; state.force = 1; state.quiet = 1; state.refresh_cache = 1; state.istate = index; - progress = get_progress(o); - - if (o->update) - git_attr_set_direction(GIT_ATTR_CHECKOUT, index); - - cnt = remove_workingtree_files(o, progress); - remove_marked_cache_entries(index); - remove_scheduled_dirs(); - for (i = 0; i < index->cache_nr; i++) { struct cache_entry *ce = index->cache[i]; if (ce->ce_flags & CE_UPDATE) { if (ce->ce_flags & CE_WT_REMOVE) die("BUG: both update and delete flags are set on %s", ce->name); display_progress(progress, ++cnt); ce->ce_flags &= ~CE_UPDATE; - if (o->update && !o->dry_run) { + if (o->update && !o->dry_run) errs |= checkout_entry(ce, &state, NULL); - } } } + + return errs; +} + +static int check_updates(struct unpack_trees_options *o) +{ + struct progress *progress = NULL; + struct index_state *index = &o->result; + int errs; + unsigned total_removed; + + progress = get_progress(o); + + if (o->update) + git_attr_set_direction(GIT_ATTR_CHECKOUT, index); + + total_removed = remove_workingtree_files(o, progress); + remove_marked_cache_entries(index); + remove_scheduled_dirs(); + errs = update_working_tree_files(o, progress, total_removed); + stop_progress(&progress); if (o->update) git_attr_set_direction(GIT_ATTR_CHECKIN, NULL); return errs != 0; } static int verify_uptodate_sparse(const struct cache_entry *ce, struct unpack_trees_options *o); static int verify_absent_sparse(const struct cache_entry *ce, enum unpack_trees_error_types, struct unpack_trees_options *o); static int apply_sparse_checkout(struct index_state *istate, struct cache_entry *ce, struct unpack_trees_options *o) -- 2.11.0.31.g919a8d0.dirty ``` ## Stefan Beller, 2017-01-06 21:03 Subject: [PATCH 4/5] unpack-trees: factor file removal out of check_updates Message-ID: <20170106210330.31761-5-sbeller@google.com> URL: https://gitlist.dev/e/20170106210330.31761-5-sbeller%40google.com In-Reply-To: <20170106210330.31761-1-sbeller@google.com> ``` This makes check_updates shorter and easier to understand. Signed-off-by: Stefan Beller --- unpack-trees.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/unpack-trees.c b/unpack-trees.c index 971d091fd0..b954ec1233 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -237,30 +237,50 @@ static void display_error_msgs(struct unpack_trees_options *o) } /* * Unlink the last component and schedule the leading directories for * removal, such that empty directories get removed. */ static void unlink_entry(const struct cache_entry *ce) { if (!check_leading_path(ce->name, ce_namelen(ce))) return; if (remove_or_warn(ce->ce_mode, ce->name)) return; schedule_dir_for_removal(ce->name, ce_namelen(ce)); } +static int remove_workingtree_files(struct unpack_trees_options *o, + struct progress *progress) +{ + int i; + unsigned cnt = 0; + struct index_state *index = &o->result; + + for (i = 0; i < index->cache_nr; i++) { + const struct cache_entry *ce = index->cache[i]; + + if (ce->ce_flags & CE_WT_REMOVE) { + display_progress(progress, ++cnt); + if (o->update && !o->dry_run) + unlink_entry(ce); + } + } + + return cnt; +} + static struct progress *get_progress(struct unpack_trees_options *o) { unsigned cnt = 0, total = 0; struct index_state *index = &o->result; if (!o->update || !o->verbose_update) return NULL; for (; cnt < index->cache_nr; cnt++) { const struct cache_entry *ce = index->cache[cnt]; if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE)) total++; } return start_progress_delay(_("Checking out files"), @@ -273,39 +293,32 @@ static int check_updates(struct unpack_trees_options *o) int i, errs = 0; struct progress *progress = NULL; struct index_state *index = &o->result; struct checkout state = CHECKOUT_INIT; state.force = 1; state.quiet = 1; state.refresh_cache = 1; state.istate = index; progress = get_progress(o); if (o->update) git_attr_set_direction(GIT_ATTR_CHECKOUT, index); - for (i = 0; i < index->cache_nr; i++) { - const struct cache_entry *ce = index->cache[i]; - if (ce->ce_flags & CE_WT_REMOVE) { - display_progress(progress, ++cnt); - if (o->update && !o->dry_run) - unlink_entry(ce); - } - } + cnt = remove_workingtree_files(o, progress); remove_marked_cache_entries(index); remove_scheduled_dirs(); for (i = 0; i < index->cache_nr; i++) { struct cache_entry *ce = index->cache[i]; if (ce->ce_flags & CE_UPDATE) { if (ce->ce_flags & CE_WT_REMOVE) die("BUG: both update and delete flags are set on %s", ce->name); display_progress(progress, ++cnt); ce->ce_flags &= ~CE_UPDATE; if (o->update && !o->dry_run) { errs |= checkout_entry(ce, &state, NULL); } -- 2.11.0.31.g919a8d0.dirty ``` ## Stefan Beller, 2017-01-06 21:03 Subject: [PATCH 3/5] unpack-trees: factor progress setup out of check_updates Message-ID: <20170106210330.31761-4-sbeller@google.com> URL: https://gitlist.dev/e/20170106210330.31761-4-sbeller%40google.com In-Reply-To: <20170106210330.31761-1-sbeller@google.com> ``` This makes check_updates shorter and easier to understand. Signed-off-by: Stefan Beller --- unpack-trees.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/unpack-trees.c b/unpack-trees.c index f16ef14294..971d091fd0 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -237,55 +237,63 @@ static void display_error_msgs(struct unpack_trees_options *o) } /* * Unlink the last component and schedule the leading directories for * removal, such that empty directories get removed. */ static void unlink_entry(const struct cache_entry *ce) { if (!check_leading_path(ce->name, ce_namelen(ce))) return; if (remove_or_warn(ce->ce_mode, ce->name)) return; schedule_dir_for_removal(ce->name, ce_namelen(ce)); } -static int check_updates(struct unpack_trees_options *o) +static struct progress *get_progress(struct unpack_trees_options *o) { unsigned cnt = 0, total = 0; + struct index_state *index = &o->result; + + if (!o->update || !o->verbose_update) + return NULL; + + for (; cnt < index->cache_nr; cnt++) { + const struct cache_entry *ce = index->cache[cnt]; + if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE)) + total++; + } + + return start_progress_delay(_("Checking out files"), + total, 50, 1); +} + +static int check_updates(struct unpack_trees_options *o) +{ + unsigned cnt = 0; int i, errs = 0; struct progress *progress = NULL; struct index_state *index = &o->result; struct checkout state = CHECKOUT_INIT; 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++) { - const struct cache_entry *ce = index->cache[cnt]; - if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE)) - total++; - } - - progress = start_progress_delay(_("Checking out files"), - total, 50, 1); - cnt = 0; - } + progress = get_progress(o); if (o->update) git_attr_set_direction(GIT_ATTR_CHECKOUT, index); for (i = 0; i < index->cache_nr; i++) { const struct cache_entry *ce = index->cache[i]; if (ce->ce_flags & CE_WT_REMOVE) { display_progress(progress, ++cnt); if (o->update && !o->dry_run) unlink_entry(ce); } } remove_marked_cache_entries(index); remove_scheduled_dirs(); -- 2.11.0.31.g919a8d0.dirty ``` ## Stefan Beller, 2017-01-06 21:03 Subject: [PATCH 2/5] unpack-trees: remove unneeded continue Message-ID: <20170106210330.31761-3-sbeller@google.com> URL: https://gitlist.dev/e/20170106210330.31761-3-sbeller%40google.com In-Reply-To: <20170106210330.31761-1-sbeller@google.com> ``` The continue is the last statement in the loop, so not needed. This situation arose in 700e66d66 (2010-07-30, unpack-trees: let read-tree -u remove index entries outside sparse area) when statements after the continue were removed. Signed-off-by: Stefan Beller --- unpack-trees.c | 1 - 1 file changed, 1 deletion(-) diff --git a/unpack-trees.c b/unpack-trees.c index 55c75b4d6a..f16ef14294 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -272,31 +272,30 @@ static int check_updates(struct unpack_trees_options *o) progress = start_progress_delay(_("Checking out files"), total, 50, 1); cnt = 0; } if (o->update) git_attr_set_direction(GIT_ATTR_CHECKOUT, index); for (i = 0; i < index->cache_nr; i++) { const struct cache_entry *ce = index->cache[i]; if (ce->ce_flags & CE_WT_REMOVE) { display_progress(progress, ++cnt); if (o->update && !o->dry_run) unlink_entry(ce); - continue; } } remove_marked_cache_entries(index); remove_scheduled_dirs(); for (i = 0; i < index->cache_nr; i++) { struct cache_entry *ce = index->cache[i]; if (ce->ce_flags & CE_UPDATE) { if (ce->ce_flags & CE_WT_REMOVE) die("BUG: both update and delete flags are set on %s", ce->name); display_progress(progress, ++cnt); ce->ce_flags &= ~CE_UPDATE; if (o->update && !o->dry_run) { -- 2.11.0.31.g919a8d0.dirty ``` ## Jeff King, 2017-01-07 01:36 Subject: Re: [PATCH 4/5] unpack-trees: factor file removal out of check_updates Message-ID: <20170107013602.act7aouuzgzpwb54@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170107013602.act7aouuzgzpwb54%40sigill.intra.peff.net In-Reply-To: <20170106210330.31761-5-sbeller@google.com> ``` On Fri, Jan 06, 2017 at 01:03:29PM -0800, Stefan Beller wrote: > +static int remove_workingtree_files(struct unpack_trees_options *o, > + struct progress *progress) > +{ > + int i; > + unsigned cnt = 0; > + struct index_state *index = &o->result; > + > + for (i = 0; i < index->cache_nr; i++) { > + const struct cache_entry *ce = index->cache[i]; > + > + if (ce->ce_flags & CE_WT_REMOVE) { > + display_progress(progress, ++cnt); > + if (o->update && !o->dry_run) > + unlink_entry(ce); > + } > + } > + > + return cnt; > +} "cnt" is unsigned here, as it is in the caller. Should the return value match? -Peff ``` ## Stefan Beller, 2017-01-09 19:30 Subject: Re: [PATCH 4/5] unpack-trees: factor file removal out of check_updates Message-ID: URL: https://gitlist.dev/e/CAGZ79kbZz5s5TdudGcOADDxYShTAm5sTCHRo81SSCc13MoF7WA%40mail.gmail.com In-Reply-To: <20170107013602.act7aouuzgzpwb54@sigill.intra.peff.net> ``` On Fri, Jan 6, 2017 at 5:36 PM, Jeff King wrote: >> +static int remove_workingtree_files(struct unpack_trees_options *o, >> + unsigned cnt = 0; > > "cnt" is unsigned here, as it is in the caller. Should the return value > match? Yes, obviously. :/ Thanks for catching, Stefan ```