{"thread":{"id":"52559","subject":"[PATCH 1/1] unpack-trees: exit check_updates() early if updates are not wanted","startedAt":"2020-01-03T21:42:17Z","lastAt":"2020-01-07T16:38:08Z","messageCount":7,"participants":["Elijah Newren via GitGitGadget","Junio C Hamano","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"389200","messageId":"de0f381284cfe90c1bd8521abb8d29e3529c981a.1578087730.git.gitgitgadget@gmail.com","threadId":"52559","inReplyTo":"pull.686.git.git.1578087730.gitgitgadget@gmail.com","subject":"[PATCH 1/1] unpack-trees: exit check_updates() early if updates are not wanted","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-03T21:42:09Z","receivedAt":"2020-01-03T21:42:17Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\ncheck_updates() has a lot of code that repeatedly checks whether\no->update or o->dry_run are set. (Note that o->dry_run is a\nnear-synonym for !o->update, but not quite as per commit 2c9078d05bf2\n(\"unpack-trees: add the dry_run flag to unpack_trees_options\",\n2011-05-25).) In fact, this function almost turns into a no-op whenever\nthe condition\n !o->update || o->dry_run\nis met. Simplify the code by checking this condition at the beginning\nof the function, and when it is true, do the few things that are\nrelevant and return early.\n\nThere are a few things that make the conversion not quite obvious:\n * The fact that check_updates() does not actually turn into a no-op\n when updates are not wanted may be slightly surprising. However,\n commit 33ecf7eb61 (Discard \"deleted\" cache entries after using them\n to update the working tree, 2008-02-07) put the discarding of\n unused cache entries in check_updates() so we still need to keep\n the call to remove_marked_cache_entries(). It's possible this\n call belongs in another function, but it is certainly needed as\n tests will fail if it is removed.\n * The original called remove_scheduled_dirs() unconditionally.\n Technically, commit 7847892716 (unlink_entry(): introduce\n schedule_dir_for_removal(), 2009-02-09) should have made that call\n conditional, but it didn't matter in practice because\n remove_scheduled_dirs() becomes a no-op when all the calls to\n unlink_entry() are skipped. As such, we do not need to call it.\n * When (o->dry_run && o->update), the original would have two calls\n to git_attr_set_direction() surrounding a bunch of skipped updates.\n These two calls to git_attr_set_direction() cancel each other out\n and thus can be omitted when o->dry_run is true just as they\n already are when !o->update.\n\nSigned-off-by: Elijah Newren \n---\n unpack-trees.c | 26 ++++++++++++++------------\n 1 file changed, 14 insertions(+), 12 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 2399b6818b..4c68dbdb43 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -372,15 +372,20 @@ static int check_updates(struct unpack_trees_options *o)\n \tstate.refresh_cache = 1;\n \tstate.istate = index;\n \n+\tif (!o->update || o->dry_run) {\n+\t\tremove_marked_cache_entries(index, 0);\n+\t\ttrace_performance_leave(\"check_updates\");\n+\t\treturn 0;\n+\t}\n+\n \tif (o->clone)\n \t\tsetup_collided_checkout_detection(&state, index);\n \n \tprogress = get_progress(o);\n \n-\tif (o->update)\n-\t\tgit_attr_set_direction(GIT_ATTR_CHECKOUT);\n+\tgit_attr_set_direction(GIT_ATTR_CHECKOUT);\n \n-\tif (should_update_submodules() && o->update && !o->dry_run)\n+\tif (should_update_submodules())\n \t\tload_gitmodules_file(index, NULL);\n \n \tfor (i = 0; i < index->cache_nr; i++) {\n@@ -388,18 +393,18 @@ static int check_updates(struct unpack_trees_options *o)\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\tunlink_entry(ce);\n \t\t}\n \t}\n+\n \tremove_marked_cache_entries(index, 0);\n \tremove_scheduled_dirs();\n \n-\tif (should_update_submodules() && o->update && !o->dry_run)\n+\tif (should_update_submodules())\n \t\tload_gitmodules_file(index, &state);\n \n \tenable_delayed_checkout(&state);\n-\tif (has_promisor_remote() && o->update && !o->dry_run) {\n+\tif (has_promisor_remote()) {\n \t\t/*\n \t\t * Prefetch the objects that are to be checked out in the loop\n \t\t * below.\n@@ -431,15 +436,12 @@ 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\t\terrs |= checkout_entry(ce, &state, NULL, NULL);\n-\t\t\t}\n+\t\t\terrs |= checkout_entry(ce, &state, NULL, NULL);\n \t\t}\n \t}\n \tstop_progress(&progress);\n \terrs |= finish_delayed_checkout(&state, NULL);\n-\tif (o->update)\n-\t\tgit_attr_set_direction(GIT_ATTR_CHECKIN);\n+\tgit_attr_set_direction(GIT_ATTR_CHECKIN);\n \n \tif (o->clone)\n \t\treport_collided_checkout(index);\n-- \ngitgitgadget\n"},{"id":"389201","messageId":"pull.686.git.git.1578087730.gitgitgadget@gmail.com","threadId":"52559","inReplyTo":null,"subject":"[PATCH 0/1] unpack-trees: exit check_updates() early if updates are not wanted","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-03T21:42:08Z","receivedAt":"2020-01-03T21:42:17Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"In trying to understand check_updates(), I found I was able to simplify the\nfunction by making it exit early when updates are not wanted.\n\nElijah Newren (1):\n unpack-trees: exit check_updates() early if updates are not wanted\n\n unpack-trees.c | 26 ++++++++++++++------------\n 1 file changed, 14 insertions(+), 12 deletions(-)\n\n\nbase-commit: 8679ef24ed64018bb62170c43ce73e0261c0600a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-686%2Fnewren%2Fsimplify-check-updates-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-686/newren/simplify-check-updates-v1\nPull-Request: https://github.com/git/git/pull/686\n-- \ngitgitgadget\n"},{"id":"389234","messageId":"xmqqimlqbx61.fsf@gitster-ct.c.googlers.com","threadId":"52559","inReplyTo":"de0f381284cfe90c1bd8521abb8d29e3529c981a.1578087730.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] unpack-trees: exit check_updates() early if updates are not wanted","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-04T23:48:38Z","receivedAt":"2020-01-04T23:52:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> +\tif (!o->update || o->dry_run) {\n> +\t\tremove_marked_cache_entries(index, 0);\n> +\t\ttrace_performance_leave(\"check_updates\");\n> +\t\treturn 0;\n> +\t}\n\nOK, so the idea is that bunch of codepaths we see later are\nprotected by \"o->update && !o->dry_run\" in the original and are not\nexecuted, so we can leave early here.\n\nSo the primary task of the reviewers of this patch is to see if the\nremainder of the function has some code that were *not* no-op when\n(!o->update || o->dry_run) is true in the current code, which would\nindicate possible behaviour change, and assess if the change in\nbehaviour matters in the real world if there is.\n\n> \tif (o->clone)\n> \t\tsetup_collided_checkout_detection(&state, index);\n\nIf there were such a thing as dry-run of a clone, we will stop\ncalling the report based on the thing we are setting up here?\nPresumably that does not happen in the current code---is that\nsomething we guarantee in the future evolution of the code, though?\n\n> \tprogress = get_progress(o);\n\nget_progress() would yield NULL when !o->update, but o->dry_run does\nnot influence it, so we would have called the progress API in\ntoday's code, if o->dry_run and o->update are both true.\n\nPresumably, o->update and o->dry_run are not true at the same time\nin the current code---but even if in the future we start supporting\nthe combination, dry-run should be skipping the filesystem update\n(which is the slow part) and lack of progress may not matter, I\nguess?\n\nIt seems to me that unpack_trees_options.dry_run can become true\nonly in \"git read-tree\" when the -n (or --dry-run) option is given\nand no other codepath touches it. Am I reading the code correctly?\n\nSimilarly, unpack_trees_options.clone is turned on only in the\nbuiltin/clone.c::checkout(). It _might_ occur to future developers\nthat \"git clone --no-checkout\" is better implemented by actually\ngoing through builtin/clone.c::checkout() with .dry_run turned on,\ninstead of leaving that function early. That would for example\nallow collided_checkout() detection to still be done---in such a\nfuture, is the optimization this patch makes still safe, I wonder?\n\n> -\tif (o->update)\n> -\t\tgit_attr_set_direction(GIT_ATTR_CHECKOUT);\n> +\tgit_attr_set_direction(GIT_ATTR_CHECKOUT);\n> \n> -\tif (should_update_submodules() && o->update && !o->dry_run)\n> +\tif (should_update_submodules())\n> \t\tload_gitmodules_file(index, NULL);\n\nGood (no behaviour change because this wouldn't have been done under\nthe early-exit condition anyway).\n> \n> \tfor (i = 0; i < index->cache_nr; i++) {\n> @@ -388,18 +393,18 @@ static int check_updates(struct unpack_trees_options *o)\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\tunlink_entry(ce);\n\nGood (no behaviour change because this wouldn't have been done under\nthe early-exit condition anyway).\n\n> \t\t}\n> \t}\n> +\n> \tremove_marked_cache_entries(index, 0);\n> \tremove_scheduled_dirs();\n> \n> -\tif (should_update_submodules() && o->update && !o->dry_run)\n> +\tif (should_update_submodules())\n> \t\tload_gitmodules_file(index, &state);\n\nGood (no behaviour change because this wouldn't have been done under\nthe early-exit condition anyway).\n> \n> \tenable_delayed_checkout(&state);\n> -\tif (has_promisor_remote() && o->update && !o->dry_run) {\n> +\tif (has_promisor_remote()) {\n\nGood (no behaviour change because this wouldn't have been done under\nthe early-exit condition anyway).\n\n> \t\t/*\n> \t\t * Prefetch the objects that are to be checked out in the loop\n> \t\t * below.\n> @@ -431,15 +436,12 @@ 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\t\terrs |= checkout_entry(ce, &state, NULL, NULL);\n> -\t\t\t}\n> +\t\t\terrs |= checkout_entry(ce, &state, NULL, NULL);\n\nGood (no behaviour change because this wouldn't have been done under\nthe early-exit condition anyway).\n\n> \t\t}\n> \t}\n> \tstop_progress(&progress);\n> \terrs |= finish_delayed_checkout(&state, NULL);\n> -\tif (o->update)\n> -\t\tgit_attr_set_direction(GIT_ATTR_CHECKIN);\n> +\tgit_attr_set_direction(GIT_ATTR_CHECKIN);\n> \n> \tif (o->clone)\n> \t\treport_collided_checkout(index);\n\nBehaviour around this one (and the corresponding setup) may make a\ndifference before and after the patch to future developers (who may\nneed to revert this change to achieve what they want to do), but I\nthink it is a no-op clean-up for today's code.\n\n"},{"id":"389236","messageId":"CABPp-BGeVd-Bbh=nKWL1B9mmvG_Pf6ckEH7DVc5M5A=z4Yx1wA@mail.gmail.com","threadId":"52559","inReplyTo":"xmqqimlqbx61.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] unpack-trees: exit check_updates() early if updates are not wanted","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-01-05T00:49:15Z","receivedAt":"2020-01-05T00:49:26Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Junio,\n\nOn Sat, Jan 4, 2020 at 3:48 PM Junio C Hamano wrote:\n>\n> \"Elijah Newren via GitGitGadget\" writes:\n>\n> > + if (!o->update || o->dry_run) {\n> > + remove_marked_cache_entries(index, 0);\n> > + trace_performance_leave(\"check_updates\");\n> > + return 0;\n> > + }\n>\n> OK, so the idea is that bunch of codepaths we see later are\n> protected by \"o->update && !o->dry_run\" in the original and are not\n> executed, so we can leave early here.\n>\n> So the primary task of the reviewers of this patch is to see if the\n> remainder of the function has some code that were *not* no-op when\n> (!o->update || o->dry_run) is true in the current code, which would\n> indicate possible behaviour change, and assess if the change in\n> behaviour matters in the real world if there is.\n>\n> > if (o->clone)\n> > setup_collided_checkout_detection(&state, index);\n>\n> If there were such a thing as dry-run of a clone, we will stop\n> calling the report based on the thing we are setting up here?\n> Presumably that does not happen in the current code---is that\n> something we guarantee in the future evolution of the code, though?\n\nsetup_collided_checkout_detection() clears the CE_MATCHED flag for\neach cache entry, which can be updated via way of checkout_entry()'s\ncall to mark_colliding_entries(). Then the trailing\nreport_collided_checkout() at the end of the function will report any\npaths with CE_MATCHED set. However, when we're in a dry-run, all\ncheckout_entry() calls are skipped, so we won't detect ever detect any\ncollided entries. As such, there's no point in calling either the\nsetup_collided_checkout_detection() or report_collided_checkout()\nfuntions.\n\nSorry for forgetting to include this in the commit message.\n\n> > progress = get_progress(o);\n>\n> get_progress() would yield NULL when !o->update, but o->dry_run does\n> not influence it, so we would have called the progress API in\n> today's code, if o->dry_run and o->update are both true.\n>\n> Presumably, o->update and o->dry_run are not true at the same time\n> in the current code---but even if in the future we start supporting\n> the combination, dry-run should be skipping the filesystem update\n> (which is the slow part) and lack of progress may not matter, I\n> guess?\n\nThe point of the progress display was to show how long it takes to do\nthe updates. For a dry-run, that time is zero because we skip the\nupdates. And if we're skipping the updates, shouldn't we also skip\nmeasuring how long it takes to do them?\n\nYeah, I should have included this in the commit message too.\n\n> It seems to me that unpack_trees_options.dry_run can become true\n> only in \"git read-tree\" when the -n (or --dry-run) option is given\n> and no other codepath touches it. Am I reading the code correctly?\n\nI hadn't checked previously, but that looks right to me.\n\n> Similarly, unpack_trees_options.clone is turned on only in the\n> builtin/clone.c::checkout(). It _might_ occur to future developers\n> that \"git clone --no-checkout\" is better implemented by actually\n> going through builtin/clone.c::checkout() with .dry_run turned on,\n> instead of leaving that function early. That would for example\n> allow collided_checkout() detection to still be done---in such a\n> future, is the optimization this patch makes still safe, I wonder?\n\nIf it is intended that a report of colliding paths be given when doing\na dry-run using unpack_trees, then the current code is quite buggy\nbecause it'll never report anything. In order to report anything, the\nmechanism for detecting them would need to be drastically reworked.\nAs such, I'd view the optimization as at least making it more obvious\nwhy the code won't report collisions with dry-run rather than\npretending it will. Or are you saying that I've just uncovered a bug\nand it should really be fixed rather than exploited for an\noptimization?\n\n> > - if (o->update)\n> > - git_attr_set_direction(GIT_ATTR_CHECKOUT);\n> > + git_attr_set_direction(GIT_ATTR_CHECKOUT);\n> >\n> > - if (should_update_submodules() && o->update && !o->dry_run)\n> > + if (should_update_submodules())\n> > load_gitmodules_file(index, NULL);\n>\n> Good (no behaviour change because this wouldn't have been done under\n> the early-exit condition anyway).\n> >\n> > for (i = 0; i < index->cache_nr; i++) {\n> > @@ -388,18 +393,18 @@ static int check_updates(struct unpack_trees_options *o)\n> >\n> > if (ce->ce_flags & CE_WT_REMOVE) {\n> > display_progress(progress, ++cnt);\n> > - if (o->update && !o->dry_run)\n> > - unlink_entry(ce);\n> > + unlink_entry(ce);\n>\n> Good (no behaviour change because this wouldn't have been done under\n> the early-exit condition anyway).\n>\n> > }\n> > }\n> > +\n> > remove_marked_cache_entries(index, 0);\n> > remove_scheduled_dirs();\n> >\n> > - if (should_update_submodules() && o->update && !o->dry_run)\n> > + if (should_update_submodules())\n> > load_gitmodules_file(index, &state);\n>\n> Good (no behaviour change because this wouldn't have been done under\n> the early-exit condition anyway).\n> >\n> > enable_delayed_checkout(&state);\n> > - if (has_promisor_remote() && o->update && !o->dry_run) {\n> > + if (has_promisor_remote()) {\n>\n> Good (no behaviour change because this wouldn't have been done under\n> the early-exit condition anyway).\n>\n> > /*\n> > * Prefetch the objects that are to be checked out in the loop\n> > * below.\n> > @@ -431,15 +436,12 @@ static int check_updates(struct unpack_trees_options *o)\n> > ce->name);\n> > display_progress(progress, ++cnt);\n> > ce->ce_flags &= ~CE_UPDATE;\n> > - if (o->update && !o->dry_run) {\n> > - errs |= checkout_entry(ce, &state, NULL, NULL);\n> > - }\n> > + errs |= checkout_entry(ce, &state, NULL, NULL);\n>\n> Good (no behaviour change because this wouldn't have been done under\n> the early-exit condition anyway).\n>\n> > }\n> > }\n> > stop_progress(&progress);\n> > errs |= finish_delayed_checkout(&state, NULL);\n> > - if (o->update)\n> > - git_attr_set_direction(GIT_ATTR_CHECKIN);\n> > + git_attr_set_direction(GIT_ATTR_CHECKIN);\n> >\n> > if (o->clone)\n> > report_collided_checkout(index);\n>\n> Behaviour around this one (and the corresponding setup) may make a\n> difference before and after the patch to future developers (who may\n> need to revert this change to achieve what they want to do), but I\n> think it is a no-op clean-up for today's code.\n"},{"id":"389340","messageId":"pull.686.v2.git.git.1578380277.gitgitgadget@gmail.com","threadId":"52559","inReplyTo":"pull.686.git.git.1578087730.gitgitgadget@gmail.com","subject":"[PATCH v2 0/1] unpack-trees: exit check_updates() early if updates are not wanted","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-07T06:57:56Z","receivedAt":"2020-01-07T06:58:06Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"In trying to understand check_updates(), I found I was able to simplify the\nfunction by making it exit early when updates are not wanted.\n\nChanges since v1:\n\n * Added two bullet points to the end of the commit message to explain the\n questions Junio brought up about function calls that are skipped by the\n early return.\n\nElijah Newren (1):\n unpack-trees: exit check_updates() early if updates are not wanted\n\n unpack-trees.c | 26 ++++++++++++++------------\n 1 file changed, 14 insertions(+), 12 deletions(-)\n\n\nbase-commit: 8679ef24ed64018bb62170c43ce73e0261c0600a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-686%2Fnewren%2Fsimplify-check-updates-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-686/newren/simplify-check-updates-v2\nPull-Request: https://github.com/git/git/pull/686\n\nRange-diff vs v1:\n\n 1: de0f381284 ! 1: dd27727332 unpack-trees: exit check_updates() early if updates are not wanted\n @@ -33,6 +33,21 @@\n These two calls to git_attr_set_direction() cancel each other out\n and thus can be omitted when o->dry_run is true just as they\n already are when !o->update.\n + * The code would previously call setup_collided_checkout_detection()\n + and report_collided_checkout() even when o->dry_run. However, this\n + was just an expensive no-op because\n + setup_collided_checkout_detection() merely cleared the CE_MATCHED\n + flag for each cache entry, and report_collided_checkout() reported\n + which ones had it set. Since a dry-run would skip all the\n + checkout_entry() calls, CE_MATCHED would never get set and thus\n + no collisions would be reported. Since we can't detect the\n + collisions anyway without doing updates, skipping the collisions\n + detection setup and reporting is an optimization.\n + * The code previously would call get_progress() and\n + display_progress() even when (!o->update || o->dry_run). This\n + served to show how long it took to skip all the updates, which is\n + somewhat useless. Since we are skipping the updates, we can skip\n + showing how long it takes to skip them.\n \n Signed-off-by: Elijah Newren \n \n\n-- \ngitgitgadget\n"},{"id":"389341","messageId":"dd277273324eaba5e1f4a368bf2e4d046033c776.1578380277.git.gitgitgadget@gmail.com","threadId":"52559","inReplyTo":"pull.686.v2.git.git.1578380277.gitgitgadget@gmail.com","subject":"[PATCH v2 1/1] unpack-trees: exit check_updates() early if updates are not wanted","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-07T06:57:57Z","receivedAt":"2020-01-07T06:58:13Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\ncheck_updates() has a lot of code that repeatedly checks whether\no->update or o->dry_run are set. (Note that o->dry_run is a\nnear-synonym for !o->update, but not quite as per commit 2c9078d05bf2\n(\"unpack-trees: add the dry_run flag to unpack_trees_options\",\n2011-05-25).) In fact, this function almost turns into a no-op whenever\nthe condition\n !o->update || o->dry_run\nis met. Simplify the code by checking this condition at the beginning\nof the function, and when it is true, do the few things that are\nrelevant and return early.\n\nThere are a few things that make the conversion not quite obvious:\n * The fact that check_updates() does not actually turn into a no-op\n when updates are not wanted may be slightly surprising. However,\n commit 33ecf7eb61 (Discard \"deleted\" cache entries after using them\n to update the working tree, 2008-02-07) put the discarding of\n unused cache entries in check_updates() so we still need to keep\n the call to remove_marked_cache_entries(). It's possible this\n call belongs in another function, but it is certainly needed as\n tests will fail if it is removed.\n * The original called remove_scheduled_dirs() unconditionally.\n Technically, commit 7847892716 (unlink_entry(): introduce\n schedule_dir_for_removal(), 2009-02-09) should have made that call\n conditional, but it didn't matter in practice because\n remove_scheduled_dirs() becomes a no-op when all the calls to\n unlink_entry() are skipped. As such, we do not need to call it.\n * When (o->dry_run && o->update), the original would have two calls\n to git_attr_set_direction() surrounding a bunch of skipped updates.\n These two calls to git_attr_set_direction() cancel each other out\n and thus can be omitted when o->dry_run is true just as they\n already are when !o->update.\n * The code would previously call setup_collided_checkout_detection()\n and report_collided_checkout() even when o->dry_run. However, this\n was just an expensive no-op because\n setup_collided_checkout_detection() merely cleared the CE_MATCHED\n flag for each cache entry, and report_collided_checkout() reported\n which ones had it set. Since a dry-run would skip all the\n checkout_entry() calls, CE_MATCHED would never get set and thus\n no collisions would be reported. Since we can't detect the\n collisions anyway without doing updates, skipping the collisions\n detection setup and reporting is an optimization.\n * The code previously would call get_progress() and\n display_progress() even when (!o->update || o->dry_run). This\n served to show how long it took to skip all the updates, which is\n somewhat useless. Since we are skipping the updates, we can skip\n showing how long it takes to skip them.\n\nSigned-off-by: Elijah Newren \n---\n unpack-trees.c | 26 ++++++++++++++------------\n 1 file changed, 14 insertions(+), 12 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 2399b6818b..4c68dbdb43 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -372,15 +372,20 @@ static int check_updates(struct unpack_trees_options *o)\n \tstate.refresh_cache = 1;\n \tstate.istate = index;\n \n+\tif (!o->update || o->dry_run) {\n+\t\tremove_marked_cache_entries(index, 0);\n+\t\ttrace_performance_leave(\"check_updates\");\n+\t\treturn 0;\n+\t}\n+\n \tif (o->clone)\n \t\tsetup_collided_checkout_detection(&state, index);\n \n \tprogress = get_progress(o);\n \n-\tif (o->update)\n-\t\tgit_attr_set_direction(GIT_ATTR_CHECKOUT);\n+\tgit_attr_set_direction(GIT_ATTR_CHECKOUT);\n \n-\tif (should_update_submodules() && o->update && !o->dry_run)\n+\tif (should_update_submodules())\n \t\tload_gitmodules_file(index, NULL);\n \n \tfor (i = 0; i < index->cache_nr; i++) {\n@@ -388,18 +393,18 @@ static int check_updates(struct unpack_trees_options *o)\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\tunlink_entry(ce);\n \t\t}\n \t}\n+\n \tremove_marked_cache_entries(index, 0);\n \tremove_scheduled_dirs();\n \n-\tif (should_update_submodules() && o->update && !o->dry_run)\n+\tif (should_update_submodules())\n \t\tload_gitmodules_file(index, &state);\n \n \tenable_delayed_checkout(&state);\n-\tif (has_promisor_remote() && o->update && !o->dry_run) {\n+\tif (has_promisor_remote()) {\n \t\t/*\n \t\t * Prefetch the objects that are to be checked out in the loop\n \t\t * below.\n@@ -431,15 +436,12 @@ 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\t\terrs |= checkout_entry(ce, &state, NULL, NULL);\n-\t\t\t}\n+\t\t\terrs |= checkout_entry(ce, &state, NULL, NULL);\n \t\t}\n \t}\n \tstop_progress(&progress);\n \terrs |= finish_delayed_checkout(&state, NULL);\n-\tif (o->update)\n-\t\tgit_attr_set_direction(GIT_ATTR_CHECKIN);\n+\tgit_attr_set_direction(GIT_ATTR_CHECKIN);\n \n \tif (o->clone)\n \t\treport_collided_checkout(index);\n-- \ngitgitgadget\n"},{"id":"389394","messageId":"xmqq8smj6x3s.fsf@gitster-ct.c.googlers.com","threadId":"52559","inReplyTo":"dd277273324eaba5e1f4a368bf2e4d046033c776.1578380277.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/1] unpack-trees: exit check_updates() early if updates are not wanted","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-07T16:37:59Z","receivedAt":"2020-01-07T16:38:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> From: Elijah Newren \n>\n> check_updates() has a lot of code that repeatedly checks whether\n> o->update or o->dry_run are set. (Note that o->dry_run is a\n> near-synonym for !o->update, but not quite as per commit 2c9078d05bf2\n> (\"unpack-trees: add the dry_run flag to unpack_trees_options\",\n> 2011-05-25).) In fact, this function almost turns into a no-op whenever\n> the condition\n> !o->update || o->dry_run\n> is met. Simplify the code by checking this condition at the beginning\n> of the function, and when it is true, do the few things that are\n> relevant and return early.\n\nThanks; will queue.\n\n"}]}