{"thread":{"id":"44791","subject":"[PATCHv2] unpack-trees: move checkout state into check_updates","startedAt":"2016-12-29T19:43:16Z","lastAt":"2017-01-01T01:26:43Z","messageCount":2,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"308498","messageId":"20161229194309.2373-1-sbeller@google.com","threadId":"44791","inReplyTo":null,"subject":"[PATCHv2] unpack-trees: move checkout state into check_updates","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-29T19:43:09Z","receivedAt":"2016-12-29T19:43:16Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The checkout state was introduced via 16da134b1f9\n(read-trees: refactor the unpack_trees() part, 2006-07-30). An attempt to\nrefactor the checkout state was done in b56aa5b268e (unpack-trees: pass\ncheckout state explicitly to check_updates(), 2016-09-13), but we can\ngo even further.\n\nThe `struct checkout state` is not used in unpack_trees apart from\ninitializing it, so move it into the function that makes use of it,\nwhich is `check_updates`.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nRene wrote:\n> we could use \"index\" instead of \"&o->result\".  Not sure if it's worth a patch, though.\n\ndone in this iteration of the patch.\nI also reordered the variables at the beginning of the function for readability.\n\nThanks,\nStefan\n\n unpack-trees.c | 25 ++++++++++++-------------\n 1 file changed, 12 insertions(+), 13 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 7a6df99d10..eb44f50e65 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -218,14 +218,18 @@ static void unlink_entry(const struct cache_entry *ce)\n \tschedule_dir_for_removal(ce->name, ce_namelen(ce));\n }\n \n-static int check_updates(struct unpack_trees_options *o,\n-\t\t\t const struct checkout *state)\n+static int check_updates(struct unpack_trees_options *o)\n {\n \tunsigned cnt = 0, total = 0;\n \tstruct progress *progress = NULL;\n \tstruct index_state *index = &o->result;\n-\tint i;\n-\tint errs = 0;\n+\tstruct checkout state = CHECKOUT_INIT;\n+\tint i, errs = 0;\n+\n+\tstate.force = 1;\n+\tstate.quiet = 1;\n+\tstate.refresh_cache = 1;\n+\tstate.istate = index;\n \n \tif (o->update && o->verbose_update) {\n \t\tfor (total = cnt = 0; cnt < index->cache_nr; cnt++) {\n@@ -240,7 +244,7 @@ static int check_updates(struct unpack_trees_options *o,\n \t}\n \n \tif (o->update)\n-\t\tgit_attr_set_direction(GIT_ATTR_CHECKOUT, &o->result);\n+\t\tgit_attr_set_direction(GIT_ATTR_CHECKOUT, index);\n \tfor (i = 0; i < index->cache_nr; i++) {\n \t\tconst struct cache_entry *ce = index->cache[i];\n \n@@ -251,7 +255,7 @@ static int check_updates(struct unpack_trees_options *o,\n \t\t\tcontinue;\n \t\t}\n \t}\n-\tremove_marked_cache_entries(&o->result);\n+\tremove_marked_cache_entries(index);\n \tremove_scheduled_dirs();\n \n \tfor (i = 0; i < index->cache_nr; i++) {\n@@ -264,7 +268,7 @@ static int check_updates(struct unpack_trees_options *o,\n \t\t\tdisplay_progress(progress, ++cnt);\n \t\t\tce->ce_flags &= ~CE_UPDATE;\n \t\t\tif (o->update && !o->dry_run) {\n-\t\t\t\terrs |= checkout_entry(ce, state, NULL);\n+\t\t\t\terrs |= checkout_entry(ce, &state, NULL);\n \t\t\t}\n \t\t}\n \t}\n@@ -1094,14 +1098,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \tint i, ret;\n \tstatic struct cache_entry *dfc;\n \tstruct exclude_list el;\n-\tstruct checkout state = CHECKOUT_INIT;\n \n \tif (len > MAX_UNPACK_TREES)\n \t\tdie(\"unpack_trees takes at most %d trees\", MAX_UNPACK_TREES);\n-\tstate.force = 1;\n-\tstate.quiet = 1;\n-\tstate.refresh_cache = 1;\n-\tstate.istate = &o->result;\n \n \tmemset(&el, 0, sizeof(el));\n \tif (!core_apply_sparse_checkout || !o->update)\n@@ -1238,7 +1237,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t}\n \n \to->src_index = NULL;\n-\tret = check_updates(o, &state) ? (-2) : 0;\n+\tret = check_updates(o) ? (-2) : 0;\n \tif (o->dst_index) {\n \t\tif (!ret) {\n \t\t\tif (!o->result.cache_tree)\n-- \n2.11.0.259.ga95e92af08.dirty\n\n"},{"id":"308560","messageId":"xmqq8tqv6051.fsf@gitster.mtv.corp.google.com","threadId":"44791","inReplyTo":"20161229194309.2373-1-sbeller@google.com","subject":"Re: [PATCHv2] unpack-trees: move checkout state into check_updates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-01T01:26:34Z","receivedAt":"2017-01-01T01:26:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> The checkout state was introduced via 16da134b1f9\n> (read-trees: refactor the unpack_trees() part, 2006-07-30). An attempt to\n> refactor the checkout state was done in b56aa5b268e (unpack-trees: pass\n> checkout state explicitly to check_updates(), 2016-09-13), but we can\n> go even further.\n>\n> The `struct checkout state` is not used in unpack_trees apart from\n> initializing it, so move it into the function that makes use of it,\n> which is `check_updates`.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n\nI'd add René's Reviewed-by: here.\n\n> -static int check_updates(struct unpack_trees_options *o,\n> -\t\t\t const struct checkout *state)\n> +static int check_updates(struct unpack_trees_options *o)\n>  {\n>  \tunsigned cnt = 0, total = 0;\n>  \tstruct progress *progress = NULL;\n>  \tstruct index_state *index = &o->result;\n> -\tint i;\n> -\tint errs = 0;\n> +\tstruct checkout state = CHECKOUT_INIT;\n> +\tint i, errs = 0;\n> +\n> +\tstate.force = 1;\n> +\tstate.quiet = 1;\n> +\tstate.refresh_cache = 1;\n> +\tstate.istate = index;\n\nI think moving heavier and initialized variables earlier and more\nlightweight and ephemeral ones like \"i\" later does make it easier to\nfollow.  \"errs\" has the significance and the lifetime similar to\ncnt/total, and logically should be higher, though.  It is not a big\nenough deal to reroll (but as your futzing of the variable definition\norder was not a big enough deal to do in this patch, either, so...).\n\nQueued.  Thanks.\n"}]}