{"thread":{"id":"44841","subject":"[PATCHv2 5/5] unpack-trees: factor working tree update out of check_updates","startedAt":"2017-01-09T19:47:01Z","lastAt":"2017-01-10T21:34:52Z","messageCount":10,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"309057","messageId":"20170109194621.17013-6-sbeller@google.com","threadId":"44841","inReplyTo":"20170109194621.17013-1-sbeller@google.com","subject":"[PATCHv2 5/5] unpack-trees: factor working tree update out of check_updates","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-09T19:46:21Z","receivedAt":"2017-01-09T19:47:01Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This makes check_updates shorter and easier to understand.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n unpack-trees.c | 40 ++++++++++++++++++++++++++--------------\n 1 file changed, 26 insertions(+), 14 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex ac59510251..ddb3e3dcff 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -256,12 +256,13 @@ static struct progress *get_progress(struct unpack_trees_options *o)\n \t\t\t\t    total, 50, 1);\n }\n \n-static int check_updates(struct unpack_trees_options *o)\n+static int update_working_tree_files(struct unpack_trees_options *o,\n+\t\t\t\t     struct progress *progress,\n+\t\t\t\t     unsigned start_cnt)\n {\n-\tunsigned cnt = 0;\n+\tunsigned cnt = start_cnt;\n \tint i, errs = 0;\n \n-\tstruct progress *progress = NULL;\n \tstruct index_state *index = &o->result;\n \tstruct checkout state = CHECKOUT_INIT;\n \n@@ -270,15 +271,6 @@ static int check_updates(struct unpack_trees_options *o)\n \tstate.refresh_cache = 1;\n \tstate.istate = index;\n \n-\tprogress = get_progress(o);\n-\n-\tif (o->update)\n-\t\tgit_attr_set_direction(GIT_ATTR_CHECKOUT, index);\n-\n-\tcnt = remove_workingtree_files(o, progress);\n-\tremove_marked_cache_entries(index);\n-\tremove_scheduled_dirs();\n-\n \tfor (i = 0; i < index->cache_nr; i++) {\n \t\tstruct cache_entry *ce = index->cache[i];\n \n@@ -288,11 +280,31 @@ static int check_updates(struct unpack_trees_options *o)\n \t\t\t\t    ce->name);\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\tif (o->update && !o->dry_run)\n \t\t\t\terrs |= checkout_entry(ce, &state, NULL);\n-\t\t\t}\n \t\t}\n \t}\n+\n+\treturn errs;\n+}\n+\n+static int check_updates(struct unpack_trees_options *o)\n+{\n+\tstruct progress *progress = NULL;\n+\tstruct index_state *index = &o->result;\n+\tint errs;\n+\tunsigned total_removed;\n+\n+\tprogress = get_progress(o);\n+\n+\tif (o->update)\n+\t\tgit_attr_set_direction(GIT_ATTR_CHECKOUT, index);\n+\n+\ttotal_removed = remove_workingtree_files(o, progress);\n+\tremove_marked_cache_entries(index);\n+\tremove_scheduled_dirs();\n+\terrs = update_working_tree_files(o, progress, total_removed);\n+\n \tstop_progress(&progress);\n \tif (o->update)\n \t\tgit_attr_set_direction(GIT_ATTR_CHECKIN, NULL);\n-- \n2.11.0.rc2.30.g7c4be45.dirty\n\n"},{"id":"309058","messageId":"20170109194621.17013-1-sbeller@google.com","threadId":"44841","inReplyTo":null,"subject":"[PATCHv2 0/5] refactor unpack-trees","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-09T19:46:16Z","receivedAt":"2017-01-09T19:47:04Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v2:\n\n* Fixed the return type to be unsigned in patch 4.\n\nThis replaces origin/rs/unpack-trees-reduce-file-scope-global,\nthe first patch has the following diff to that branch\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex edf9fa2f6c..8e6768f283 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -221,10 +221,11 @@ static void unlink_entry(const struct cache_entry *ce)\n static int check_updates(struct unpack_trees_options *o)\n {\n        unsigned cnt = 0, total = 0;\n+       int i, errs = 0;\n+\n        struct progress *progress = NULL;\n        struct index_state *index = &o->result;\n        struct checkout state = CHECKOUT_INIT;\n-       int i, errs = 0;\n \n        state.force = 1;\n        state.quiet = 1;\n        \nThanks,\nStefan\n\nv1:\nunpack-trees is a central file needed for the understanding\nof working tree manipulation. To help with the understanding\nrefactor the code to be more readable.\n\nThe first patch was a standalone patch 8 days ago;\nnow incorporated into this series as a v3,\nreducing the scope of the checkout state.\n\nThe second patch removes a single continue statement;\nit needed some digging to explain, but looks trivial.\n\nThe last 3 patches shorten the check_updates function by adding more\nfunctions. If we ever want to parallelize file IO then these smaller\nfunctions would be the scope to do it, keeping the check_updates as\na high level function guiding through the steps what is happening during\na working tree update.\n\nThanks,\nStefan\n\nStefan Beller (5):\n  unpack-trees: move checkout state into check_updates\n  unpack-trees: remove unneeded continue\n  unpack-trees: factor progress setup out of check_updates\n  unpack-trees: factor file removal out of check_updates\n  unpack-trees: factor working tree update out of check_updates\n\n unpack-trees.c | 96 ++++++++++++++++++++++++++++++++++++++--------------------\n 1 file changed, 64 insertions(+), 32 deletions(-)\n\n-- \n2.11.0.31.g919a8d0.dirty\n\n"},{"id":"309059","messageId":"20170109194621.17013-5-sbeller@google.com","threadId":"44841","inReplyTo":"20170109194621.17013-1-sbeller@google.com","subject":"[PATCHv2 4/5] unpack-trees: factor file removal out of check_updates","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-09T19:46:20Z","receivedAt":"2017-01-09T19:47:12Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This makes check_updates shorter and easier to understand.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n unpack-trees.c | 29 +++++++++++++++++++++--------\n 1 file changed, 21 insertions(+), 8 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex b564024472..ac59510251 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -218,6 +218,26 @@ static void unlink_entry(const struct cache_entry *ce)\n \tschedule_dir_for_removal(ce->name, ce_namelen(ce));\n }\n \n+static unsigned remove_workingtree_files(struct unpack_trees_options *o,\n+\t\t\t\t\t struct progress *progress)\n+{\n+\tint i;\n+\tunsigned cnt = 0;\n+\tstruct index_state *index = &o->result;\n+\n+\tfor (i = 0; i < index->cache_nr; i++) {\n+\t\tconst struct cache_entry *ce = index->cache[i];\n+\n+\t\tif (ce->ce_flags & CE_WT_REMOVE) {\n+\t\t\tdisplay_progress(progress, ++cnt);\n+\t\t\tif (o->update && !o->dry_run)\n+\t\t\t\tunlink_entry(ce);\n+\t\t}\n+\t}\n+\n+\treturn cnt;\n+}\n+\n static struct progress *get_progress(struct unpack_trees_options *o)\n {\n \tunsigned cnt = 0, total = 0;\n@@ -254,15 +274,8 @@ static int check_updates(struct unpack_trees_options *o)\n \n \tif (o->update)\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-\t\tif (ce->ce_flags & CE_WT_REMOVE) {\n-\t\t\tdisplay_progress(progress, ++cnt);\n-\t\t\tif (o->update && !o->dry_run)\n-\t\t\t\tunlink_entry(ce);\n-\t\t}\n-\t}\n+\tcnt = remove_workingtree_files(o, progress);\n \tremove_marked_cache_entries(index);\n \tremove_scheduled_dirs();\n \n-- \n2.11.0.rc2.30.g7c4be45.dirty\n\n"},{"id":"309060","messageId":"20170109194621.17013-4-sbeller@google.com","threadId":"44841","inReplyTo":"20170109194621.17013-1-sbeller@google.com","subject":"[PATCHv2 3/5] unpack-trees: factor progress setup out of check_updates","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-09T19:46:19Z","receivedAt":"2017-01-09T19:47:16Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This makes check_updates shorter and easier to understand.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n unpack-trees.c | 32 ++++++++++++++++++++------------\n 1 file changed, 20 insertions(+), 12 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex d443decb23..b564024472 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -218,9 +218,27 @@ 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+static struct progress *get_progress(struct unpack_trees_options *o)\n {\n \tunsigned cnt = 0, total = 0;\n+\tstruct index_state *index = &o->result;\n+\n+\tif (!o->update || !o->verbose_update)\n+\t\treturn NULL;\n+\n+\tfor (; cnt < index->cache_nr; cnt++) {\n+\t\tconst struct cache_entry *ce = index->cache[cnt];\n+\t\tif (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))\n+\t\t\ttotal++;\n+\t}\n+\n+\treturn start_progress_delay(_(\"Checking out files\"),\n+\t\t\t\t    total, 50, 1);\n+}\n+\n+static int check_updates(struct unpack_trees_options *o)\n+{\n+\tunsigned cnt = 0;\n \tint i, errs = 0;\n \n \tstruct progress *progress = NULL;\n@@ -232,17 +250,7 @@ static int check_updates(struct unpack_trees_options *o)\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-\t\t\tconst struct cache_entry *ce = index->cache[cnt];\n-\t\t\tif (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))\n-\t\t\t\ttotal++;\n-\t\t}\n-\n-\t\tprogress = start_progress_delay(_(\"Checking out files\"),\n-\t\t\t\t\t\ttotal, 50, 1);\n-\t\tcnt = 0;\n-\t}\n+\tprogress = get_progress(o);\n \n \tif (o->update)\n \t\tgit_attr_set_direction(GIT_ATTR_CHECKOUT, index);\n-- \n2.11.0.rc2.30.g7c4be45.dirty\n\n"},{"id":"309061","messageId":"20170109194621.17013-2-sbeller@google.com","threadId":"44841","inReplyTo":"20170109194621.17013-1-sbeller@google.com","subject":"[PATCHv2 1/5] unpack-trees: move checkout state into check_updates","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-09T19:46:17Z","receivedAt":"2017-01-09T19:47:19Z","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>\nReviewed-by: René Scharfe <l.s.r@web.de>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n unpack-trees.c | 26 +++++++++++++-------------\n 1 file changed, 13 insertions(+), 13 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex ea6bdd20e0..8e6768f283 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -218,14 +218,19 @@ 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+\tint i, errs = 0;\n+\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+\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 +245,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 +256,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 +269,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 +1099,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 +1238,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.rc2.30.g7c4be45.dirty\n\n"},{"id":"309062","messageId":"20170109194621.17013-3-sbeller@google.com","threadId":"44841","inReplyTo":"20170109194621.17013-1-sbeller@google.com","subject":"[PATCHv2 2/5] unpack-trees: remove unneeded continue","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-09T19:46:18Z","receivedAt":"2017-01-09T19:47:20Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The continue is the last statement in the loop, so not needed.\nThis situation arose in 700e66d66 (2010-07-30, unpack-trees: let\nread-tree -u remove index entries outside sparse area) when statements\nafter the continue were removed.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n unpack-trees.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 8e6768f283..d443decb23 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -253,7 +253,6 @@ static int check_updates(struct unpack_trees_options *o)\n \t\t\tdisplay_progress(progress, ++cnt);\n \t\t\tif (o->update && !o->dry_run)\n \t\t\t\tunlink_entry(ce);\n-\t\t\tcontinue;\n \t\t}\n \t}\n \tremove_marked_cache_entries(index);\n-- \n2.11.0.rc2.30.g7c4be45.dirty\n\n"},{"id":"309153","messageId":"xmqqinpmsna7.fsf@gitster.mtv.corp.google.com","threadId":"44841","inReplyTo":"20170109194621.17013-3-sbeller@google.com","subject":"Re: [PATCHv2 2/5] unpack-trees: remove unneeded continue","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-10T19:55:12Z","receivedAt":"2017-01-10T19:56:00Z","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 continue is the last statement in the loop, so not needed.\n> This situation arose in 700e66d66 (2010-07-30, unpack-trees: let\n> read-tree -u remove index entries outside sparse area) when statements\n> after the continue were removed.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n\nThanks for digging the history to find the culprit.  \n\nThis is an unrelated tangent, but I do not think of a way other than\n\"log -L\" to find it; conceptually, \"blame\" ought to be usable for\nit, but it is not useful for finding deleted lines.\n\n>  unpack-trees.c | 1 -\n>  1 file changed, 1 deletion(-)\n>\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 8e6768f283..d443decb23 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -253,7 +253,6 @@ static int check_updates(struct unpack_trees_options *o)\n>  \t\t\tdisplay_progress(progress, ++cnt);\n>  \t\t\tif (o->update && !o->dry_run)\n>  \t\t\t\tunlink_entry(ce);\n> -\t\t\tcontinue;\n>  \t\t}\n>  \t}\n>  \tremove_marked_cache_entries(index);\n"},{"id":"309154","messageId":"xmqqd1fusmsu.fsf@gitster.mtv.corp.google.com","threadId":"44841","inReplyTo":"20170109194621.17013-5-sbeller@google.com","subject":"Re: [PATCHv2 4/5] unpack-trees: factor file removal out of check_updates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-10T20:05:37Z","receivedAt":"2017-01-10T20:05:44Z","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> This makes check_updates shorter and easier to understand.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n\nI agree that 3/5 made it easier to understand by ejecting a block\nthat is not essential to the functionality of the function out of\nit, making the remainder of the fuction about \"removing gone files\nand then write out the modified files\".  \n\nThe ejecting of the first half of these two operations, both are\nwhat this function is about, done by this step feels backwards.  If\nanything, the \"only do the actual working tree manipulation when not\ndoing a dry-run and told to update\" logic that must be in both are\nspread in two helper functions after step 5/5, and with the added\nboilerplate for these two helpers, the end result becomes _longer_\nto understand what is really going on when check_updates() is\ncalled.\n\nIs the original after step 3/5 too long and hard to understand?\n\n>  unpack-trees.c | 29 +++++++++++++++++++++--------\n>  1 file changed, 21 insertions(+), 8 deletions(-)\n>\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index b564024472..ac59510251 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -218,6 +218,26 @@ static void unlink_entry(const struct cache_entry *ce)\n>  \tschedule_dir_for_removal(ce->name, ce_namelen(ce));\n>  }\n>  \n> +static unsigned remove_workingtree_files(struct unpack_trees_options *o,\n> +\t\t\t\t\t struct progress *progress)\n> +{\n> +\tint i;\n> +\tunsigned cnt = 0;\n> +\tstruct index_state *index = &o->result;\n> +\n> +\tfor (i = 0; i < index->cache_nr; i++) {\n> +\t\tconst struct cache_entry *ce = index->cache[i];\n> +\n> +\t\tif (ce->ce_flags & CE_WT_REMOVE) {\n> +\t\t\tdisplay_progress(progress, ++cnt);\n> +\t\t\tif (o->update && !o->dry_run)\n> +\t\t\t\tunlink_entry(ce);\n> +\t\t}\n> +\t}\n> +\n> +\treturn cnt;\n> +}\n> +\n>  static struct progress *get_progress(struct unpack_trees_options *o)\n>  {\n>  \tunsigned cnt = 0, total = 0;\n> @@ -254,15 +274,8 @@ static int check_updates(struct unpack_trees_options *o)\n>  \n>  \tif (o->update)\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> -\t\tif (ce->ce_flags & CE_WT_REMOVE) {\n> -\t\t\tdisplay_progress(progress, ++cnt);\n> -\t\t\tif (o->update && !o->dry_run)\n> -\t\t\t\tunlink_entry(ce);\n> -\t\t}\n> -\t}\n> +\tcnt = remove_workingtree_files(o, progress);\n>  \tremove_marked_cache_entries(index);\n>  \tremove_scheduled_dirs();\n"},{"id":"309180","messageId":"CAGZ79kbXVCvZuj6rTckGwOfFtRSFpx9p611oKfFLAayTgB242Q@mail.gmail.com","threadId":"44841","inReplyTo":"xmqqd1fusmsu.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv2 4/5] unpack-trees: factor file removal out of check_updates","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-10T20:51:03Z","receivedAt":"2017-01-10T20:51:09Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jan 10, 2017 at 12:05 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> This makes check_updates shorter and easier to understand.\n>>\n>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>> ---\n>\n> I agree that 3/5 made it easier to understand by ejecting a block\n> that is not essential to the functionality of the function out of\n> it, making the remainder of the fuction about \"removing gone files\n> and then write out the modified files\".\n>\n> The ejecting of the first half of these two operations, both are\n> what this function is about, done by this step feels backwards.  If\n> anything, the \"only do the actual working tree manipulation when not\n> doing a dry-run and told to update\" logic that must be in both are\n> spread in two helper functions after step 5/5, and with the added\n> boilerplate for these two helpers, the end result becomes _longer_\n> to understand what is really going on when check_updates() is\n> called.\n>\n> Is the original after step 3/5 too long and hard to understand?\n\nIt is ok to understand for the current state of the world,\nso please drop 4 and 5 for now.\n\nIn a future, when\n* working tree operations would learn about threading or\n* working tree operations were aware of submodules\nthen steps of 4 and 5 would be longer, such that the check_updates\nfunction may require further splitting up.\n\nAs far as I understand the operations now, a working tree operation\nis done like this:\n\n* First compute the new index, how the world would look like\n  (don't touch a thing in the working tree, it is like a complete\n  dry run, that just checks for potential loss of data, e.g.\n  untracked files, merge conflicts etc)\n* remove all files to be marked for deletion\n* add and update all files that are new or change content.\n\ncheck_updates does the last two things listed here, which in the\ngrand scheme of things looked odd to me.\n\nWhen introducing e.g. threaded checkout, then each of the functions\nintroduced in patch 4&5 would be one compartment, i.e. the function\nremove_workingtree_files would need to have all its tasks finished\nbefore we even queue up tasks for updating files, such that we\ndo not run into D/F conflicts racily.\n\nSo yeah, maybe this is too much future-visionary thinking, so please\ndrop 4/5; I can revive them if needed. I think I mainly did them\nfor my own clarity and understanding.\n\nThanks,\nStefan\n"},{"id":"309182","messageId":"xmqq37gqr43w.fsf@gitster.mtv.corp.google.com","threadId":"44841","inReplyTo":"CAGZ79kbXVCvZuj6rTckGwOfFtRSFpx9p611oKfFLAayTgB242Q@mail.gmail.com","subject":"Re: [PATCHv2 4/5] unpack-trees: factor file removal out of check_updates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-10T21:34:43Z","receivedAt":"2017-01-10T21:34:52Z","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> As far as I understand the operations now, a working tree operation\n> is done like this:\n>\n> * First compute the new index, how the world would look like\n>   (don't touch a thing in the working tree, it is like a complete\n>   dry run, that just checks for potential loss of data, e.g.\n>   untracked files, merge conflicts etc)\n> * remove all files to be marked for deletion\n> * add and update all files that are new or change content.\n>\n> check_updates does the last two things listed here, which in the\n> grand scheme of things looked odd to me.\n\nIn the grand scheme of things, the flow has always been understood\nmore like this two-step process:\n\n    - compute the result in core (rejecting an inconsistent result\n      before touching the working tree)\n\n    - reflect the result to the working tree\n\nand the latter is done by check_updates().  \n\nRemoving gone entries before instanciating possibly new entries is\ndone in order to make room where we can create a new path D/F in the\nresult, after removing an old path D [*1*].  But it is merely an\nimplementation detail of the latter, i.e. \"reflect the result to the\nworking tree.\n\nIt is arguably true that check_updates() is not about \"checking\" but\nis about \"doing\", hence is misnamed.\n\n[Footnote]\n\n*1* If the result computed in-core wants to create D/F, it must have\nremoval of D---otherwise the result is inconsistent.\n"}]}