{"thread":{"id":"44077","subject":"[PATCH] unpack-trees: pass checkout state explicitly to check_updates()","startedAt":"2016-09-13T17:37:43Z","lastAt":"2016-09-13T23:26:05Z","messageCount":2,"participants":["René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"301860","messageId":"b57982ca-d508-5016-a187-fc3b829f9b0c@web.de","threadId":"44077","inReplyTo":null,"subject":"[PATCH] unpack-trees: pass checkout state explicitly to check_updates()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-09-13T17:37:06Z","receivedAt":"2016-09-13T17:37:43Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add a parameter for the struct checkout variable to check_updates()\ninstead of using a static global variable.  Passing it explicitly makes\nobject ownership and usage more easily apparent.  And we get rid of a\nstatic variable; those can be problematic in library-like code.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n unpack-trees.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 11c37fb..74d6dd4 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -218,8 +218,8 @@ static void unlink_entry(const struct cache_entry *ce)\n \tschedule_dir_for_removal(ce->name, ce_namelen(ce));\n }\n \n-static struct checkout state;\n-static int check_updates(struct unpack_trees_options *o)\n+static int check_updates(struct unpack_trees_options *o,\n+\t\t\t const struct checkout *state)\n {\n \tunsigned cnt = 0, total = 0;\n \tstruct progress *progress = NULL;\n@@ -264,7 +264,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,6 +1094,7 @@ 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;\n \n \tif (len > MAX_UNPACK_TREES)\n \t\tdie(\"unpack_trees takes at most %d trees\", MAX_UNPACK_TREES);\n@@ -1239,7 +1240,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) ? (-2) : 0;\n+\tret = check_updates(o, &state) ? (-2) : 0;\n \tif (o->dst_index) {\n \t\tif (!ret) {\n \t\t\tif (!o->result.cache_tree)\n-- \n2.10.0\n\n"},{"id":"301887","messageId":"xmqqa8fbtm9w.fsf@gitster.mtv.corp.google.com","threadId":"44077","inReplyTo":"b57982ca-d508-5016-a187-fc3b829f9b0c@web.de","subject":"Re: [PATCH] unpack-trees: pass checkout state explicitly to check_updates()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-13T23:25:47Z","receivedAt":"2016-09-13T23:26:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Add a parameter for the struct checkout variable to check_updates()\n> instead of using a static global variable.  Passing it explicitly makes\n> object ownership and usage more easily apparent.  And we get rid of a\n> static variable; those can be problematic in library-like code.\n> ...\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 11c37fb..74d6dd4 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -218,8 +218,8 @@ static void unlink_entry(const struct cache_entry *ce)\n>  \tschedule_dir_for_removal(ce->name, ce_namelen(ce));\n>  }\n>  \n> -static struct checkout state;\n\n> ...\n> @@ -1094,6 +1094,7 @@ 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;\n\nDoes the distinction between this thing in BSS implicitly cleared\nand the new one on stack that does not seem to have any\ninitialization matter?\n\n\t... goes and looks ...\n\nOK, after this hunk we clear and set up everything in state, so\nthere is no difference in behaviour.  Just we got rid of an\nunnecessary file-scope global.\n\nNice.  Thanks.\n  \n>  \tif (len > MAX_UNPACK_TREES)\n>  \t\tdie(\"unpack_trees takes at most %d trees\", MAX_UNPACK_TREES);\n> @@ -1239,7 +1240,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) ? (-2) : 0;\n> +\tret = check_updates(o, &state) ? (-2) : 0;\n>  \tif (o->dst_index) {\n>  \t\tif (!ret) {\n>  \t\t\tif (!o->result.cache_tree)\n"}]}