{"thread":{"id":"48948","subject":"[PATCH v1] checkout: optionally speed up \"git checkout -b foo\"","startedAt":"2018-07-24T18:01:48Z","lastAt":"2018-09-19T04:41:19Z","messageCount":32,"participants":["Ben Peart","Eric Sunshine","Junio C Hamano","Duy Nguyen","Elijah Newren","Ævar Arnfjörð Bjarmason","Taylor Blau","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"353500","messageId":"20180724180122.29212-1-benpeart@microsoft.com","threadId":"48948","inReplyTo":null,"subject":"[PATCH v1] checkout: optionally speed up \"git checkout -b foo\"","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2018-07-24T18:01:39Z","receivedAt":"2018-07-24T18:01:48Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"From: Ben Peart \n\nIf the new core.optimizecheckout config setting is set to true, speed up\n\"git checkout -b foo\" by avoiding the work to merge the working tree. This\nis valid because no merge needs to occur - only creating the new branch/\nupdating the refs. Any other options force it through the old code path.\n\nThis change in behavior is off by default and behind the config setting so\nthat users have to opt-in to the optimized behavior.\n\nWe've been running with this patch internally for a long time but it was\nrejected when I submitted it to the mailing list before because it\nimplicitly changes the behavior of checkout -b. Trying it again configured\nbehind a config setting as a potential solution for other optimizations to\ncheckout that could change the behavior as well.\n\nhttps://public-inbox.org/git/20180724042740.GB13248@sigill.intra.peff.net/T/#m75afe3ab318d23f36334cf3a6e3d058839592469\n\nSigned-off-by: Ben Peart \n---\n\nNotes:\n Base Ref: master\n Web-Diff: https://github.com/benpeart/git/commit/f43d934ce7\n Checkout: git fetch https://github.com/benpeart/git checkout-b-v1 && git checkout f43d934ce7\n\n Documentation/config.txt | 6 +++\n builtin/checkout.c | 94 ++++++++++++++++++++++++++++++++++++++++\n cache.h | 1 +\n config.c | 5 +++\n environment.c | 1 +\n 5 files changed, 107 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex a32172a43c..2c4f513bf1 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -911,6 +911,12 @@ core.commitGraph::\n \tEnable git commit graph feature. Allows reading from the\n \tcommit-graph file.\n \n+core.optimizedCheckout\n+\tSpeed up \"git checkout -b foo\" by skipping much of the work of a\n+\tfull checkout command. This changs the behavior as it will skip\n+\tmerging the trees and updating the index and instead only create\n+\tand switch to the new ref.\n+\n core.sparseCheckout::\n \tEnable \"sparse checkout\" feature. See section \"Sparse checkout\" in\n \tlinkgit:git-read-tree[1] for more information.\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 28627650cd..b186a3201e 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -41,6 +41,10 @@ struct checkout_opts {\n \tint ignore_skipworktree;\n \tint ignore_other_worktrees;\n \tint show_progress;\n+\t/*\n+\t * If new checkout options are added, needs_working_tree_merge\n+\t * should be updated accordingly.\n+\t */\n \n \tconst char *new_branch;\n \tconst char *new_branch_force;\n@@ -471,6 +475,88 @@ static void setup_branch_path(struct branch_info *branch)\n \tbranch->path = strbuf_detach(&buf, NULL);\n }\n \n+static int needs_working_tree_merge(const struct checkout_opts *opts,\n+\tconst struct branch_info *old_branch_info,\n+\tconst struct branch_info *new_branch_info)\n+{\n+\t/*\n+\t * We must do the merge if we are actually moving to a new\n+\t * commit tree.\n+\t */\n+\tif (!old_branch_info->commit || !new_branch_info->commit ||\n+\t\toidcmp(&old_branch_info->commit->object.oid, &new_branch_info->commit->object.oid))\n+\t\treturn 1;\n+\n+\t/*\n+\t * opts->patch_mode cannot be used with switching branches so is\n+\t * not tested here\n+\t */\n+\n+\t/*\n+\t * opts->quiet only impacts output so doesn't require a merge\n+\t */\n+\n+\t/*\n+\t * Honor the explicit request for a three-way merge or to throw away\n+\t * local changes\n+\t */\n+\tif (opts->merge || opts->force)\n+\t\treturn 1;\n+\n+\t/*\n+\t * --detach is documented as \"updating the index and the files in the\n+\t * working tree\" but this optimization skips those steps so fall through\n+\t * to the regular code path.\n+\t */\n+\tif (opts->force_detach)\n+\t\treturn 1;\n+\n+\t/*\n+\t * opts->writeout_stage cannot be used with switching branches so is\n+\t * not tested here\n+\t */\n+\n+\t/*\n+\t * Honor the explicit ignore requests\n+\t */\n+\tif (!opts->overwrite_ignore || opts->ignore_skipworktree ||\n+\t\topts->ignore_other_worktrees)\n+\t\treturn 1;\n+\n+\t/*\n+\t * opts->show_progress only impacts output so doesn't require a merge\n+\t */\n+\n+\t/*\n+\t * If we aren't creating a new branch any changes or updates will\n+\t * happen in the existing branch. Since that could only be updating\n+\t * the index and working directory, we don't want to skip those steps\n+\t * or we've defeated any purpose in running the command.\n+\t */\n+\tif (!opts->new_branch)\n+\t\treturn 1;\n+\n+\t/*\n+\t * new_branch_force is defined to \"create/reset and checkout a branch\"\n+\t * so needs to go through the merge to do the reset\n+\t */\n+\tif (opts->new_branch_force)\n+\t\treturn 1;\n+\n+\t/*\n+\t * A new orphaned branch requrires the index and the working tree to be\n+\t * adjusted to \n+\t */\n+\tif (opts->new_orphan_branch)\n+\t\treturn 1;\n+\n+\t/*\n+\t * Remaining variables are not checkout options but used to track state\n+\t */\n+\n+\treturn 0;\n+}\n+\n static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t struct branch_info *old_branch_info,\n \t\t\t struct branch_info *new_branch_info,\n@@ -479,6 +565,14 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \tint ret;\n \tstruct lock_file lock_file = LOCK_INIT;\n \n+\t/*\n+\t * Skip merging the trees, updating the index, and work tree only if we\n+\t * are simply creating a new branch via \"git checkout -b foo.\" Any\n+\t * other options or usage will continue to do all these steps.\n+\t */\n+\tif (core_optimize_checkout && !needs_working_tree_merge(opts, old_branch_info, new_branch_info))\n+\t\treturn 0;\n+\n \thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \tif (read_cache_preload(NULL) < 0)\n \t\treturn error(_(\"index file corrupt\"));\ndiff --git a/cache.h b/cache.h\nindex 8b447652a7..7f2b386c67 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -815,6 +815,7 @@ extern int fsync_object_files;\n extern int core_preload_index;\n extern int core_commit_graph;\n extern int core_apply_sparse_checkout;\n+extern int core_optimize_checkout;\n extern int precomposed_unicode;\n extern int protect_hfs;\n extern int protect_ntfs;\ndiff --git a/config.c b/config.c\nindex 7968ef7566..b59a303e52 100644\n--- a/config.c\n+++ b/config.c\n@@ -1319,6 +1319,11 @@ static int git_default_core_config(const char *var, const char *value)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.optimizecheckout\")) {\n+\t\tcore_optimize_checkout = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"core.precomposeunicode\")) {\n \t\tprecomposed_unicode = git_config_bool(var, value);\n \t\treturn 0;\ndiff --git a/environment.c b/environment.c\nindex 013e845235..1991fb4855 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -68,6 +68,7 @@ char *notes_ref_name;\n int grafts_replace_parents = 1;\n int core_commit_graph;\n int core_apply_sparse_checkout;\n+int core_optimize_checkout;\n int merge_log_config = -1;\n int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */\n unsigned long pack_size_limit_cfg;\n\nbase-commit: b7bd9486b055c3f967a870311e704e3bb0654e4f\n-- \n2.17.0.gvfs.1.123.g449c066\n\n"},{"id":"353508","messageId":"CAPig+cQZ4g-6uT3zB0n2XWb-68DUUBZdaimTb6_Y4DNZrLUdyQ@mail.gmail.com","threadId":"48948","inReplyTo":"20180724180122.29212-1-benpeart@microsoft.com","subject":"Re: [PATCH v1] checkout: optionally speed up \"git checkout -b foo\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-24T18:42:00Z","receivedAt":"2018-07-24T18:42:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 24, 2018 at 2:01 PM Ben Peart wrote:\n> If the new core.optimizecheckout config setting is set to true, speed up\n\nMaybe:\n\n Add core.optimizeCheckout config setting which, when true, speeds up\n\n> \"git checkout -b foo\" by avoiding the work to merge the working tree. This\n> is valid because no merge needs to occur - only creating the new branch/\n> updating the refs. Any other options force it through the old code path.\n>\n> This change in behavior is off by default and behind the config setting so\n> that users have to opt-in to the optimized behavior.\n>\n> We've been running with this patch internally for a long time but it was\n> rejected when I submitted it to the mailing list before because it\n> implicitly changes the behavior of checkout -b. Trying it again configured\n> behind a config setting as a potential solution for other optimizations to\n> checkout that could change the behavior as well.\n\nThis paragraph is mere commentary which probably belongs below the\n\"---\" line following your sign-off.\n\n> https://public-inbox.org/git/20180724042740.GB13248@sigill.intra.peff.net/T/#m75afe3ab318d23f36334cf3a6e3d058839592469\n\nIs this link meant to reference the previous attempt of optimizing\n\"checkout -b\"? Although there's a single mention of \"checkout -b\" in\nthat discussion, it doesn't seem to be the previous attempt or explain\nwhy it was rejected.\n\nIt would be quite nice to see a discussion in both the commit message\nand the documentation about the pros and cons of enabling this\noptimization. That it was previously rejected suggests that there may\nbe serious or unexpected consequences. How will a typical user know\nwhether its use is desirable or not?\n\n> Signed-off-by: Ben Peart \n> ---\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> @@ -911,6 +911,12 @@ core.commitGraph::\n> +core.optimizedCheckout\n> + Speed up \"git checkout -b foo\" by skipping much of the work of a\n> + full checkout command. This changs the behavior as it will skip\n\ns/changs/changes/\n\n> + merging the trees and updating the index and instead only create\n> + and switch to the new ref.\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> @@ -471,6 +475,88 @@ static void setup_branch_path(struct branch_info *branch)\n> +static int needs_working_tree_merge(const struct checkout_opts *opts,\n> + const struct branch_info *old_branch_info,\n> + const struct branch_info *new_branch_info)\n> +{\n> + /*\n> + * We must do the merge if we are actually moving to a new\n> + * commit tree.\n> + */\n> + if (!old_branch_info->commit || !new_branch_info->commit ||\n> + oidcmp(&old_branch_info->commit->object.oid, &new_branch_info->commit->object.oid))\n> + return 1;\n> + [...]\n> + return 0;\n> +}\n\nThis long list of special-case checks doesn't leave me too enthused,\nhowever, that aside, this approach seems backward. Rather than erring\non the side of safety by falling back to the merging behavior, it errs\nin the other direction, which may be a problem if this list of\nspecial-case checks ever gets out of sync with 'checkout_opts'. That\nis, if someone adds a new option which ought to employ the merging\nbehavior, but forgets to update this function, then this function will\nincorrectly default to using the optimization.\n\nA safer approach would be the inverse, namely:\n\n static int skip_worktree_merge(...)\n {\n if (...meets all criteria for optimization...)\n return 1;\n return 0;\n }\n\n> static int merge_working_tree(const struct checkout_opts *opts,\n> struct branch_info *old_branch_info,\n> struct branch_info *new_branch_info,\n> {\n> + /*\n> + * Skip merging the trees, updating the index, and work tree only if we\n> + * are simply creating a new branch via \"git checkout -b foo.\" Any\n> + * other options or usage will continue to do all these steps.\n> + */\n> + if (core_optimize_checkout && !needs_working_tree_merge(opts, old_branch_info, new_branch_info))\n> + return 0;\n\nThis seems a somewhat odd place to hook in this optimization,\nespecially as there is only a single caller of this function. Instead,\none might expect the caller itself to make this judgment and avoid\ntrying the merge in the first place if not needed. That is, in\nswitch_branches:\n\n if (!skip_worktree_merge(...))\n ret = merge_working_tree(...);\n"},{"id":"353514","messageId":"xmqqh8koxwwi.fsf@gitster-ct.c.googlers.com","threadId":"48948","inReplyTo":"20180724180122.29212-1-benpeart@microsoft.com","subject":"Re: [PATCH v1] checkout: optionally speed up \"git checkout -b foo\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-24T19:21:17Z","receivedAt":"2018-07-24T19:21:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart writes:\n\n> From: Ben Peart \n>\n> If the new core.optimizecheckout config setting is set to true, speed up\n> \"git checkout -b foo\" by avoiding the work to merge the working tree. This\n> is valid because no merge needs to occur - only creating the new branch/\n> updating the refs. Any other options force it through the old code path.\n>\n> This change in behavior is off by default and behind the config setting so\n> that users have to opt-in to the optimized behavior.\n\n\n\n\n> We've been running with this patch internally for a long time but it was\n> rejected when I submitted it to the mailing list before because it\n> implicitly changes the behavior of checkout -b. Trying it again configured\n> behind a config setting as a potential solution for other optimizations to\n> checkout that could change the behavior as well.\n>\n> https://public-inbox.org/git/20180724042740.GB13248@sigill.intra.peff.net/T/#m75afe3ab318d23f36334cf3a6e3d058839592469\n\nAn incorrect link? It does not look like a thread that explains\nwhat was previously submitted but failed. The last paragraph looks\nlike a fine material below the three-dash line.\n\n\n> Signed-off-by: Ben Peart \n> ---\n>\n> Notes:\n> Base Ref: master\n> Web-Diff: https://github.com/benpeart/git/commit/f43d934ce7\n> Checkout: git fetch https://github.com/benpeart/git checkout-b-v1 && git checkout f43d934ce7\n>\n> Documentation/config.txt | 6 +++\n> builtin/checkout.c | 94 ++++++++++++++++++++++++++++++++++++++++\n> cache.h | 1 +\n> config.c | 5 +++\n> environment.c | 1 +\n> 5 files changed, 107 insertions(+)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index a32172a43c..2c4f513bf1 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -911,6 +911,12 @@ core.commitGraph::\n> \tEnable git commit graph feature. Allows reading from the\n> \tcommit-graph file.\n> \n> +core.optimizedCheckout\n> +\tSpeed up \"git checkout -b foo\" by skipping much of the work of a\n> +\tfull checkout command. This changs the behavior as it will skip\n> +\tmerging the trees and updating the index and instead only create\n> +\tand switch to the new ref.\n\nBy the way, why is it a core.* thing, not checkout.* thing?\n\nIf a new feature is not necessarily recommendable for normal users\nand it needs to be hidden behind an opt-in knob (I do not have a\nstrong opinion if that is or is not the case for this particular\nfeature at this point), the documentation for the knob should give a\nbit more than \"This chang(e)s the behavior\" to the readers, I would\nthink, to be intellectually honest ;-). Let's tell them what bad\nthings happen if we pretend that we switched the branch without\ntwoway merge and the index update to help them make an informed\ndecision.\n\n> +static int needs_working_tree_merge(const struct checkout_opts *opts,\n> +\tconst struct branch_info *old_branch_info,\n> +\tconst struct branch_info *new_branch_info)\n> +{\n> +\t/*\n> +\t * We must do the merge if we are actually moving to a new\n> +\t * commit tree.\n\nWhat's a \"commit tree\"? Shouldn't it be just a \"commit\"?\n\n> +\t */\n> +\tif (!old_branch_info->commit || !new_branch_info->commit ||\n> +\t\toidcmp(&old_branch_info->commit->object.oid, &new_branch_info->commit->object.oid))\n> +\t\treturn 1;\n> +\n> +\t/*\n> +\t * opts->patch_mode cannot be used with switching branches so is\n> +\t * not tested here\n> +\t */\n> +\n> +\t/*\n> +\t * opts->quiet only impacts output so doesn't require a merge\n> +\t */\n> +\n> +\t/*\n> +\t * Honor the explicit request for a three-way merge or to throw away\n> +\t * local changes\n> +\t */\n> +\tif (opts->merge || opts->force)\n> +\t\treturn 1;\n> +\n> +\t/*\n> +\t * --detach is documented as \"updating the index and the files in the\n> +\t * working tree\" but this optimization skips those steps so fall through\n> +\t * to the regular code path.\n> +\t */\n> +\tif (opts->force_detach)\n> +\t\treturn 1;\n> +\n> +\t/*\n> +\t * opts->writeout_stage cannot be used with switching branches so is\n> +\t * not tested here\n> +\t */\n> +\n> +\t/*\n> +\t * Honor the explicit ignore requests\n> +\t */\n> +\tif (!opts->overwrite_ignore || opts->ignore_skipworktree ||\n> +\t\topts->ignore_other_worktrees)\n> +\t\treturn 1;\n> +\n> +\t/*\n> +\t * opts->show_progress only impacts output so doesn't require a merge\n> +\t */\n> +\n> +\t/*\n> +\t * If we aren't creating a new branch any changes or updates will\n> +\t * happen in the existing branch. Since that could only be updating\n> +\t * the index and working directory, we don't want to skip those steps\n> +\t * or we've defeated any purpose in running the command.\n> +\t */\n> +\tif (!opts->new_branch)\n> +\t\treturn 1;\n> +\n> +\t/*\n> +\t * new_branch_force is defined to \"create/reset and checkout a branch\"\n> +\t * so needs to go through the merge to do the reset\n> +\t */\n> +\tif (opts->new_branch_force)\n> +\t\treturn 1;\n> +\n> +\t/*\n> +\t * A new orphaned branch requrires the index and the working tree to be\n> +\t * adjusted to \n> +\t */\n> +\tif (opts->new_orphan_branch)\n> +\t\treturn 1;\n> +\n> +\t/*\n> +\t * Remaining variables are not checkout options but used to track state\n> +\t */\n> +\n> +\treturn 0;\n> +}\n\nThis helper function alone looks like we are creating a maintenance\nnightmare from a quick scan. How are we going to keep this up to\ndate?\n\nI offhand do not know how \"git checkout -b foo\" would behave\ndifferently if we do not do a two-way merge between HEAD and HEAD to\nupdate the index. We'd still need to list the local modifications\nand say \"Switched to a new branch 'foo'\", but that would be a minor\nthing compared to the two-way merge machinery.\n\nWas the primary reason why the patch \"changes the behaviour\" because\nnobody could prove that needs_working_tree_merge() helper reliably\ndetects that \"checkout -b foo\" case and that case alone, and show a\nway to make sure it will keep doing so in the future when other new\nfeatures are added to the command?\n\n> @@ -479,6 +565,14 @@ static int merge_working_tree(const struct checkout_opts *opts,\n> \tint ret;\n> \tstruct lock_file lock_file = LOCK_INIT;\n> \n> +\t/*\n> +\t * Skip merging the trees, updating the index, and work tree only if we\n> +\t * are simply creating a new branch via \"git checkout -b foo.\" Any\n> +\t * other options or usage will continue to do all these steps.\n> +\t */\n> +\tif (core_optimize_checkout && !needs_working_tree_merge(opts, old_branch_info, new_branch_info))\n> +\t\treturn 0;\n> +\n> \thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n> \tif (read_cache_preload(NULL) < 0)\n> \t\treturn error(_(\"index file corrupt\"));\n\nThanks.\n"},{"id":"353519","messageId":"960c32c0-5b10-6854-73c9-392814dddbb3@gmail.com","threadId":"48948","inReplyTo":"CAPig+cQZ4g-6uT3zB0n2XWb-68DUUBZdaimTb6_Y4DNZrLUdyQ@mail.gmail.com","subject":"Re: [PATCH v1] checkout: optionally speed up \"git checkout -b foo\"","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-07-24T19:45:19Z","receivedAt":"2018-07-24T19:45:24Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 7/24/2018 2:42 PM, Eric Sunshine wrote:\n> On Tue, Jul 24, 2018 at 2:01 PM Ben Peart wrote:\n>> If the new core.optimizecheckout config setting is set to true, speed up\n> \n> Maybe:\n> \n> Add core.optimizeCheckout config setting which, when true, speeds up\n> \n\nSure\n\n>> \"git checkout -b foo\" by avoiding the work to merge the working tree. This\n>> is valid because no merge needs to occur - only creating the new branch/\n>> updating the refs. Any other options force it through the old code path.\n>>\n>> This change in behavior is off by default and behind the config setting so\n>> that users have to opt-in to the optimized behavior.\n>>\n>> We've been running with this patch internally for a long time but it was\n>> rejected when I submitted it to the mailing list before because it\n>> implicitly changes the behavior of checkout -b. Trying it again configured\n>> behind a config setting as a potential solution for other optimizations to\n>> checkout that could change the behavior as well.\n> \n> This paragraph is mere commentary which probably belongs below the\n> \"---\" line following your sign-off.\n> \n\nHopefully this commentary (I'll move it below the --- line) is clearer:\n\nWe've been running with this patch internally for a long time but it was\nrejected when I submitted it to the mailing list before [1] because it\nimplicitly changes the behavior of checkout -b as it no longer updates\nthe working directory.\n\nI'm submitting it again behind a config setting so that it doesn't cause\nany back compat issues unless the user explicitly opts in to the new\nbehavior. My hope is this same setting and model can be used if/when we\nmake other performance optimizations to checkout like using the cache\ntree to avoid having to traverse the entire tree being discussed [2].\n\n[1] \nhttps://public-inbox.org/git/20160909192520.4812-1-benpeart@microsoft.com/\n[2] \nhttps://public-inbox.org/git/20180724042740.GB13248@sigill.intra.peff.net/T/#m75afe3ab318d23f36334cf3a6e3d058839592469\n\n>> https://public-inbox.org/git/20180724042740.GB13248@sigill.intra.peff.net/T/#m75afe3ab318d23f36334cf3a6e3d058839592469\n> \n> Is this link meant to reference the previous attempt of optimizing\n> \"checkout -b\"? Although there's a single mention of \"checkout -b\" in\n> that discussion, it doesn't seem to be the previous attempt or explain\n> why it was rejected.\n> \n> It would be quite nice to see a discussion in both the commit message\n> and the documentation about the pros and cons of enabling this\n> optimization. That it was previously rejected suggests that there may\n> be serious or unexpected consequences. How will a typical user know\n> whether its use is desirable or not?\n> \n>> Signed-off-by: Ben Peart \n>> ---\n>> diff --git a/Documentation/config.txt b/Documentation/config.txt\n>> @@ -911,6 +911,12 @@ core.commitGraph::\n>> +core.optimizedCheckout\n>> + Speed up \"git checkout -b foo\" by skipping much of the work of a\n>> + full checkout command. This changs the behavior as it will skip\n> \n> s/changs/changes/\n> \n>> + merging the trees and updating the index and instead only create\n>> + and switch to the new ref.\n>> diff --git a/builtin/checkout.c b/builtin/checkout.c\n>> @@ -471,6 +475,88 @@ static void setup_branch_path(struct branch_info *branch)\n>> +static int needs_working_tree_merge(const struct checkout_opts *opts,\n>> + const struct branch_info *old_branch_info,\n>> + const struct branch_info *new_branch_info)\n>> +{\n>> + /*\n>> + * We must do the merge if we are actually moving to a new\n>> + * commit tree.\n>> + */\n>> + if (!old_branch_info->commit || !new_branch_info->commit ||\n>> + oidcmp(&old_branch_info->commit->object.oid, &new_branch_info->commit->object.oid))\n>> + return 1;\n>> + [...]\n>> + return 0;\n>> +}\n> \n> This long list of special-case checks doesn't leave me too enthused,\n> however, that aside, this approach seems backward. Rather than erring\n> on the side of safety by falling back to the merging behavior, it errs\n> in the other direction, which may be a problem if this list of\n> special-case checks ever gets out of sync with 'checkout_opts'. That\n> is, if someone adds a new option which ought to employ the merging\n> behavior, but forgets to update this function, then this function will\n> incorrectly default to using the optimization.\n> \n\nI'm not thrilled with the long list either (the plethora of comments \nprobably makes it appear worse than it is) but I don't see how flipping \nthe logic around makes it fail if someone adds a new option. The \"meets \nall criteria for optimization\" code can only test existing options.\n\n> A safer approach would be the inverse, namely:\n> \n> static int skip_worktree_merge(...)\n> {\n> if (...meets all criteria for optimization...)\n> return 1;\n> return 0;\n> }\n> \n>> static int merge_working_tree(const struct checkout_opts *opts,\n>> struct branch_info *old_branch_info,\n>> struct branch_info *new_branch_info,\n>> {\n>> + /*\n>> + * Skip merging the trees, updating the index, and work tree only if we\n>> + * are simply creating a new branch via \"git checkout -b foo.\" Any\n>> + * other options or usage will continue to do all these steps.\n>> + */\n>> + if (core_optimize_checkout && !needs_working_tree_merge(opts, old_branch_info, new_branch_info))\n>> + return 0;\n> \n> This seems a somewhat odd place to hook in this optimization,\n> especially as there is only a single caller of this function. Instead,\n> one might expect the caller itself to make this judgment and avoid\n> trying the merge in the first place if not needed. That is, in\n> switch_branches:\n> \n> if (!skip_worktree_merge(...))\n> ret = merge_working_tree(...);\n> \n\nI personally agree, it was moved to its current location per feedback \nthe first time around. Perhaps with the addition of the config setting \nit will be better received moved out to the caller.\n"},{"id":"353526","messageId":"ab8ee481-54fa-a014-69d9-8f621b136766@gmail.com","threadId":"48948","inReplyTo":"xmqqh8koxwwi.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1] checkout: optionally speed up \"git checkout -b foo\"","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-07-24T20:47:37Z","receivedAt":"2018-07-24T20:47:41Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 7/24/2018 3:21 PM, Junio C Hamano wrote:\n> Ben Peart writes:\n> \n>> From: Ben Peart \n>>\n>> If the new core.optimizecheckout config setting is set to true, speed up\n>> \"git checkout -b foo\" by avoiding the work to merge the working tree. This\n>> is valid because no merge needs to occur - only creating the new branch/\n>> updating the refs. Any other options force it through the old code path.\n>>\n>> This change in behavior is off by default and behind the config setting so\n>> that users have to opt-in to the optimized behavior.\n> \n> \n> \n> \n>> We've been running with this patch internally for a long time but it was\n>> rejected when I submitted it to the mailing list before because it\n>> implicitly changes the behavior of checkout -b. Trying it again configured\n>> behind a config setting as a potential solution for other optimizations to\n>> checkout that could change the behavior as well.\n>>\n>> https://public-inbox.org/git/20180724042740.GB13248@sigill.intra.peff.net/T/#m75afe3ab318d23f36334cf3a6e3d058839592469\n> \n> An incorrect link? It does not look like a thread that explains\n> what was previously submitted but failed. The last paragraph looks\n> like a fine material below the three-dash line.\n> \n\nSee my earlier reply about this section in:\n\nhttps://public-inbox.org/git/xmqqh8koxwwi.fsf@gitster-ct.c.googlers.com/T/#mb31136a09dbc1a963a5a62e840b118ac33043edf\n\n> \n>> Signed-off-by: Ben Peart \n>> ---\n>>\n>> Notes:\n>> Base Ref: master\n>> Web-Diff: https://github.com/benpeart/git/commit/f43d934ce7\n>> Checkout: git fetch https://github.com/benpeart/git checkout-b-v1 && git checkout f43d934ce7\n>>\n>> Documentation/config.txt | 6 +++\n>> builtin/checkout.c | 94 ++++++++++++++++++++++++++++++++++++++++\n>> cache.h | 1 +\n>> config.c | 5 +++\n>> environment.c | 1 +\n>> 5 files changed, 107 insertions(+)\n>>\n>> diff --git a/Documentation/config.txt b/Documentation/config.txt\n>> index a32172a43c..2c4f513bf1 100644\n>> --- a/Documentation/config.txt\n>> +++ b/Documentation/config.txt\n>> @@ -911,6 +911,12 @@ core.commitGraph::\n>> \tEnable git commit graph feature. Allows reading from the\n>> \tcommit-graph file.\n>> \n>> +core.optimizedCheckout\n>> +\tSpeed up \"git checkout -b foo\" by skipping much of the work of a\n>> +\tfull checkout command. This changs the behavior as it will skip\n>> +\tmerging the trees and updating the index and instead only create\n>> +\tand switch to the new ref.\n> \n> By the way, why is it a core.* thing, not checkout.* thing?\n> \n\nI followed the naming convention used by core.sparsecheckout but I'm \nhappy to call it whatever people want.\n\n> If a new feature is not necessarily recommendable for normal users\n> and it needs to be hidden behind an opt-in knob (I do not have a\n> strong opinion if that is or is not the case for this particular\n> feature at this point), the documentation for the knob should give a\n> bit more than \"This chang(e)s the behavior\" to the readers, I would\n> think, to be intellectually honest ;-). Let's tell them what bad\n> things happen if we pretend that we switched the branch without\n> twoway merge and the index update to help them make an informed\n> decision.\n> \n\nI attempted to explain what the change in behavior was in the same \nsentence by saying what it skips and what it still does. If that isn't \nintellectually honest, help me know how to better explain it so that it is.\n\nIs this better?\n\nSpeed up \"git checkout -b \" by skipping the twoway merge and \nindex update. Instead, just create a new branch named and \nswitch to it. The working directory and index are left unchanged.\n\n>> +static int needs_working_tree_merge(const struct checkout_opts *opts,\n>> +\tconst struct branch_info *old_branch_info,\n>> +\tconst struct branch_info *new_branch_info)\n>> +{\n>> +\t/*\n>> +\t * We must do the merge if we are actually moving to a new\n>> +\t * commit tree.\n> \n> What's a \"commit tree\"? Shouldn't it be just a \"commit\"?\n> \n\nSure\n\n>> +\t */\n>> +\tif (!old_branch_info->commit || !new_branch_info->commit ||\n>> +\t\toidcmp(&old_branch_info->commit->object.oid, &new_branch_info->commit->object.oid))\n>> +\t\treturn 1;\n>> +\n>> +\t/*\n>> +\t * opts->patch_mode cannot be used with switching branches so is\n>> +\t * not tested here\n>> +\t */\n>> +\n>> +\t/*\n>> +\t * opts->quiet only impacts output so doesn't require a merge\n>> +\t */\n>> +\n>> +\t/*\n>> +\t * Honor the explicit request for a three-way merge or to throw away\n>> +\t * local changes\n>> +\t */\n>> +\tif (opts->merge || opts->force)\n>> +\t\treturn 1;\n>> +\n>> +\t/*\n>> +\t * --detach is documented as \"updating the index and the files in the\n>> +\t * working tree\" but this optimization skips those steps so fall through\n>> +\t * to the regular code path.\n>> +\t */\n>> +\tif (opts->force_detach)\n>> +\t\treturn 1;\n>> +\n>> +\t/*\n>> +\t * opts->writeout_stage cannot be used with switching branches so is\n>> +\t * not tested here\n>> +\t */\n>> +\n>> +\t/*\n>> +\t * Honor the explicit ignore requests\n>> +\t */\n>> +\tif (!opts->overwrite_ignore || opts->ignore_skipworktree ||\n>> +\t\topts->ignore_other_worktrees)\n>> +\t\treturn 1;\n>> +\n>> +\t/*\n>> +\t * opts->show_progress only impacts output so doesn't require a merge\n>> +\t */\n>> +\n>> +\t/*\n>> +\t * If we aren't creating a new branch any changes or updates will\n>> +\t * happen in the existing branch. Since that could only be updating\n>> +\t * the index and working directory, we don't want to skip those steps\n>> +\t * or we've defeated any purpose in running the command.\n>> +\t */\n>> +\tif (!opts->new_branch)\n>> +\t\treturn 1;\n>> +\n>> +\t/*\n>> +\t * new_branch_force is defined to \"create/reset and checkout a branch\"\n>> +\t * so needs to go through the merge to do the reset\n>> +\t */\n>> +\tif (opts->new_branch_force)\n>> +\t\treturn 1;\n>> +\n>> +\t/*\n>> +\t * A new orphaned branch requrires the index and the working tree to be\n>> +\t * adjusted to \n>> +\t */\n>> +\tif (opts->new_orphan_branch)\n>> +\t\treturn 1;\n>> +\n>> +\t/*\n>> +\t * Remaining variables are not checkout options but used to track state\n>> +\t */\n>> +\n>> +\treturn 0;\n>> +}\n> \n> This helper function alone looks like we are creating a maintenance\n> nightmare from a quick scan. How are we going to keep this up to\n> date?\n> \n> I offhand do not know how \"git checkout -b foo\" would behave\n> differently if we do not do a two-way merge between HEAD and HEAD to\n> update the index. We'd still need to list the local modifications\n> and say \"Switched to a new branch 'foo'\", but that would be a minor\n> thing compared to the two-way merge machinery.\n> \n\nWith the patch, the index and working directory aren't modified, it does \nstill list the local modifications and says \"Switched to...\"\n\nThe changes in behavior come from other inputs. For example, if you \nturned on sparse checkout and then did a \"git checkout -b foo\" the old \ncode path would update the skip-worktree bit in the index and remove all \nthe files from the working directory that were no longer specified in \nthe sparse-checkout file. The new behavior would not do that as it no \nlonger updates the index or working directory.\n\nThe checkout documentation doesn't _say_ that \"git checkout -b foo\" will \nalso update the working directory after changing the sparse-checkout \nsettings but it happens to do that. Is that a common scenario? No - but \nit is an undocumented behavior that will change if they opt in to this \nnew behavior. At least the documentation for the new flag _does_ let \nthe user know the working directory and index won't be changed so if \nthey opt in, they shouldn't be too surprised.\n\n> Was the primary reason why the patch \"changes the behaviour\" because\n> nobody could prove that needs_working_tree_merge() helper reliably\n> detects that \"checkout -b foo\" case and that case alone, and show a\n> way to make sure it will keep doing so in the future when other new\n> features are added to the command?\n> \n\nMy concern isn't ensuring that the patch reliably detects \"checkout -b\", \nthe challenge is ensuring it will keep doing so as features are added \nthat impact the \"-b\" option. The problem of adding a new option and \nhaving to ensure it behaves properly with all the other options isn't \nnew but I agree this does add one more case that has to be handled.\n\n>> @@ -479,6 +565,14 @@ static int merge_working_tree(const struct checkout_opts *opts,\n>> \tint ret;\n>> \tstruct lock_file lock_file = LOCK_INIT;\n>> \n>> +\t/*\n>> +\t * Skip merging the trees, updating the index, and work tree only if we\n>> +\t * are simply creating a new branch via \"git checkout -b foo.\" Any\n>> +\t * other options or usage will continue to do all these steps.\n>> +\t */\n>> +\tif (core_optimize_checkout && !needs_working_tree_merge(opts, old_branch_info, new_branch_info))\n>> +\t\treturn 0;\n>> +\n>> \thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n>> \tif (read_cache_preload(NULL) < 0)\n>> \t\treturn error(_(\"index file corrupt\"));\n> \n> Thanks.\n> \n"},{"id":"353646","messageId":"xmqqeffqrqb3.fsf@gitster-ct.c.googlers.com","threadId":"48948","inReplyTo":"960c32c0-5b10-6854-73c9-392814dddbb3@gmail.com","subject":"Re: [PATCH v1] checkout: optionally speed up \"git checkout -b foo\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-26T15:04:48Z","receivedAt":"2018-07-26T15:04:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart writes:\n\n[jc: it was a bit surprising that Eric covered all the bits I\ncovered while we were writing without peeking each other's ;-)]\n\n>> This long list of special-case checks doesn't leave me too enthused,\n>> however, that aside, this approach seems backward. Rather than erring\n>> on the side of safety by falling back to the merging behavior, it errs\n>> in the other direction, ...\n>\n> I'm not thrilled with the long list either (the plethora of comments\n> probably makes it appear worse than it is) but I don't see how...\n\nIf there were a simple and futureproof way to tell the option\nparsing loop to notice any feature other than \"-b newbranch\" was\nused, then such a whitelisting may be a viable way, but there is\nnot, and the whitelist condition can become (over time---we are\ntalking about futureproofing and not \"a filter that happens to match\ntoday's feature set\") just as complex as this blacklisting function\nis (e.g. feature A and feature B when used alone may be compatible\nwith the optimization but not when used both), so if we were to use\nthis optimization, I think this long list of special-case checks is\nthe best we could do.\n\n>> if (!skip_worktree_merge(...))\n>> ret = merge_working_tree(...);\n>>\n>\n> I personally agree, it was moved to its current location per feedback\n> the first time around. Perhaps with the addition of the config\n> setting it will be better received moved out to the caller.\n\nSounds sensible. I am still not enthused by the configuration\nvariable that is underdocumented. You already said that we know\nthat this optimization does a wrong thing when sparse checkout is\nused---any other cases? I do not think of any myself, and if that\nis true, I am wondering if it makes more sense to \"do we have sparse\nconfiguration?\" as part of the blacklist condition. That way, the\nusers do not have to set anything and they will get an optimization\nbenefit from an obvious change to skip \"read-tree -m HEAD HEAD\" that\nought to be a no-op.\n\nThanks.\n"},{"id":"353669","messageId":"CAPig+cRtUeSCUnUFw0xgXEV7nyBiORZSLfLYfhKWjXxcgHvQmg@mail.gmail.com","threadId":"48948","inReplyTo":"xmqqeffqrqb3.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1] checkout: optionally speed up \"git checkout -b foo\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-26T18:59:02Z","receivedAt":"2018-07-26T18:59:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jul 26, 2018 at 11:04 AM Junio C Hamano wrote:\n> Ben Peart writes:\n> > I'm not thrilled with the long list either (the plethora of comments\n> > probably makes it appear worse than it is) but I don't see how...\n>\n> If there were a simple and futureproof way to tell the option\n> parsing loop to notice any feature other than \"-b newbranch\" was\n> used, then such a whitelisting may be a viable way, but there is\n> not, and the whitelist condition can become (over time---we are\n> talking about futureproofing and not \"a filter that happens to match\n> today's feature set\") just as complex as this blacklisting function\n> is (e.g. feature A and feature B when used alone may be compatible\n> with the optimization but not when used both), so if we were to use\n> this optimization, I think this long list of special-case checks is\n> the best we could do.\n\nI'm wondering if a two-stage parse-options invocations could make this\npotential maintenance problem more manageable by altogether\neliminating needs_working_tree_merge(). Something very roughly along\nthe lines of:\n\n new_branch_and_passive_options = {\n OPT_STRING('b', NULL, ...),\n ...options which can't impact \"optimization\" decision...\n };\n argc = parse_options(argc, argv, prefix,\n new_branch_and_passive_options, NULL,\n PARSE_OPT_KEEP_UNKNOWN | PARSE_OPT_KEEP_DASHDASH);\n\n can_optimize_new_branch = 1;\n for (i = 0; i < argc; i++)\n if (argv[i][0] == '-') {\n can_optimize_new_branch = 0;\n break;\n }\n\n options = {\n ...all other options...\n }\n argc = parse_options(argc, argv, prefix, options,\n checkout_usage, PARSE_OPT_KEEP_DASHDASH);\n\n ...as before...\n\nThe can_optimize_new_branch check could, of course, be fooled by a\nnon-option which starts with a \"-\", but that would err toward safety\nof not optimizing, so shouldn't be a worry.\n"},{"id":"353671","messageId":"CAPig+cRgJh2+1wdQS3a8Vd+D0VDPwLO8HXGYZ6MhkCK3ww47sA@mail.gmail.com","threadId":"48948","inReplyTo":"CAPig+cRtUeSCUnUFw0xgXEV7nyBiORZSLfLYfhKWjXxcgHvQmg@mail.gmail.com","subject":"Re: [PATCH v1] checkout: optionally speed up \"git checkout -b foo\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-26T19:08:32Z","receivedAt":"2018-07-26T19:08:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jul 26, 2018 at 2:59 PM Eric Sunshine wrote:\n> On Thu, Jul 26, 2018 at 11:04 AM Junio C Hamano wrote:\n> > If there were a simple and futureproof way to tell the option\n> > parsing loop to notice any feature other than \"-b newbranch\" was\n> > used, then such a whitelisting may be a viable way [...]\n>\n> I'm wondering if a two-stage parse-options invocations could make this\n> potential maintenance problem more manageable by altogether\n> eliminating needs_working_tree_merge().\n\nA downside of this approach is that it too becomes a nightmare if\ngit-checkout grows additional special cases like the proposed \"-b\", in\nwhich case multi-stage or disjoint or intersecting parse-options\ninvocations might arise. Another downside is that this parse-options\nidea is somewhat invasive, whereas, needs_working_tree_merge(),\ndespite its ugliness, is at least is self-contained and not at all\ninvasive.\n"},{"id":"354060","messageId":"20180731163909.19004-1-benpeart@microsoft.com","threadId":"48948","inReplyTo":"20180724180122.29212-1-benpeart@microsoft.com","subject":"[PATCH v2] checkout: optimize \"git checkout -b \"","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2018-07-31T16:39:25Z","receivedAt":"2018-07-31T16:59:55Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"From: Ben Peart \n\nSkip merging the commit, updating the index and working directory if and\nonly if we are creating a new branch via \"git checkout -b .\"\nAny other checkout options will still go through the former code path. \n\nIf sparse_checkout is on, require the user to manually opt in to this\noptimzed behavior by setting the config setting checkout.optimizeNewBranch\nto true as we will no longer update the skip-worktree bit in the index, nor\nadd/remove files in the working directory to reflect the current sparse\ncheckout settings.\n\nSigned-off-by: Ben Peart \n---\n\nThe biggest change in this version was suggested in feedback to the last\npatch. I have turned on the optimzation by default if sparse-checkout is\nnot on so that most users do not have to set anything and they will get the\nbenefit of the optimization.\n\nBecause users that use sparse checkout are probably doing so because they\nhave a large working directory, they stand to benefit the most from this\noptimization. To enable them to benefit, I added a \"checkout.optimizeNewBranch\"\nconfig setting that allows them to opt-in to this optimization if they are\nwilling to accept the change in behavior. I updated the documentation which\nshould make it clear exactly what the change in behavior is.\n\nNotes:\n Base Ref: master\n Web-Diff: https://github.com/benpeart/git/commit/5ea167fe90\n Checkout: git fetch https://github.com/benpeart/git checkout-b-v2 && git checkout 5ea167fe90\n \n ### Patches\n\n Documentation/config.txt | 5 ++\n builtin/checkout.c | 113 +++++++++++++++++++++++++++++--\n t/t1090-sparse-checkout-scope.sh | 14 ++++\n 3 files changed, 128 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 43b2de7b5f..acf81143d4 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1101,6 +1101,11 @@ browser..path::\n \tbrowse HTML help (see `-w` option in linkgit:git-help[1]) or a\n \tworking repository in gitweb (see linkgit:git-instaweb[1]).\n \n+checkout.optimizeNewBranch\n+\tWhen set to true, \"git checkout -b \" will not update the\n+\tskip-worktree bit in the index nor add/remove files in the working\n+\tdirectory to reflect the current sparse checkout settings.\n+\n clean.requireForce::\n \tA boolean to make git-clean do nothing unless given -f,\n \t-i or -n. Defaults to true.\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 28627650cd..991b71a341 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -24,6 +24,8 @@\n #include \"submodule-config.h\"\n #include \"submodule.h\"\n \n+static int checkout_optimize_new_branch;\n+\n static const char * const checkout_usage[] = {\n \tN_(\"git checkout [] \"),\n \tN_(\"git checkout [] [] -- ...\"),\n@@ -41,6 +43,10 @@ struct checkout_opts {\n \tint ignore_skipworktree;\n \tint ignore_other_worktrees;\n \tint show_progress;\n+\t/*\n+\t * If new checkout options are added, skip_merge_working_tree\n+\t * should be updated accordingly.\n+\t */\n \n \tconst char *new_branch;\n \tconst char *new_branch_force;\n@@ -471,6 +477,98 @@ static void setup_branch_path(struct branch_info *branch)\n \tbranch->path = strbuf_detach(&buf, NULL);\n }\n \n+/*\n+ * Skip merging the trees, updating the index and working directory if and\n+ * only if we are creating a new branch via \"git checkout -b .\"\n+ */\n+static int skip_merge_working_tree(const struct checkout_opts *opts,\n+\tconst struct branch_info *old_branch_info,\n+\tconst struct branch_info *new_branch_info)\n+{\n+\t/*\n+\t * Do the merge if sparse checkout is on and the user has not opted in\n+\t * to the optimized behavior\n+\t */\n+\tif (core_apply_sparse_checkout && !checkout_optimize_new_branch)\n+\t\treturn 0;\n+\n+\t/*\n+\t * We must do the merge if we are actually moving to a new commit.\n+\t */\n+\tif (!old_branch_info->commit || !new_branch_info->commit ||\n+\t\toidcmp(&old_branch_info->commit->object.oid, &new_branch_info->commit->object.oid))\n+\t\treturn 0;\n+\n+\t/*\n+\t * opts->patch_mode cannot be used with switching branches so is\n+\t * not tested here\n+\t */\n+\n+\t/*\n+\t * opts->quiet only impacts output so doesn't require a merge\n+\t */\n+\n+\t/*\n+\t * Honor the explicit request for a three-way merge or to throw away\n+\t * local changes\n+\t */\n+\tif (opts->merge || opts->force)\n+\t\treturn 0;\n+\n+\t/*\n+\t * --detach is documented as \"updating the index and the files in the\n+\t * working tree\" but this optimization skips those steps so fall through\n+\t * to the regular code path.\n+\t */\n+\tif (opts->force_detach)\n+\t\treturn 0;\n+\n+\t/*\n+\t * opts->writeout_stage cannot be used with switching branches so is\n+\t * not tested here\n+\t */\n+\n+\t/*\n+\t * Honor the explicit ignore requests\n+\t */\n+\tif (!opts->overwrite_ignore || opts->ignore_skipworktree ||\n+\t\topts->ignore_other_worktrees)\n+\t\treturn 0;\n+\n+\t/*\n+\t * opts->show_progress only impacts output so doesn't require a merge\n+\t */\n+\n+\t/*\n+\t * If we aren't creating a new branch any changes or updates will\n+\t * happen in the existing branch. Since that could only be updating\n+\t * the index and working directory, we don't want to skip those steps\n+\t * or we've defeated any purpose in running the command.\n+\t */\n+\tif (!opts->new_branch)\n+\t\treturn 0;\n+\n+\t/*\n+\t * new_branch_force is defined to \"create/reset and checkout a branch\"\n+\t * so needs to go through the merge to do the reset\n+\t */\n+\tif (opts->new_branch_force)\n+\t\treturn 0;\n+\n+\t/*\n+\t * A new orphaned branch requrires the index and the working tree to be\n+\t * adjusted to \n+\t */\n+\tif (opts->new_orphan_branch)\n+\t\treturn 0;\n+\n+\t/*\n+\t * Remaining variables are not checkout options but used to track state\n+\t */\n+\n+\treturn 1;\n+}\n+\n static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t struct branch_info *old_branch_info,\n \t\t\t struct branch_info *new_branch_info,\n@@ -845,10 +943,12 @@ static int switch_branches(const struct checkout_opts *opts,\n \t\tparse_commit_or_die(new_branch_info->commit);\n \t}\n \n-\tret = merge_working_tree(opts, &old_branch_info, new_branch_info, &writeout_error);\n-\tif (ret) {\n-\t\tfree(path_to_free);\n-\t\treturn ret;\n+\tif (!skip_merge_working_tree(opts, &old_branch_info, new_branch_info)) {\n+\t\tret = merge_working_tree(opts, &old_branch_info, new_branch_info, &writeout_error);\n+\t\tif (ret) {\n+\t\t\tfree(path_to_free);\n+\t\t\treturn ret;\n+\t\t}\n \t}\n \n \tif (!opts->quiet && !old_branch_info.path && old_branch_info.commit && new_branch_info->commit != old_branch_info.commit)\n@@ -863,6 +963,11 @@ static int switch_branches(const struct checkout_opts *opts,\n \n static int git_checkout_config(const char *var, const char *value, void *cb)\n {\n+\tif (!strcmp(var, \"checkout.optimizenewbranch\")) {\n+\t\tcheckout_optimize_new_branch = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"diff.ignoresubmodules\")) {\n \t\tstruct checkout_opts *opts = cb;\n \t\thandle_ignore_submodules_arg(&opts->diff_options, value);\ndiff --git a/t/t1090-sparse-checkout-scope.sh b/t/t1090-sparse-checkout-scope.sh\nindex 1f61eb3e88..25d7c700f6 100755\n--- a/t/t1090-sparse-checkout-scope.sh\n+++ b/t/t1090-sparse-checkout-scope.sh\n@@ -31,6 +31,20 @@ test_expect_success 'perform sparse checkout of master' '\n \ttest_path_is_file c\n '\n \n+test_expect_success 'checkout -b checkout.optimizeNewBranch interaction' '\n+\tcp .git/info/sparse-checkout .git/info/sparse-checkout.bak &&\n+\ttest_when_finished \"\n+\t\tmv -f .git/info/sparse-checkout.bak .git/info/sparse-checkout\n+\t\tgit checkout master\n+\t\" &&\n+\techo \"/b\" >>.git/info/sparse-checkout &&\n+\ttest \"$(git ls-files -t b)\" = \"S b\" &&\n+\tgit -c checkout.optimizeNewBranch=true checkout -b fast &&\n+\ttest \"$(git ls-files -t b)\" = \"S b\" &&\n+\tgit checkout -b slow &&\n+\ttest \"$(git ls-files -t b)\" = \"H b\"\n+'\n+\n test_expect_success 'merge feature branch into sparse checkout of master' '\n \tgit merge feature &&\n \ttest_path_is_file a &&\n\nbase-commit: ffc6fa0e396238de3a30623912980263b4f283ab\n-- \n2.17.0.gvfs.1.123.g449c066\n\n"},{"id":"354114","messageId":"xmqq36vzb2eg.fsf@gitster-ct.c.googlers.com","threadId":"48948","inReplyTo":"20180731163909.19004-1-benpeart@microsoft.com","subject":"Re: [PATCH v2] checkout: optimize \"git checkout -b \"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-31T20:01:27Z","receivedAt":"2018-07-31T20:01:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart writes:\n\n> The biggest change in this version was suggested in feedback to the last\n> patch. I have turned on the optimzation by default if sparse-checkout is\n> not on so that most users do not have to set anything and they will get the\n> benefit of the optimization.\n\nSounds like a good thing to do.\n\nIf we missed something in the logic of skip_merge_working_tree(),\nthe breakage may affect more unsuspecting people, which may or may\nnot be a bad thing---at least it would allow us to notice it sooner.\n\n> +\tWhen set to true, \"git checkout -b \" will not update the\n> +\tskip-worktree bit in the index nor add/remove files in the working\n> +\tdirectory to reflect the current sparse checkout settings.\n\nThis reads a lot clearer, at least to me, than the documentation\nupdate in the previous round, by speaking in terms of what the\nuser-visible side effect of this setting would be.\n\nNicely done. Thanks.\n"},{"id":"354156","messageId":"CACsJy8DMEMsDnKZc65K-0EJcm2udXZ7OKY=xoFmX4COM0dSH=g@mail.gmail.com","threadId":"48948","inReplyTo":"20180731163909.19004-1-benpeart@microsoft.com","subject":"Re: [PATCH v2] checkout: optimize \"git checkout -b \"","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-01T15:10:25Z","receivedAt":"2018-08-01T15:10:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jul 31, 2018 at 7:03 PM Ben Peart wrote:\n>\n> From: Ben Peart \n>\n> Skip merging the commit, updating the index and working directory if and\n> only if we are creating a new branch via \"git checkout -b .\"\n> Any other checkout options will still go through the former code path.\n\nI'd like to see this giant list of checks broken down and pushed down\nto smaller areas so that chances of new things being added but checks\nnot updated become much smaller. And ideally there should just be no\nbehavior change (I think with your change, \"checkout -b\" will not\nreport local changes, but it's not mentioned in the config document;\nmore things like that can easily slip).\n\nSo. I assume this reason for this patch is because on super large worktrees\n\n - 2-way merge is too slow\n - applying spare checkout patterns on a huge worktree is also slow\n - writing index is, again, slow\n - show_local_changes() slow\n\nFor 2-way merge, I believe we can detect inside unpack_trees() that\nit's a 2-way merge (fn == twoway_merge), from HEAD to HEAD (simple\nenough check), then from the 2-way merge table we know for sure\nnothing is going to change and we can just skip traverse_trees() call\nin unpack_trees().\n\nOn the sparse checkout application. This only needs to be done when\nthere are new files added, or the spare-checkout file has been updated\nsince the last time it's been used. We can keep track of these things\n(sparse-checkout file change could be kept track with just stat info\nmaybe as an index extension) then we can skip applying sparse checkout\nnot for this particular case for but general checkouts as well. Spare\ncheckout file rarely changes. Big win overall.\n\nAnd if all go according to plan, there will be no changes made in the\nindex (by either 2-way merge or sparse checkout stuff) we should be\nable to just skip writing down the index, if we haven't done that\nalready.\n\nshow_local_changes() should be sped up significantly with the new\ncache-tree optimization I'm working on in another thread.\n\nIf I have not made any mistake in my analysis so far, we achieve a big\nspeedup without adding a new config knob and can still fall back to\nslower-but-same-behavior when things are not in the right condition. I\nknow it will not be the same speedup as this patch because when facing\nthousands of items, even counting them takes time. But I think it's a\nreasonable speedup without making the code base more fragile.\n-- \nDuy\n"},{"id":"354286","messageId":"3900401c-4d7b-233c-2098-9771a06ec0dd@gmail.com","threadId":"48948","inReplyTo":"CACsJy8DMEMsDnKZc65K-0EJcm2udXZ7OKY=xoFmX4COM0dSH=g@mail.gmail.com","subject":"Re: [PATCH v2] checkout: optimize \"git checkout -b \"","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-08-02T18:02:00Z","receivedAt":"2018-08-02T18:02:04Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 8/1/2018 11:10 AM, Duy Nguyen wrote:\n> On Tue, Jul 31, 2018 at 7:03 PM Ben Peart wrote:\n>>\n>> From: Ben Peart \n>>\n>> Skip merging the commit, updating the index and working directory if and\n>> only if we are creating a new branch via \"git checkout -b .\"\n>> Any other checkout options will still go through the former code path.\n> \n> I'd like to see this giant list of checks broken down and pushed down\n> to smaller areas so that chances of new things being added but checks\n> not updated become much smaller. And ideally there should just be no\n> behavior change (I think with your change, \"checkout -b\" will not\n> report local changes, but it's not mentioned in the config document;\n> more things like that can easily slip).\n> \n\nOne trade off of pushing these optimizations down into the lower-level \nfunctions is that they have a greater potential to break other command \nif our assumptions are wrong. Changing these low level functions is a \nmuch more invasive set of patches.\n\nI didn't feel confident enough to pursue this path and instead, decided \nto do the single, high level optimization around the specific scenario. \nWhile it has its own drawbacks (the nasty set of conditions we're \ntesting), the potential for breaking other commands is much smaller.\n\nThat said, I'm willing to look into the model of pushing the \nchecks/optimizations down to smaller areas if we can 1) ensure we aren't \nbreaking other commands and 2) we can get similar performance.\n\n> So. I assume this reason for this patch is because on super large worktrees\n> \n> - 2-way merge is too slow\n> - applying spare checkout patterns on a huge worktree is also slow\n> - writing index is, again, slow\n> - show_local_changes() slow\n> \n\nThat is pretty close but here is some real data on a large repo.\n\n\"git checkout -b \" with this patch takes 0.32 seconds.\n\"git checkout -b \" without this patch takes 14.6 seconds.\n\nNote, all numbers are with a hot disk cache - real world numbers for the \nunpatched case can be much worse as it has to do a lot of disk IO to \nread/write the 267 MB index, load 500K+ tree objects, etc:\n\nName \tInc %\t Inc\n ||+ git!mainCRTStartup \t 89.2\t 13,380\n || + git!__tmainCRTStartup \t 89.2\t 13,380\n || + git!cmd_main \t 89.2\t 13,380\n || + git!handle_builtin \t 89.2\t 13,380\n || + git!cmd_checkout \t 89.2\t 13,380\n || + git!unpack_trees \t 71.5\t 10,725\n || |+ git!traverse_trees \t 39.7\t 5,956\n || |+ git!cache_tree_update \t 16.1\t 2,408\n || |+ git!??unpack_callback \t 11.0\t 1,649\n || |+ git!discard_index \t 2.8\t 423\n || + git!write_locked_index \t 8.4\t 1,257\n || + git!??cache_tree_invalidate_path\t 5.1\t 767\n || + git!read_index_preload \t 3.4\t 514\n\n> For 2-way merge, I believe we can detect inside unpack_trees() that\n> it's a 2-way merge (fn == twoway_merge), from HEAD to HEAD (simple\n> enough check), then from the 2-way merge table we know for sure\n> nothing is going to change and we can just skip traverse_trees() call\n> in unpack_trees().\n> \n\nIf we can skip the call to traverse_trees(), that will give us the bulk \nof the savings (39.7% + 11% = 50.7% if my math is correct).\n\n> On the sparse checkout application. This only needs to be done when\n> there are new files added, or the spare-checkout file has been updated\n> since the last time it's been used. We can keep track of these things\n> (sparse-checkout file change could be kept track with just stat info\n> maybe as an index extension) then we can skip applying sparse checkout\n> not for this particular case for but general checkouts as well. Spare\n> checkout file rarely changes. Big win overall.\n> \n\nWith the current patch, we don't need to load or update the index at \nall. Without the patch, we've already replaced the standard \nsparse-checkout logic with something significantly faster so in our \nparticular case, I think it's safe to skip the additional complexity of \nkeeping track of changes to the sparse-checkout file.\n\n> And if all go according to plan, there will be no changes made in the\n> index (by either 2-way merge or sparse checkout stuff) we should be\n> able to just skip writing down the index, if we haven't done that\n> already.\n> \n\nThat would be great as writing the index is 8.4% of the time spent.\n\n> show_local_changes() should be sped up significantly with the new\n> cache-tree optimization I'm working on in another thread.\n> \n\nAs you can see, updating the cache_tree is relatively expensive (16.1% + \n5.1%) so we would definitely benefit from any improvements there.\n\n> If I have not made any mistake in my analysis so far, we achieve a big\n> speedup without adding a new config knob and can still fall back to\n> slower-but-same-behavior when things are not in the right condition. I\n> know it will not be the same speedup as this patch because when facing\n> thousands of items, even counting them takes time. But I think it's a\n> reasonable speedup without making the code base more fragile.\n> \n\nSo my rough math says we can potentially save 50.7% + 8.4% + (x * 21.2%) \n(where x is the percentage savings with an optimized cache_tree). If we \nassume x == 50%, that means we can save 69.7% of the overall time.\n\nFor comparison, that would put \"git checkout -b \" at:\n\n0.3 seconds - with the current patch\n4.4 seconds - with the proposed patch\n14.6 seconds - with no patch\n\nAm I missing anything? Is my math wrong? Any other ideas for how to \nimprove the proposed patch?\n\nThanks,\n\nBen\n"},{"id":"354399","messageId":"20180803155838.GA13512@duynguyen.home","threadId":"48948","inReplyTo":"3900401c-4d7b-233c-2098-9771a06ec0dd@gmail.com","subject":"Re: [PATCH v2] checkout: optimize \"git checkout -b \"","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-03T15:58:38Z","receivedAt":"2018-08-03T15:58:46Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Aug 02, 2018 at 02:02:00PM -0400, Ben Peart wrote:\n> \n> \n> On 8/1/2018 11:10 AM, Duy Nguyen wrote:\n> > On Tue, Jul 31, 2018 at 7:03 PM Ben Peart wrote:\n> >>\n> >> From: Ben Peart \n> >>\n> >> Skip merging the commit, updating the index and working directory if and\n> >> only if we are creating a new branch via \"git checkout -b .\"\n> >> Any other checkout options will still go through the former code path.\n> > \n> > I'd like to see this giant list of checks broken down and pushed down\n> > to smaller areas so that chances of new things being added but checks\n> > not updated become much smaller. And ideally there should just be no\n> > behavior change (I think with your change, \"checkout -b\" will not\n> > report local changes, but it's not mentioned in the config document;\n> > more things like that can easily slip).\n> > \n> \n> One trade off of pushing these optimizations down into the lower-level \n> functions is that they have a greater potential to break other command \n> if our assumptions are wrong. Changing these low level functions is a \n> much more invasive set of patches.\n> \n> I didn't feel confident enough to pursue this path and instead, decided \n> to do the single, high level optimization around the specific scenario. \n> While it has its own drawbacks (the nasty set of conditions we're \n> testing), the potential for breaking other commands is much smaller.\n> \n> That said, I'm willing to look into the model of pushing the \n> checks/optimizations down to smaller areas if we can 1) ensure we aren't \n> breaking other commands and 2) we can get similar performance.\n> \n> > So. I assume this reason for this patch is because on super large worktrees\n> > \n> > - 2-way merge is too slow\n> > - applying spare checkout patterns on a huge worktree is also slow\n> > - writing index is, again, slow\n> > - show_local_changes() slow\n> > \n> \n> That is pretty close but here is some real data on a large repo.\n> \n> \"git checkout -b \" with this patch takes 0.32 seconds.\n> \"git checkout -b \" without this patch takes 14.6 seconds.\n> \n> Note, all numbers are with a hot disk cache - real world numbers for the \n> unpatched case can be much worse as it has to do a lot of disk IO to \n> read/write the 267 MB index, load 500K+ tree objects, etc:\n> \n> Name \tInc %\t Inc\n> ||+ git!mainCRTStartup \t 89.2\t 13,380\n> || + git!__tmainCRTStartup \t 89.2\t 13,380\n> || + git!cmd_main \t 89.2\t 13,380\n> || + git!handle_builtin \t 89.2\t 13,380\n> || + git!cmd_checkout \t 89.2\t 13,380\n> || + git!unpack_trees \t 71.5\t 10,725\n> || |+ git!traverse_trees \t 39.7\t 5,956\n> || |+ git!cache_tree_update \t 16.1\t 2,408\n> || |+ git!??unpack_callback \t 11.0\t 1,649\n> || |+ git!discard_index \t 2.8\t 423\n> || + git!write_locked_index \t 8.4\t 1,257\n> || + git!??cache_tree_invalidate_path\t 5.1\t 767\n> || + git!read_index_preload \t 3.4\t 514\n> \n> > For 2-way merge, I believe we can detect inside unpack_trees() that\n> > it's a 2-way merge (fn == twoway_merge), from HEAD to HEAD (simple\n> > enough check), then from the 2-way merge table we know for sure\n> > nothing is going to change and we can just skip traverse_trees() call\n> > in unpack_trees().\n> > \n> \n> If we can skip the call to traverse_trees(), that will give us the bulk \n> of the savings (39.7% + 11% = 50.7% if my math is correct).\n\nThat cache_tree_invalidate_path() should belong to unpack_trees() as\nwell. At least I known unpack_trees() does that (and I'm pretty sure\nit's useless to call it in this context), but maybe it's called\nelsewhere too.\n\n> \n> > On the sparse checkout application. This only needs to be done when\n> > there are new files added, or the spare-checkout file has been updated\n> > since the last time it's been used. We can keep track of these things\n> > (sparse-checkout file change could be kept track with just stat info\n> > maybe as an index extension) then we can skip applying sparse checkout\n> > not for this particular case for but general checkouts as well. Spare\n> > checkout file rarely changes. Big win overall.\n> > \n> \n> With the current patch, we don't need to load or update the index at \n> all. Without the patch, we've already replaced the standard \n> sparse-checkout logic with something significantly faster so in our \n> particular case, I think it's safe to skip the additional complexity of \n> keeping track of changes to the sparse-checkout file.\n> \n> > And if all go according to plan, there will be no changes made in the\n> > index (by either 2-way merge or sparse checkout stuff) we should be\n> > able to just skip writing down the index, if we haven't done that\n> > already.\n> > \n> \n> That would be great as writing the index is 8.4% of the time spent.\n> \n> > show_local_changes() should be sped up significantly with the new\n> > cache-tree optimization I'm working on in another thread.\n> > \n> \n> As you can see, updating the cache_tree is relatively expensive (16.1% + \n> 5.1%) so we would definitely benefit from any improvements there.\n> \n> > If I have not made any mistake in my analysis so far, we achieve a big\n> > speedup without adding a new config knob and can still fall back to\n> > slower-but-same-behavior when things are not in the right condition. I\n> > know it will not be the same speedup as this patch because when facing\n> > thousands of items, even counting them takes time. But I think it's a\n> > reasonable speedup without making the code base more fragile.\n> > \n> \n> So my rough math says we can potentially save 50.7% + 8.4% + (x * 21.2%) \n> (where x is the percentage savings with an optimized cache_tree). If we \n> assume x == 50%, that means we can save 69.7% of the overall time.\n> \n> For comparison, that would put \"git checkout -b \" at:\n> \n> 0.3 seconds - with the current patch\n> 4.4 seconds - with the proposed patch\n> 14.6 seconds - with no patch\n\n4.4 seconds is in my opnion already very reasonable compared to the\nno-patch number, and other more often used commands, which I assume\ntake seconds as well. Once common commands take a lot less time, we\ncould have another look at this \"checkout -b\".\n\nThese numbers btw should be in the commit message if you send another\noptimization patch because it's very unconvincing to say \"optimize\"\nwithout numbers. My initial reaction was, the majority of open source\nprojects are of moderate size and \"checkout\" takes subseconds anyway,\nwhat is the point of optimization (even if you could reduce 90% of\ntime) since it will not be noticeable anyway?\n\n> Am I missing anything? Is my math wrong? Any other ideas for how to \n> improve the proposed patch?\n\nBut if you still want to push it further, this is something I have in\nmind. It probably has bugs, but at least preliminary test shows me\nthat it could skip 99% work inside unpack_trees() and not need to\nwrite the index.\n\nThe main check to determine \"checkout -b\" is basically the new\noidcmp() in merge_working_tree(). Not sure if I miss any condition in\nthat check, I didn't look very closely at checkout code.\n\n-- 8< --\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 28627650cd..912e565acc 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -478,6 +478,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n {\n \tint ret;\n \tstruct lock_file lock_file = LOCK_INIT;\n+\tint skip_cache_tree_update = 0;\n \n \thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n \tif (read_cache_preload(NULL) < 0)\n@@ -493,6 +494,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\tstruct tree_desc trees[2];\n \t\tstruct tree *tree;\n \t\tstruct unpack_trees_options topts;\n+\t\tint nr_trees;\n \n \t\tmemset(&topts, 0, sizeof(topts));\n \t\ttopts.head_idx = -1;\n@@ -514,20 +516,45 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\ttopts.merge = 1;\n \t\ttopts.gently = opts->merge && old_branch_info->commit;\n \t\ttopts.verbose_update = opts->show_progress;\n-\t\ttopts.fn = twoway_merge;\n+\t\t/*\n+\t\t * We repair cache-tree explicitly in this function,\n+\t\t * no point in doing it again inside unpack_trees()\n+\t\t */\n+\t\ttopts.skip_cache_tree_update = 1;\n \t\tif (opts->overwrite_ignore) {\n \t\t\ttopts.dir = xcalloc(1, sizeof(*topts.dir));\n \t\t\ttopts.dir->flags |= DIR_SHOW_IGNORED;\n \t\t\tsetup_standard_excludes(topts.dir);\n \t\t}\n-\t\ttree = parse_tree_indirect(old_branch_info->commit ?\n-\t\t\t\t\t &old_branch_info->commit->object.oid :\n-\t\t\t\t\t the_hash_algo->empty_tree);\n-\t\tinit_tree_desc(&trees[0], tree->buffer, tree->size);\n-\t\ttree = parse_tree_indirect(&new_branch_info->commit->object.oid);\n-\t\tinit_tree_desc(&trees[1], tree->buffer, tree->size);\n-\n-\t\tret = unpack_trees(2, trees, &topts);\n+\t\tif (!oidcmp(&old_branch_info->commit->object.oid,\n+\t\t\t &new_branch_info->commit->object.oid)) {\n+\t\t\t/*\n+\t\t\t * Skip 2-way merge because we know the end\n+\t\t\t * result would be the same anyway (to double\n+\t\t\t * check).\n+\t\t\t */\n+\t\t\ttopts.fn = noop_merge;\n+\t\t\tnr_trees = 0;\n+\t\t\t/*\n+\t\t\t * Repairing cache-tree is an optional optimization\n+\t\t\t * and could be safely skipped. Maybe have some\n+\t\t\t * heuristics based on the_index->cache_nr to\n+\t\t\t * determine if updating cache-tree could be too\n+\t\t\t * costly (including index writing cost) and skip it\n+\t\t\t * instead.\n+\t\t\t */\n+\t\t\tskip_cache_tree_update = 1;\n+\t\t} else {\n+\t\t\ttopts.fn = twoway_merge;\n+\t\t\ttree = parse_tree_indirect(old_branch_info->commit ?\n+\t\t\t\t\t\t &old_branch_info->commit->object.oid :\n+\t\t\t\t\t\t the_hash_algo->empty_tree);\n+\t\t\tinit_tree_desc(&trees[0], tree->buffer, tree->size);\n+\t\t\ttree = parse_tree_indirect(&new_branch_info->commit->object.oid);\n+\t\t\tinit_tree_desc(&trees[1], tree->buffer, tree->size);\n+\t\t\tnr_trees = 2;\n+\t\t}\n+\t\tret = unpack_trees(nr_trees, trees, &topts);\n \t\tclear_unpack_trees_porcelain(&topts);\n \t\tif (ret == -1) {\n \t\t\t/*\n@@ -597,13 +624,15 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t}\n \t}\n \n-\tif (!active_cache_tree)\n-\t\tactive_cache_tree = cache_tree();\n+\tif (!skip_cache_tree_update) {\n+\t\tif (!active_cache_tree)\n+\t\t\tactive_cache_tree = cache_tree();\n \n-\tif (!cache_tree_fully_valid(active_cache_tree))\n-\t\tcache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n+\t\tif (!cache_tree_fully_valid(active_cache_tree))\n+\t\t\tcache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n+\t}\n \n-\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK | SKIP_IF_UNCHANGED))\n \t\tdie(_(\"unable to write new index file\"));\n \n \tif (!opts->force && !opts->quiet)\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex cd0680f11e..007d6662f2 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1275,6 +1275,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+\tint result_is_actually_src = 0;\n \n \tif (len > MAX_UNPACK_TREES)\n \t\tdie(\"unpack_trees takes at most %d trees\", MAX_UNPACK_TREES);\n@@ -1353,6 +1354,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \n \t\tif (traverse_trees(len, t, &info) < 0)\n \t\t\tgoto return_failed;\n+\t} else if (o->fn == noop_merge) {\n+\t\to->result = *o->src_index;\n+\t\to->cache_bottom = o->src_index->cache_nr;\n+\t\tresult_is_actually_src = 1;\n \t}\n \n \t/* Any left-over entries in the index? */\n@@ -1426,7 +1431,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \n \tret = check_updates(o) ? (-2) : 0;\n \tif (o->dst_index) {\n-\t\tif (!ret) {\n+\t\tif (!ret &&!o->skip_cache_tree_update) {\n \t\t\tif (!o->result.cache_tree)\n \t\t\t\to->result.cache_tree = cache_tree();\n \t\t\tif (!cache_tree_fully_valid(o->result.cache_tree))\n@@ -1435,7 +1440,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t\t\t\t\t\t WRITE_TREE_REPAIR);\n \t\t}\n \t\tmove_index_extensions(&o->result, o->src_index);\n-\t\tdiscard_index(o->dst_index);\n+\t\tif (result_is_actually_src && o->dst_index == o->src_index)\n+\t\t\t; /* no discarding */\n+\t\telse\n+\t\t\tdiscard_index(o->dst_index);\n \t\t*o->dst_index = o->result;\n \t} else {\n \t\tdiscard_index(&o->result);\n@@ -2193,3 +2201,9 @@ int oneway_merge(const struct cache_entry * const *src,\n \t}\n \treturn merged_entry(a, old, o);\n }\n+\n+int noop_merge(const struct cache_entry * const *src,\n+\t struct unpack_trees_options *o)\n+{\n+\tBUG(\"This is basically a flag and not supposed to be called\");\n+}\ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex c2b434c606..3e5bce19a1 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -52,6 +52,7 @@ struct unpack_trees_options {\n \t\t diff_index_cached,\n \t\t debug_unpack,\n \t\t skip_sparse_checkout,\n+\t\t skip_cache_tree_update,\n \t\t gently,\n \t\t exiting_early,\n \t\t show_all_errors,\n@@ -96,5 +97,7 @@ int bind_merge(const struct cache_entry * const *src,\n \t struct unpack_trees_options *o);\n int oneway_merge(const struct cache_entry * const *src,\n \t\t struct unpack_trees_options *o);\n+int noop_merge(const struct cache_entry * const *src,\n+\t struct unpack_trees_options *o);\n \n #endif\n-- 8< --\n\n--\nDuy\n"},{"id":"354541","messageId":"20180805085718.GA27625@duynguyen.home","threadId":"48948","inReplyTo":"3900401c-4d7b-233c-2098-9771a06ec0dd@gmail.com","subject":"Re: [PATCH v2] checkout: optimize \"git checkout -b \"","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-05T08:57:18Z","receivedAt":"2018-08-05T08:57:26Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Aug 02, 2018 at 02:02:00PM -0400, Ben Peart wrote:\n> > And if all go according to plan, there will be no changes made in the\n> > index (by either 2-way merge or sparse checkout stuff) we should be\n> > able to just skip writing down the index, if we haven't done that\n> > already.\n> > \n> \n> That would be great as writing the index is 8.4% of the time spent.\n\nAnd that is of course not good. Avoiding writing index at all is nice\nand all but we should not take that much time writing it (what about\nother commands?)\n\nI have another idea to reduce index size, which means cheaper trailer\nhash calculation, but this may also be a direction you could look\ninto.\n\nI notice that we have a lot of duplicate data in the index. For\nexample, most files would have the same ctime and even mtime (at least\nthe seconds part). group id, device id and file mode should also be\nthe same in normal case. I guess on Windows, inode is always the same\nas well. So why store (and hash) them repeatedly?\n\nThis patch writes \"index version 5\" that only writes those fields out\nif they are different from the previous entry (we already do this for\npath name in v4).\n\nOn webkit repo (275k files), writing time can be reduced from 0.314s\n(v2) to 0.206s (v5). File size of all versions:\n\n 9.8M Aug 5 10:27 .git/index.gz\n 38M Aug 5 09:54 .git/index-v2\n 22M Aug 5 10:26 .git/index-v4\n 15M Aug 5 10:34 .git/index-v5\n\nAs you can see there's still room to compress, but I don't think we\nwant to go full deflate mode because compression time goes up.\n\nAnother option I think you could consider is not storing full hash. As\nlong as the shortened hash is still unique, we can expand to full hash\nat read time. Of course you can't go too short (I'm thinking half the\ncurrent hash size is reasonable trade off) and need to take into\naccount how much time will be lost for looking up and expanding these\nshort hashes at read time.\n\n-- 8< --\ndiff --git a/cache.h b/cache.h\nindex e6f7ee4b64..1d6bc7d122 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -129,7 +129,7 @@ struct cache_header {\n };\n \n #define INDEX_FORMAT_LB 2\n-#define INDEX_FORMAT_UB 4\n+#define INDEX_FORMAT_UB 5\n \n /*\n * The \"cache_time\" is just the low 32 bits of the\ndiff --git a/read-cache.c b/read-cache.c\nindex b0b5df5de7..9bb095d6b2 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2002,7 +2002,7 @@ static int ce_write_flush(git_hash_ctx *context, int fd)\n \treturn 0;\n }\n \n-static int ce_write(git_hash_ctx *context, int fd, void *data, unsigned int len)\n+static int ce_write(git_hash_ctx *context, int fd, const void *data, unsigned int len)\n {\n \twhile (len) {\n \t\tunsigned int buffered = write_buffer_len;\n@@ -2024,6 +2024,18 @@ static int ce_write(git_hash_ctx *context, int fd, void *data, unsigned int len)\n \treturn 0;\n }\n \n+static int ce_write_u32(git_hash_ctx *context, int fd, uint32_t data)\n+{\n+\tdata = htonl(data);\n+\treturn ce_write(context, fd, &data, sizeof(data));\n+}\n+\n+static int ce_write_u16(git_hash_ctx *context, int fd, uint16_t data)\n+{\n+\tdata = htons(data);\n+\treturn ce_write(context, fd, &data, sizeof(data));\n+}\n+\n static int write_index_ext_header(git_hash_ctx *context, int fd,\n \t\t\t\t unsigned int ext, unsigned int sz)\n {\n@@ -2133,8 +2145,46 @@ static void copy_cache_entry_to_ondisk(struct ondisk_cache_entry *ondisk,\n \t}\n }\n \n+#define ONDISK_MASK (CE_STAGEMASK | CE_INTENT_TO_ADD | CE_SKIP_WORKTREE)\n+\n+static int ce_write_entry_v5(git_hash_ctx *c, int fd,\n+\t\t\t const struct cache_entry *ce,\n+\t\t\t const struct cache_entry *pce)\n+{\n+\tuint8_t mask = 0;\n+\tconst struct stat_data *st1 = &ce->ce_stat_data;\n+\tconst struct stat_data *st2 = &pce->ce_stat_data;\n+\t//int ret = 0;\n+\n+\tmask |= (st1->sd_ctime.sec != st2->sd_ctime.sec) << 0;\n+\tmask |= (st1->sd_mtime.sec != st2->sd_mtime.sec) << 1;\n+\tmask |= (st1->sd_dev != st2->sd_dev) << 2;\n+\tmask |= (st1->sd_ino != st2->sd_ino) << 3;\n+\tmask |= (ce->ce_mode != pce->ce_mode) << 4;\n+\tmask |= (st1->sd_uid != st2->sd_uid) << 5;\n+\tmask |= (st1->sd_gid != st2->sd_gid) << 6;\n+\tmask |= ((ce->ce_flags & ONDISK_MASK) != (pce->ce_flags & ONDISK_MASK)) << 7;\n+\n+\tce_write(c, fd, &mask, 1);\n+\tce_write(c, fd, ce->oid.hash, GIT_SHA1_RAWSZ);\n+\tce_write_u32(c, fd, st1->sd_ctime.nsec);\n+\tce_write_u32(c, fd, st1->sd_mtime.nsec);\n+\tce_write_u32(c, fd, st1->sd_size);\n+\tif (mask & (1 << 0)) ce_write_u32(c, fd, st1->sd_ctime.sec);\n+\tif (mask & (1 << 1)) ce_write_u32(c, fd, st1->sd_mtime.sec);\n+\tif (mask & (1 << 2)) ce_write_u32(c, fd, st1->sd_dev);\n+\tif (mask & (1 << 3)) ce_write_u32(c, fd, st1->sd_ino);\n+\tif (mask & (1 << 4)) ce_write_u16(c, fd, ce->ce_mode);\n+\tif (mask & (1 << 5)) ce_write_u32(c, fd, st1->sd_uid);\n+\tif (mask & (1 << 6)) ce_write_u32(c, fd, st1->sd_gid);\n+\tif (mask & (1 << 7)) ce_write_u32(c, fd, ce->ce_flags & ONDISK_MASK);\n+\treturn 0;\n+}\n+\n static int ce_write_entry(git_hash_ctx *c, int fd, struct cache_entry *ce,\n-\t\t\t struct strbuf *previous_name, struct ondisk_cache_entry *ondisk)\n+\t\t\t const struct cache_entry *pce,\n+\t\t\t struct strbuf *previous_name,\n+\t\t\t struct ondisk_cache_entry *ondisk)\n {\n \tint size;\n \tint result;\n@@ -2173,8 +2223,12 @@ static int ce_write_entry(git_hash_ctx *c, int fd, struct cache_entry *ce,\n \t\tto_remove = previous_name->len - common;\n \t\tprefix_size = encode_varint(to_remove, to_remove_vi);\n \n-\t\tcopy_cache_entry_to_ondisk(ondisk, ce);\n-\t\tresult = ce_write(c, fd, ondisk, size);\n+\t\tif (!pce) {\n+\t\t\tcopy_cache_entry_to_ondisk(ondisk, ce);\n+\t\t\tresult = ce_write(c, fd, ondisk, size);\n+\t\t} else {\n+\t\t\tresult = ce_write_entry_v5(c, fd, ce, pce);\n+\t\t}\n \t\tif (!result)\n \t\t\tresult = ce_write(c, fd, to_remove_vi, prefix_size);\n \t\tif (!result)\n@@ -2313,7 +2367,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \tif (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0)\n \t\treturn -1;\n \n-\tprevious_name = (hdr_version == 4) ? &previous_name_buf : NULL;\n+\tprevious_name = (hdr_version >= 4) ? &previous_name_buf : NULL;\n \n \tfor (i = 0; i < entries; i++) {\n \t\tstruct cache_entry *ce = cache[i];\n@@ -2334,7 +2388,10 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \n \t\t\tdrop_cache_tree = 1;\n \t\t}\n-\t\tif (ce_write_entry(&c, newfd, ce, previous_name, (struct ondisk_cache_entry *)&ondisk) < 0)\n+\t\tif (ce_write_entry(&c, newfd, ce,\n+\t\t\t\t hdr_version >= 5 && i > 0 ? cache[i - 1] : NULL,\n+\t\t\t\t previous_name,\n+\t\t\t\t (struct ondisk_cache_entry *)&ondisk) < 0)\n \t\t\terr = -1;\n \n \t\tif (err)\n-- 8< --\n--\nDuy\n"},{"id":"354607","messageId":"21ac52cb-6a23-01c7-c593-59bd86ebca52@gmail.com","threadId":"48948","inReplyTo":"20180803155838.GA13512@duynguyen.home","subject":"Re: [PATCH v2] checkout: optimize \"git checkout -b \"","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-08-06T14:25:36Z","receivedAt":"2018-08-06T14:25:41Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 8/3/2018 11:58 AM, Duy Nguyen wrote:\n> On Thu, Aug 02, 2018 at 02:02:00PM -0400, Ben Peart wrote:\n>>\n>>\n\n> But if you still want to push it further, this is something I have in\n> mind. It probably has bugs, but at least preliminary test shows me\n> that it could skip 99% work inside unpack_trees() and not need to\n> write the index.\n> \n> The main check to determine \"checkout -b\" is basically the new\n> oidcmp() in merge_working_tree(). Not sure if I miss any condition in\n> that check, I didn't look very closely at checkout code.\n> \n\nThanks Duy. I think this is an interesting idea to pursue but... when I \ntried running this patch on a virtualized repo it started triggering \nmany object downloads. After taking a quick look, it appears that \nCE_UPDATE is set on every cache entry so check_updates() ends up calling \ncheckout_entry() which writes out every file to the working tree - even \nthose supposedly skipped by the skip-wortree bit. Oops.\n\nNot too surprising (you did say it probably has bugs :)) but it means I \ncan't trivially get performance data on how much this will help. It \nalso fails a lot of tests (see below).\n\nIt experience does highlight the additional risk of this model of \nchanging the underlying functions (vs the high level optimization of my \noriginal patch). In addition, the new special cases in those \nlower-level functions do add additional complexity and fragility to the \ncodebase. So, like most things, to me it isn't a clear better/worse \ndecision - it's just different. While I like the idea of general \noptimizations that could apply more broadly to other commands; I do \nworry about the additional complexity, amount of code churn, and \nassociated risk with the change.\n\nWhen I have cycles, I'll take a look at how to fix this bug and get some \nperformance data. I just wanted to give you a heads up that I'm not \nignoring your patch, just that it is going to take additional time and \neffort before I can properly evaluate how much impact it will have.\n\n\nTest Summary Report\n-------------------\n./t1011-read-tree-sparse-checkout.sh (Wstat: 256 Tests: 21 \nFailed: 1)\n Failed test: 20\n Non-zero exit status: 1\n./t1400-update-ref.sh (Wstat: 256 Tests: \n170 Failed: 73)\n Failed tests: 40, 42-45, 55-59, 70, 72, 82, 85, 87-88\n 90-100, 103-110, 113-119, 127, 129-130\n 132-133, 136-137, 140-147, 150-157, 160-166\n 170\n Non-zero exit status: 1\n./t2011-checkout-invalid-head.sh (Wstat: 256 Tests: 10 \nFailed: 5)\n Failed tests: 3, 6-7, 9-10\n Non-zero exit status: 1\n./t2015-checkout-unborn.sh (Wstat: 256 Tests: 6 \nFailed: 3)\n Failed tests: 2-4\n Non-zero exit status: 1\n./t2017-checkout-orphan.sh (Wstat: 256 Tests: 13 \nFailed: 7)\n Failed tests: 7-13\n Non-zero exit status: 1\n./t3033-merge-toplevel.sh (Wstat: 256 Tests: 13 \nFailed: 11)\n Failed tests: 3-13\n Non-zero exit status: 1\n./t3200-branch.sh (Wstat: 256 Tests: \n139 Failed: 2)\n Failed tests: 137-138\n Non-zero exit status: 1\n./t5616-partial-clone.sh (Wstat: 256 Tests: 13 \nFailed: 1)\n Failed test: 4\n Non-zero exit status: 1\n./t5516-fetch-push.sh (Wstat: 256 Tests: 90 \nFailed: 1)\n Failed test: 34\n Non-zero exit status: 1\n./t6300-for-each-ref.sh (Wstat: 256 Tests: \n205 Failed: 9)\n Failed tests: 189-196, 199\n Non-zero exit status: 1\n./t7114-reset-sparse-checkout.sh (Wstat: 256 Tests: 3 \nFailed: 2)\n Failed tests: 2-3\n Non-zero exit status: 1\n./t7063-status-untracked-cache.sh (Wstat: 256 Tests: 50 \nFailed: 1)\n Failed test: 23\n Non-zero exit status: 1\n./t7201-co.sh (Wstat: 256 Tests: 38 \nFailed: 33)\n Failed tests: 4, 6-27, 29-38\n Non-zero exit status: 1\n./t7409-submodule-detached-work-tree.sh (Wstat: 256 Tests: 2 \nFailed: 1)\n Failed test: 1\n Non-zero exit status: 1\n./t9350-fast-export.sh (Wstat: 256 Tests: 37 \nFailed: 1)\n Failed test: 12\n Non-zero exit status: 1\n./t9903-bash-prompt.sh (Wstat: 256 Tests: 65 \nFailed: 52)\n Failed tests: 4, 6-10, 14-34, 36, 39-51, 53-62, 65\n Non-zero exit status: 1\nFiles=834, Tests=19658, 2081 wallclock secs (10.42 usr 15.09 sys + \n1082.56 cusr 3530.46 csys = 4638.53 CPU)\nResult: FAIL\n\n\n> -- 8< --\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 28627650cd..912e565acc 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -478,6 +478,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n> {\n> \tint ret;\n> \tstruct lock_file lock_file = LOCK_INIT;\n> +\tint skip_cache_tree_update = 0;\n> \n> \thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\n> \tif (read_cache_preload(NULL) < 0)\n> @@ -493,6 +494,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n> \t\tstruct tree_desc trees[2];\n> \t\tstruct tree *tree;\n> \t\tstruct unpack_trees_options topts;\n> +\t\tint nr_trees;\n> \n> \t\tmemset(&topts, 0, sizeof(topts));\n> \t\ttopts.head_idx = -1;\n> @@ -514,20 +516,45 @@ static int merge_working_tree(const struct checkout_opts *opts,\n> \t\ttopts.merge = 1;\n> \t\ttopts.gently = opts->merge && old_branch_info->commit;\n> \t\ttopts.verbose_update = opts->show_progress;\n> -\t\ttopts.fn = twoway_merge;\n> +\t\t/*\n> +\t\t * We repair cache-tree explicitly in this function,\n> +\t\t * no point in doing it again inside unpack_trees()\n> +\t\t */\n> +\t\ttopts.skip_cache_tree_update = 1;\n> \t\tif (opts->overwrite_ignore) {\n> \t\t\ttopts.dir = xcalloc(1, sizeof(*topts.dir));\n> \t\t\ttopts.dir->flags |= DIR_SHOW_IGNORED;\n> \t\t\tsetup_standard_excludes(topts.dir);\n> \t\t}\n> -\t\ttree = parse_tree_indirect(old_branch_info->commit ?\n> -\t\t\t\t\t &old_branch_info->commit->object.oid :\n> -\t\t\t\t\t the_hash_algo->empty_tree);\n> -\t\tinit_tree_desc(&trees[0], tree->buffer, tree->size);\n> -\t\ttree = parse_tree_indirect(&new_branch_info->commit->object.oid);\n> -\t\tinit_tree_desc(&trees[1], tree->buffer, tree->size);\n> -\n> -\t\tret = unpack_trees(2, trees, &topts);\n> +\t\tif (!oidcmp(&old_branch_info->commit->object.oid,\n> +\t\t\t &new_branch_info->commit->object.oid)) {\n> +\t\t\t/*\n> +\t\t\t * Skip 2-way merge because we know the end\n> +\t\t\t * result would be the same anyway (to double\n> +\t\t\t * check).\n> +\t\t\t */\n> +\t\t\ttopts.fn = noop_merge;\n> +\t\t\tnr_trees = 0;\n> +\t\t\t/*\n> +\t\t\t * Repairing cache-tree is an optional optimization\n> +\t\t\t * and could be safely skipped. Maybe have some\n> +\t\t\t * heuristics based on the_index->cache_nr to\n> +\t\t\t * determine if updating cache-tree could be too\n> +\t\t\t * costly (including index writing cost) and skip it\n> +\t\t\t * instead.\n> +\t\t\t */\n> +\t\t\tskip_cache_tree_update = 1;\n> +\t\t} else {\n> +\t\t\ttopts.fn = twoway_merge;\n> +\t\t\ttree = parse_tree_indirect(old_branch_info->commit ?\n> +\t\t\t\t\t\t &old_branch_info->commit->object.oid :\n> +\t\t\t\t\t\t the_hash_algo->empty_tree);\n> +\t\t\tinit_tree_desc(&trees[0], tree->buffer, tree->size);\n> +\t\t\ttree = parse_tree_indirect(&new_branch_info->commit->object.oid);\n> +\t\t\tinit_tree_desc(&trees[1], tree->buffer, tree->size);\n> +\t\t\tnr_trees = 2;\n> +\t\t}\n> +\t\tret = unpack_trees(nr_trees, trees, &topts);\n> \t\tclear_unpack_trees_porcelain(&topts);\n> \t\tif (ret == -1) {\n> \t\t\t/*\n> @@ -597,13 +624,15 @@ static int merge_working_tree(const struct checkout_opts *opts,\n> \t\t}\n> \t}\n> \n> -\tif (!active_cache_tree)\n> -\t\tactive_cache_tree = cache_tree();\n> +\tif (!skip_cache_tree_update) {\n> +\t\tif (!active_cache_tree)\n> +\t\t\tactive_cache_tree = cache_tree();\n> \n> -\tif (!cache_tree_fully_valid(active_cache_tree))\n> -\t\tcache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n> +\t\tif (!cache_tree_fully_valid(active_cache_tree))\n> +\t\t\tcache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n> +\t}\n> \n> -\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n> +\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK | SKIP_IF_UNCHANGED))\n> \t\tdie(_(\"unable to write new index file\"));\n> \n> \tif (!opts->force && !opts->quiet)\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index cd0680f11e..007d6662f2 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -1275,6 +1275,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> +\tint result_is_actually_src = 0;\n> \n> \tif (len > MAX_UNPACK_TREES)\n> \t\tdie(\"unpack_trees takes at most %d trees\", MAX_UNPACK_TREES);\n> @@ -1353,6 +1354,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n> \n> \t\tif (traverse_trees(len, t, &info) < 0)\n> \t\t\tgoto return_failed;\n> +\t} else if (o->fn == noop_merge) {\n> +\t\to->result = *o->src_index;\n> +\t\to->cache_bottom = o->src_index->cache_nr;\n> +\t\tresult_is_actually_src = 1;\n> \t}\n> \n> \t/* Any left-over entries in the index? */\n> @@ -1426,7 +1431,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n> \n> \tret = check_updates(o) ? (-2) : 0;\n> \tif (o->dst_index) {\n> -\t\tif (!ret) {\n> +\t\tif (!ret &&!o->skip_cache_tree_update) {\n> \t\t\tif (!o->result.cache_tree)\n> \t\t\t\to->result.cache_tree = cache_tree();\n> \t\t\tif (!cache_tree_fully_valid(o->result.cache_tree))\n> @@ -1435,7 +1440,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n> \t\t\t\t\t\t WRITE_TREE_REPAIR);\n> \t\t}\n> \t\tmove_index_extensions(&o->result, o->src_index);\n> -\t\tdiscard_index(o->dst_index);\n> +\t\tif (result_is_actually_src && o->dst_index == o->src_index)\n> +\t\t\t; /* no discarding */\n> +\t\telse\n> +\t\t\tdiscard_index(o->dst_index);\n> \t\t*o->dst_index = o->result;\n> \t} else {\n> \t\tdiscard_index(&o->result);\n> @@ -2193,3 +2201,9 @@ int oneway_merge(const struct cache_entry * const *src,\n> \t}\n> \treturn merged_entry(a, old, o);\n> }\n> +\n> +int noop_merge(const struct cache_entry * const *src,\n> +\t struct unpack_trees_options *o)\n> +{\n> +\tBUG(\"This is basically a flag and not supposed to be called\");\n> +}\n> diff --git a/unpack-trees.h b/unpack-trees.h\n> index c2b434c606..3e5bce19a1 100644\n> --- a/unpack-trees.h\n> +++ b/unpack-trees.h\n> @@ -52,6 +52,7 @@ struct unpack_trees_options {\n> \t\t diff_index_cached,\n> \t\t debug_unpack,\n> \t\t skip_sparse_checkout,\n> +\t\t skip_cache_tree_update,\n> \t\t gently,\n> \t\t exiting_early,\n> \t\t show_all_errors,\n> @@ -96,5 +97,7 @@ int bind_merge(const struct cache_entry * const *src,\n> \t struct unpack_trees_options *o);\n> int oneway_merge(const struct cache_entry * const *src,\n> \t\t struct unpack_trees_options *o);\n> +int noop_merge(const struct cache_entry * const *src,\n> +\t struct unpack_trees_options *o);\n> \n> #endif\n> -- 8< --\n> \n> --\n> Duy\n> \n"},{"id":"355763","messageId":"d15a36bc-96a8-5d92-5e41-23d726535cf3@gmail.com","threadId":"48948","inReplyTo":"21ac52cb-6a23-01c7-c593-59bd86ebca52@gmail.com","subject":"Re: [PATCH v2] checkout: optimize \"git checkout -b \"","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-08-15T21:05:36Z","receivedAt":"2018-08-15T21:05:41Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 8/6/2018 10:25 AM, Ben Peart wrote:\n> \n> \n> On 8/3/2018 11:58 AM, Duy Nguyen wrote:\n>> On Thu, Aug 02, 2018 at 02:02:00PM -0400, Ben Peart wrote:\n>>>\n>>>\n> \n>> But if you still want to push it further, this is something I have in\n>> mind. It probably has bugs, but at least preliminary test shows me\n>> that it could skip 99% work inside unpack_trees() and not need to\n>> write the index.\n>>\n>> The main check to determine \"checkout -b\" is basically the new\n>> oidcmp() in merge_working_tree(). Not sure if I miss any condition in\n>> that check, I didn't look very closely at checkout code.\n>>\n> \n> Thanks Duy.  I think this is an interesting idea to pursue but... when I \n> tried running this patch on a virtualized repo it started triggering \n> many object downloads.  After taking a quick look, it appears that \n> CE_UPDATE is set on every cache entry so check_updates() ends up calling \n> checkout_entry() which writes out every file to the working tree - even \n> those supposedly skipped by the skip-wortree bit.  Oops.\n> \n> Not too surprising (you did say it probably has bugs :)) but it means I \n> can't trivially get performance data on how much this will help.  It \n> also fails a lot of tests (see below).\n> \n> It experience does highlight the additional risk of this model of \n> changing the underlying functions (vs the high level optimization of my \n> original patch).  In addition, the new special cases in those \n> lower-level functions do add additional complexity and fragility to the \n> codebase.  So, like most things, to me it isn't a clear better/worse \n> decision - it's just different.  While I like the idea of general \n> optimizations that could apply more broadly to other commands; I do \n> worry about the additional complexity, amount of code churn, and \n> associated risk with the change.\n> \n> When I have cycles, I'll take a look at how to fix this bug and get some \n> performance data.  I just wanted to give you a heads up that I'm not \n> ignoring your patch, just that it is going to take additional time and \n> effort before I can properly evaluate how much impact it will have.\n> \n\nNow that the unpack-trees and cache-tree optimizations are settling \ndown, I took a look at this proposed patch again with the intent of \ndebugging why so many tests were broken by it.\n\nThe most obvious first fix was for all the segment faults when \ndereferencing a NULL pointer. Adding an additional test so that we only \nperform the optimization when we actually have commit ID's to compare \nfixed a bunch of the test failures.\n\nThe next fix was to resolve all the breaks caused by applying this \noptimization when sparse-checkout is turned on. Since we are skipping \nthe logic to update the skip-worktree bit, I added an additional test so \nthat we only perform the optimization when sparse checkout is not turned \non. Of course, this does completely remove the optimization when using \nsparse checkout so it isn't a workable permanent solution but it let me \nmake progress.\n\nThere are still test failures with submodules and partial clone. I \nhaven't found/added the necessary tests to prevent those breaks nor the \nfew other remaining breaks.\n\nMy current set of tests looks like this:\n\n\tif (!core_apply_sparse_checkout &&\n\t\told_branch_info->commit &&\n\t\tnew_branch_info->commit &&\n\t\t!oidcmp(&old_branch_info->commit->object.oid,\n\t\t\t&new_branch_info->commit->object.oid)) {\n\nWhile I'm sure I could find and add additional tests to handle the \nremaining bugs, the net result is starting to look as fragile as the \noriginal patch.\n\nUnfortunately it has the additional downsides of 1) being at a much \nlower level where we risk breaking more code paths and 2) not being \nnearly as much savings (with the original patch checkout -b \ntakes 0.3 seconds, this patch will make it take >4 seconds.)\n\nNet, net - I don't think this particular path is a better path to pursue.\n\nI understand the concern with the fragility of the current patch and \nit's set of tests to determine if the optimization is valid. I also \nunderstand the concern with the potential change in behavior (ie not \nshowing the local changes - even though nothing has changed). Other \nthan switching the optimization back to be \"opt-in\" via a config flag, I \ndon't currently have a great answer. I'll keep thinking and looking but \nam open to suggestions!\n\n\n\n> Test Summary Report\n> -------------------\n> ./t1011-read-tree-sparse-checkout.sh               (Wstat: 256 Tests: 21 \n> Failed: 1)\n>   Failed test:  20\n>   Non-zero exit status: 1\n> ./t1400-update-ref.sh                              (Wstat: 256 Tests: \n> 170 Failed: 73)\n>   Failed tests:  40, 42-45, 55-59, 70, 72, 82, 85, 87-88\n>                 90-100, 103-110, 113-119, 127, 129-130\n>                 132-133, 136-137, 140-147, 150-157, 160-166\n>                 170\n>   Non-zero exit status: 1\n> ./t2011-checkout-invalid-head.sh                   (Wstat: 256 Tests: 10 \n> Failed: 5)\n>   Failed tests:  3, 6-7, 9-10\n>   Non-zero exit status: 1\n> ./t2015-checkout-unborn.sh                         (Wstat: 256 Tests: 6 \n> Failed: 3)\n>   Failed tests:  2-4\n>   Non-zero exit status: 1\n> ./t2017-checkout-orphan.sh                         (Wstat: 256 Tests: 13 \n> Failed: 7)\n>   Failed tests:  7-13\n>   Non-zero exit status: 1\n> ./t3033-merge-toplevel.sh                          (Wstat: 256 Tests: 13 \n> Failed: 11)\n>   Failed tests:  3-13\n>   Non-zero exit status: 1\n> ./t3200-branch.sh                                  (Wstat: 256 Tests: \n> 139 Failed: 2)\n>   Failed tests:  137-138\n>   Non-zero exit status: 1\n> ./t5616-partial-clone.sh                           (Wstat: 256 Tests: 13 \n> Failed: 1)\n>   Failed test:  4\n>   Non-zero exit status: 1\n> ./t5516-fetch-push.sh                              (Wstat: 256 Tests: 90 \n> Failed: 1)\n>   Failed test:  34\n>   Non-zero exit status: 1\n> ./t6300-for-each-ref.sh                            (Wstat: 256 Tests: \n> 205 Failed: 9)\n>   Failed tests:  189-196, 199\n>   Non-zero exit status: 1\n> ./t7114-reset-sparse-checkout.sh                   (Wstat: 256 Tests: 3 \n> Failed: 2)\n>   Failed tests:  2-3\n>   Non-zero exit status: 1\n> ./t7063-status-untracked-cache.sh                  (Wstat: 256 Tests: 50 \n> Failed: 1)\n>   Failed test:  23\n>   Non-zero exit status: 1\n> ./t7201-co.sh                                      (Wstat: 256 Tests: 38 \n> Failed: 33)\n>   Failed tests:  4, 6-27, 29-38\n>   Non-zero exit status: 1\n> ./t7409-submodule-detached-work-tree.sh            (Wstat: 256 Tests: 2 \n> Failed: 1)\n>   Failed test:  1\n>   Non-zero exit status: 1\n> ./t9350-fast-export.sh                             (Wstat: 256 Tests: 37 \n> Failed: 1)\n>   Failed test:  12\n>   Non-zero exit status: 1\n> ./t9903-bash-prompt.sh                             (Wstat: 256 Tests: 65 \n> Failed: 52)\n>   Failed tests:  4, 6-10, 14-34, 36, 39-51, 53-62, 65\n>   Non-zero exit status: 1\n> Files=834, Tests=19658, 2081 wallclock secs (10.42 usr 15.09 sys + \n> 1082.56 cusr 3530.46 csys = 4638.53 CPU)\n> Result: FAIL\n> \n> \n"},{"id":"355867","messageId":"20180816182653.15580-1-benpeart@microsoft.com","threadId":"48948","inReplyTo":"20180724180122.29212-1-benpeart@microsoft.com","subject":"[PATCH v3] checkout: optimize \"git checkout -b \"","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2018-08-16T18:27:11Z","receivedAt":"2018-08-16T18:27:32Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"From: Ben Peart \n\nSkip merging the commit, updating the index and working directory if and\nonly if we are creating a new branch via \"git checkout -b .\"\nAny other checkout options will still go through the former code path.\n\nIf sparse_checkout is on, require the user to manually opt in to this\noptimzed behavior by setting the config setting checkout.optimizeNewBranch\nto true as we will no longer update the skip-worktree bit in the index, nor\nadd/remove files in the working directory to reflect the current sparse\ncheckout settings.\n\nFor comparison, running \"git checkout -b \" on a large repo takes:\n\n14.6 seconds - without this patch\n0.3 seconds - with this patch\n\nSigned-off-by: Ben Peart \n---\n\nThe biggest change in this version is that I have added the logic to call\nshow_local_changes() on the optimized path. Since this can be expensive on\nlarge repos (see below) this behavior is turned off if the user sets the\n\"checkout.optimizeNewBranch\" setting.\n\n$ git checkout -b newbranch\nread-cache.c:2006 performance: 0.517875500 s: read cache .git/index\nname-hash.c:605 performance: 0.277765000 s: initialize name hash\npreload-index.c:111 performance: 0.019401300 s: preload index\ndiff-lib.c:527 performance: 3.807563700 s: diff-index\nSwitched to a new branch 'newbranch'\ntrace.c:420 performance: 5.044219600 s: git command: c:git checkout -b newbranch\n\n\n$ git checkout -b newbranch1\nSwitched to a new branch 'newbranch1'\ntrace.c:420 performance: 0.332873600 s: git command: c:git checkout -b newbranch111\n\nNotes:\n Base Ref: master\n Web-Diff: https://github.com/benpeart/git/commit/aebe02c966\n Checkout: git fetch https://github.com/benpeart/git checkout-b-v3 && git checkout aebe02c966\n \n ### Patches\n\n Documentation/config.txt | 8 +++\n builtin/checkout.c | 120 +++++++++++++++++++++++++++++--\n t/t1090-sparse-checkout-scope.sh | 14 ++++\n 3 files changed, 138 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex fd8d27e761..2298ecd753 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1135,6 +1135,14 @@ and by linkgit:git-worktree[1] when 'git worktree add' refers to a\n remote branch. This setting might be used for other checkout-like\n commands or functionality in the future.\n \n+checkout.optimizeNewBranch\n+\tOptimizes the performance of \"git checkout -b \" when\n+\tusing sparse-checkout. When set to true, git will not update the\n+\trepo based on the current sparse-checkout settings. This means it\n+\twill not update the skip-worktree bit in the index nor add/remove\n+\tfiles in the working directory to reflect the current sparse checkout\n+\tsettings nor will it show the local changes.\n+\n clean.requireForce::\n \tA boolean to make git-clean do nothing unless given -f,\n \t-i or -n. Defaults to true.\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex cb6bb76312..26b1a5053a 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -25,6 +25,8 @@\n #include \"submodule.h\"\n #include \"advice.h\"\n \n+static int checkout_optimize_new_branch;\n+\n static const char * const checkout_usage[] = {\n \tN_(\"git checkout [] \"),\n \tN_(\"git checkout [] [] -- ...\"),\n@@ -42,6 +44,10 @@ struct checkout_opts {\n \tint ignore_skipworktree;\n \tint ignore_other_worktrees;\n \tint show_progress;\n+\t/*\n+\t * If new checkout options are added, skip_merge_working_tree\n+\t * should be updated accordingly.\n+\t */\n \n \tconst char *new_branch;\n \tconst char *new_branch_force;\n@@ -472,6 +478,98 @@ static void setup_branch_path(struct branch_info *branch)\n \tbranch->path = strbuf_detach(&buf, NULL);\n }\n \n+/*\n+ * Skip merging the trees, updating the index and working directory if and\n+ * only if we are creating a new branch via \"git checkout -b .\"\n+ */\n+static int skip_merge_working_tree(const struct checkout_opts *opts,\n+\tconst struct branch_info *old_branch_info,\n+\tconst struct branch_info *new_branch_info)\n+{\n+\t/*\n+\t * Do the merge if sparse checkout is on and the user has not opted in\n+\t * to the optimized behavior\n+\t */\n+\tif (core_apply_sparse_checkout && !checkout_optimize_new_branch)\n+\t\treturn 0;\n+\n+\t/*\n+\t * We must do the merge if we are actually moving to a new commit.\n+\t */\n+\tif (!old_branch_info->commit || !new_branch_info->commit ||\n+\t\toidcmp(&old_branch_info->commit->object.oid, &new_branch_info->commit->object.oid))\n+\t\treturn 0;\n+\n+\t/*\n+\t * opts->patch_mode cannot be used with switching branches so is\n+\t * not tested here\n+\t */\n+\n+\t/*\n+\t * opts->quiet only impacts output so doesn't require a merge\n+\t */\n+\n+\t/*\n+\t * Honor the explicit request for a three-way merge or to throw away\n+\t * local changes\n+\t */\n+\tif (opts->merge || opts->force)\n+\t\treturn 0;\n+\n+\t/*\n+\t * --detach is documented as \"updating the index and the files in the\n+\t * working tree\" but this optimization skips those steps so fall through\n+\t * to the regular code path.\n+\t */\n+\tif (opts->force_detach)\n+\t\treturn 0;\n+\n+\t/*\n+\t * opts->writeout_stage cannot be used with switching branches so is\n+\t * not tested here\n+\t */\n+\n+\t/*\n+\t * Honor the explicit ignore requests\n+\t */\n+\tif (!opts->overwrite_ignore || opts->ignore_skipworktree ||\n+\t\topts->ignore_other_worktrees)\n+\t\treturn 0;\n+\n+\t/*\n+\t * opts->show_progress only impacts output so doesn't require a merge\n+\t */\n+\n+\t/*\n+\t * If we aren't creating a new branch any changes or updates will\n+\t * happen in the existing branch. Since that could only be updating\n+\t * the index and working directory, we don't want to skip those steps\n+\t * or we've defeated any purpose in running the command.\n+\t */\n+\tif (!opts->new_branch)\n+\t\treturn 0;\n+\n+\t/*\n+\t * new_branch_force is defined to \"create/reset and checkout a branch\"\n+\t * so needs to go through the merge to do the reset\n+\t */\n+\tif (opts->new_branch_force)\n+\t\treturn 0;\n+\n+\t/*\n+\t * A new orphaned branch requrires the index and the working tree to be\n+\t * adjusted to \n+\t */\n+\tif (opts->new_orphan_branch)\n+\t\treturn 0;\n+\n+\t/*\n+\t * Remaining variables are not checkout options but used to track state\n+\t */\n+\n+\treturn 1;\n+}\n+\n static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t struct branch_info *old_branch_info,\n \t\t\t struct branch_info *new_branch_info,\n@@ -846,10 +944,19 @@ static int switch_branches(const struct checkout_opts *opts,\n \t\tparse_commit_or_die(new_branch_info->commit);\n \t}\n \n-\tret = merge_working_tree(opts, &old_branch_info, new_branch_info, &writeout_error);\n-\tif (ret) {\n-\t\tfree(path_to_free);\n-\t\treturn ret;\n+\t/* optimize the \"checkout -b path */\n+\tif (skip_merge_working_tree(opts, &old_branch_info, new_branch_info)) {\n+\t\tif (!checkout_optimize_new_branch && !opts->quiet) {\n+\t\t\tif (read_cache_preload(NULL) < 0)\n+\t\t\t\treturn error(_(\"index file corrupt\"));\n+\t\t\tshow_local_changes(&new_branch_info->commit->object, &opts->diff_options);\n+\t\t}\n+\t} else {\n+\t\tret = merge_working_tree(opts, &old_branch_info, new_branch_info, &writeout_error);\n+\t\tif (ret) {\n+\t\t\tfree(path_to_free);\n+\t\t\treturn ret;\n+\t\t}\n \t}\n \n \tif (!opts->quiet && !old_branch_info.path && old_branch_info.commit && new_branch_info->commit != old_branch_info.commit)\n@@ -864,6 +971,11 @@ static int switch_branches(const struct checkout_opts *opts,\n \n static int git_checkout_config(const char *var, const char *value, void *cb)\n {\n+\tif (!strcmp(var, \"checkout.optimizenewbranch\")) {\n+\t\tcheckout_optimize_new_branch = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"diff.ignoresubmodules\")) {\n \t\tstruct checkout_opts *opts = cb;\n \t\thandle_ignore_submodules_arg(&opts->diff_options, value);\ndiff --git a/t/t1090-sparse-checkout-scope.sh b/t/t1090-sparse-checkout-scope.sh\nindex 1f61eb3e88..25d7c700f6 100755\n--- a/t/t1090-sparse-checkout-scope.sh\n+++ b/t/t1090-sparse-checkout-scope.sh\n@@ -31,6 +31,20 @@ test_expect_success 'perform sparse checkout of master' '\n \ttest_path_is_file c\n '\n \n+test_expect_success 'checkout -b checkout.optimizeNewBranch interaction' '\n+\tcp .git/info/sparse-checkout .git/info/sparse-checkout.bak &&\n+\ttest_when_finished \"\n+\t\tmv -f .git/info/sparse-checkout.bak .git/info/sparse-checkout\n+\t\tgit checkout master\n+\t\" &&\n+\techo \"/b\" >>.git/info/sparse-checkout &&\n+\ttest \"$(git ls-files -t b)\" = \"S b\" &&\n+\tgit -c checkout.optimizeNewBranch=true checkout -b fast &&\n+\ttest \"$(git ls-files -t b)\" = \"S b\" &&\n+\tgit checkout -b slow &&\n+\ttest \"$(git ls-files -t b)\" = \"H b\"\n+'\n+\n test_expect_success 'merge feature branch into sparse checkout of master' '\n \tgit merge feature &&\n \ttest_path_is_file a &&\n\nbase-commit: 63749b2dea5d1501ff85bab7b8a7f64911d21dea\n-- \n2.18.0.windows.1\n\n"},{"id":"355869","messageId":"CACsJy8B84T9HSJKAErNAyoMwbBpu6Gwgyz3RsvmqD0+o=bg3Ng@mail.gmail.com","threadId":"48948","inReplyTo":"20180816182653.15580-1-benpeart@microsoft.com","subject":"Re: [PATCH v3] checkout: optimize \"git checkout -b \"","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-16T18:37:40Z","receivedAt":"2018-08-16T18:38:09Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Aug 16, 2018 at 8:27 PM Ben Peart wrote:\n>\n> From: Ben Peart \n>\n> Skip merging the commit, updating the index and working directory if and\n> only if we are creating a new branch via \"git checkout -b .\"\n> Any other checkout options will still go through the former code path.\n>\n> If sparse_checkout is on, require the user to manually opt in to this\n> optimzed behavior by setting the config setting checkout.optimizeNewBranch\n> to true as we will no longer update the skip-worktree bit in the index, nor\n> add/remove files in the working directory to reflect the current sparse\n> checkout settings.\n>\n> For comparison, running \"git checkout -b \" on a large repo takes:\n>\n> 14.6 seconds - without this patch\n> 0.3 seconds - with this patch\n\nI still don't think we should do this. If you want lightning fast\nbranch creation, just use 'git branch'. From the timing breakdown you\nshown in the other thread it looks like sparse checkout still takes\nseconds, which could be optimized (or even excluded, I mentioned this\ntoo). And split index (or something similar if you can't use it) would\ngive you saving across the board. There is still one idea Elijah gave\nme that should further lower traverse_trees() cost.\n\nBut anyway, it's not my call. I'll stop here.\n-- \nDuy\n"},{"id":"355899","messageId":"448bd740-73fb-aa3a-ded0-e4012cf6ec21@gmail.com","threadId":"48948","inReplyTo":"CACsJy8B84T9HSJKAErNAyoMwbBpu6Gwgyz3RsvmqD0+o=bg3Ng@mail.gmail.com","subject":"Re: [PATCH v3] checkout: optimize \"git checkout -b \"","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-08-17T12:37:03Z","receivedAt":"2018-08-17T12:37:08Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 8/16/2018 2:37 PM, Duy Nguyen wrote:\n> On Thu, Aug 16, 2018 at 8:27 PM Ben Peart wrote:\n>>\n>> From: Ben Peart \n>>\n>> Skip merging the commit, updating the index and working directory if and\n>> only if we are creating a new branch via \"git checkout -b .\"\n>> Any other checkout options will still go through the former code path.\n>>\n>> If sparse_checkout is on, require the user to manually opt in to this\n>> optimzed behavior by setting the config setting checkout.optimizeNewBranch\n>> to true as we will no longer update the skip-worktree bit in the index, nor\n>> add/remove files in the working directory to reflect the current sparse\n>> checkout settings.\n>>\n>> For comparison, running \"git checkout -b \" on a large repo takes:\n>>\n>> 14.6 seconds - without this patch\n>> 0.3 seconds - with this patch\n> \n> I still don't think we should do this. If you want lightning fast\n> branch creation, just use 'git branch'. From the timing breakdown you\n> shown in the other thread it looks like sparse checkout still takes\n> seconds, which could be optimized (or even excluded, I mentioned this\n> too). And split index (or something similar if you can't use it) would\n> give you saving across the board. There is still one idea Elijah gave\n> me that should further lower traverse_trees() cost.\n> \n\nWe have investigated some of these already - split index ended up \nslowing things down more than it sped them up do to the higher compute \ncosts. Sparse checkout we've already optimized significantly - limiting \nthe patterns we accept so that we can do the lookup via a hashmap \ninstead of the robust pattern matching. We will continue to look for \nother optimizations and appreciate any and all ideas!\n\nIn the end, this optimization makes a huge performance improvement by \navoiding doing a lot of work that isn't necessary. Taking a command \nfrom 14+ seconds to sub-second is just too much of a win for us to ignore.\n\n> But anyway, it's not my call. I'll stop here.\n> \n"},{"id":"356022","messageId":"CABPp-BGir_5xyqEfwytDog0rZDydPHXjuqXCpNKk67dVPXjUjA@mail.gmail.com","threadId":"48948","inReplyTo":"448bd740-73fb-aa3a-ded0-e4012cf6ec21@gmail.com","subject":"Re: [PATCH v3] checkout: optimize \"git checkout -b \"","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-19T01:44:54Z","receivedAt":"2018-08-19T01:45:08Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Aug 17, 2018 at 5:41 AM Ben Peart wrote:\n> On 8/16/2018 2:37 PM, Duy Nguyen wrote:\n> > On Thu, Aug 16, 2018 at 8:27 PM Ben Peart wrote:\n> >>\n> >> From: Ben Peart \n> >>\n> >> Skip merging the commit, updating the index and working directory if and\n> >> only if we are creating a new branch via \"git checkout -b .\"\n> >> Any other checkout options will still go through the former code path.\n> >>\n> >> If sparse_checkout is on, require the user to manually opt in to this\n> >> optimzed behavior by setting the config setting checkout.optimizeNewBranch\n> >> to true as we will no longer update the skip-worktree bit in the index, nor\n> >> add/remove files in the working directory to reflect the current sparse\n> >> checkout settings.\n> >>\n> >> For comparison, running \"git checkout -b \" on a large repo takes:\n> >>\n> >> 14.6 seconds - without this patch\n> >> 0.3 seconds - with this patch\n> >\n> > I still don't think we should do this. If you want lightning fast\n> > branch creation, just use 'git branch'. From the timing breakdown you\n> > shown in the other thread it looks like sparse checkout still takes\n> > seconds, which could be optimized (or even excluded, I mentioned this\n> > too). And split index (or something similar if you can't use it) would\n> > give you saving across the board. There is still one idea Elijah gave\n> > me that should further lower traverse_trees() cost.\n> >\n>\n> We have investigated some of these already - split index ended up\n> slowing things down more than it sped them up do to the higher compute\n> costs. Sparse checkout we've already optimized significantly - limiting\n> the patterns we accept so that we can do the lookup via a hashmap\n> instead of the robust pattern matching. We will continue to look for\n> other optimizations and appreciate any and all ideas!\n>\n> In the end, this optimization makes a huge performance improvement by\n> avoiding doing a lot of work that isn't necessary. Taking a command\n> from 14+ seconds to sub-second is just too much of a win for us to ignore.\n>\n> > But anyway, it's not my call. I'll stop here.\n\nIt's even less of my call, but since things seem to be stuck in\nwhat-should-we-do state (as per Junio's comments on this patch in the\nlast two \"What's cooking\" emails), and since Ben and Duy obviously\nhave opposite opinions on Ben's patch, let's see if I might be able to\nhelp at all. Here's my summary and my findings:\n\n== The pain ==\n- For repositories with a really large number of entries (500K as Ben\nsays), some operations are much slower than it feels like they should\nbe.\n- This does not seem to be GFVS-specific in any way, I can duplicate\nslowness with a simple git-bomb[1]-like repo that has a sparse\ncheckout pattern ignoring the \"bomb\" side. (It has 1M+1 entries in\nthe index, and .git/info/sparse-checkout ignores the 1M so the working\ncopy only has 1 entry). The timings on my repo for \"git checkout -b\n$NEWBRANCH\" are almost exactly double what Ben reports he gets on\ntheir repo.\n\n[1] https://kate.io/blog/making-your-own-exploding-git-repos/\n\n== Short term solutions ==\n- Alternative git commands exist today to do a fast checkout of a new\nbranch in a huge repo. I also get sub-second timings in my\neven-bigger repo with this:\n git branch $NEWBRANCH && git symbolic-ref HEAD $NEWBRANCH\nBut I do understand that wrapping this into a script or executable\n(git-fast-new-branch?) and asking users to use it is a usability\nproblem and an uphill battle. (Sidenote: this isn't quite the same\noperation; it's missing a reflog update. The -m option to\nsymbolic-ref doesn't seem to help; I guess the fact that HEAD's\nsha1sum is not changing is viewed as not an update? However, that\ncould also be scripted around.)\n- Ben's patch successfully drops the time for \"git checkout -b\n$NEWBRANCH\" from 26+ seconds (in my cooked-up testcase) to sub-second\n(in fact, under .1 seconds for me). That's a _huge_ win.\n\n== unpack_trees optimization notes ==\n- Ben's patch is extremely focused. It only affects \"git checkout -b\n$NEWBRANCH\". If someone runs \"git branch $NEWBRANCH && git checkout\n$NEWBRANCH\", they get the old 26+ second timing. They also suddenly\nget the really long timing if they add any other flags or checkout a\ncommit that differs in only a single entry in the entire tree. It\nwould be nice if we did general optimization for all issues rather\nthan just special casing such narrow cases.\n- However, optimizing unpack_trees is hard. It's really easy to get\nlost trying to look at the code. Time has been spent trying to\noptimizing it. Ben really likes the speedup factors of 2-3 that Duy\nhas produced. But he's pessimistic we'll find enough to bridge the\ngap for this case. And he's worried about breaking unrelated stuff\ndue to the complexity of unpack_trees.\n- Duy is pretty sure we can optimize unpack_trees in at least one more\nway. I've tried looking through the code and think there are others,\nbut then again I'm known to get lost and confused in unpack_trees.\n\n== The patch ==\n- Ben's patch only affects the \"checkout -b $NEWBRANCH\" case. He\nchecks for it by looking for any other flag that would be a different\ncase, and using the old codepath if he finds any.\n- This means there is a \"giant list of checks\" for this optimization,\nand an ongoing maintenance burden because if anyone ever adds any\nextra options, this optimization might suddenly break things if that\ngiant list of checks isn't updated. Ben even added a comment to the\ncode hoping to help alert others who come along and add extra options:\n+ /*\n+ * If new checkout options are added, skip_merge_working_tree\n+ * should be updated accordingly.\n+ */\n\n== Other notes ==\n- In my cooked-up testcase, I also noticed that things like git add or\ngit status were slow in a repo with lots of index entries. There may\nbe several other areas in need of performance boosts too.\n\n\n== My opinions ==\n- The performance wins are big enough that I can see why Ben is pushing this.\n- I totally see Duy's point that more general optimizations would be\nreally nice.\n- I really dislike the \"giant list of checks\" and the ongoing\nmaintenance burden it implies.\n\nOverall, I have to side with Duy and say I don't think we should take\nthe patch as-is. Since that'll be frustrating for Ben to hear, I've\ntried to come up with some additional alternatives:\n\n== Alternatives (both short and long term) ==\n- Use the 'git branch $NEWBRANCH && git symbolic-ref HEAD $NEWBRANCH'\ntrick. It's essentially just as fast (well, you exec git twice so\nit's slightly slower, but it's close). However, there's a difficult\nget-it-to-the-users hurdle.\n- Rewrite this patch so it instead does a very small set of checks at\nthe beginning of cmd_checkout(); e.g. check if argc == 3 and argv[1]\n== \"-b\" and if so then perform the minimum operations needed to create\nand checkout the new branch (maybe even calling in to cmd_branch() and\ncmd_symbolic_ref() and calling some reflog update function). Preface\nit with a comment that it's a performance hack that might eventually\nbe able to go away.\n- Look into the performance optimization(s) Duy mentioned.\n- Other performance optimizations in unpack_trees(); it seems we have\nan awful lot of loops over all cache entries, and it sure seems to me\nlike some of them could somehow be focused on just the things that are\nchanging instead of again checking everything. Or maybe I'm wrong and\nthey're all needed.\n- Possibly crazy idea for massive global performance win: modify how\nthe cache works so that it can have tree entries instead of just file\nentries; make use of that with sparse checkouts (and partial clones?)\nso that the cache only stores a tree when that entire tree is\n\"unwanted\". Suddenly, git status, git add, git checkout, etc., etc.\nare all much faster. merge needs some special work to operate,\nthough.\n\n\nI'm potentially interested in eventually looking into and/or helping\nwith optimization work in this area. I might even get more time to\nwork on git at $DAYJOB since sparse-checkouts/partial-clone/gvfs/etc.\nall are looking more and more interesting as we have large repos that\nare growing quickly (particularly as we consolidate many repos down to\nfewer). But they've said that such work won't take priority for\nseveral months down the road for them, it's not yet clear how much\nthey'd sponsor vs. just want to see what existed and how we can use\nit, and I'd kind of like to clear a few other things off my plate\nfirst too (such as the merge-recursive rewrite).\n\nAnyway, that's just my $0.02. I hope something I said helps.\n\nElijah\n"},{"id":"356086","messageId":"f88299c5-1697-a7ef-87d6-69f5c19d5a68@gmail.com","threadId":"48948","inReplyTo":"CABPp-BGir_5xyqEfwytDog0rZDydPHXjuqXCpNKk67dVPXjUjA@mail.gmail.com","subject":"Re: [PATCH v3] checkout: optimize \"git checkout -b \"","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-08-20T13:40:12Z","receivedAt":"2018-08-20T13:40:17Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 8/18/2018 9:44 PM, Elijah Newren wrote:\n> On Fri, Aug 17, 2018 at 5:41 AM Ben Peart wrote:\n>> On 8/16/2018 2:37 PM, Duy Nguyen wrote:\n>>> On Thu, Aug 16, 2018 at 8:27 PM Ben Peart wrote:\n>>>>\n>>>> From: Ben Peart \n>>>>\n>>>> Skip merging the commit, updating the index and working directory if and\n>>>> only if we are creating a new branch via \"git checkout -b .\"\n>>>> Any other checkout options will still go through the former code path.\n>>>>\n>>>> If sparse_checkout is on, require the user to manually opt in to this\n>>>> optimzed behavior by setting the config setting checkout.optimizeNewBranch\n>>>> to true as we will no longer update the skip-worktree bit in the index, nor\n>>>> add/remove files in the working directory to reflect the current sparse\n>>>> checkout settings.\n>>>>\n>>>> For comparison, running \"git checkout -b \" on a large repo takes:\n>>>>\n>>>> 14.6 seconds - without this patch\n>>>> 0.3 seconds - with this patch\n>>>\n>>> I still don't think we should do this. If you want lightning fast\n>>> branch creation, just use 'git branch'. From the timing breakdown you\n>>> shown in the other thread it looks like sparse checkout still takes\n>>> seconds, which could be optimized (or even excluded, I mentioned this\n>>> too). And split index (or something similar if you can't use it) would\n>>> give you saving across the board. There is still one idea Elijah gave\n>>> me that should further lower traverse_trees() cost.\n>>>\n>>\n>> We have investigated some of these already - split index ended up\n>> slowing things down more than it sped them up do to the higher compute\n>> costs. Sparse checkout we've already optimized significantly - limiting\n>> the patterns we accept so that we can do the lookup via a hashmap\n>> instead of the robust pattern matching. We will continue to look for\n>> other optimizations and appreciate any and all ideas!\n>>\n>> In the end, this optimization makes a huge performance improvement by\n>> avoiding doing a lot of work that isn't necessary. Taking a command\n>> from 14+ seconds to sub-second is just too much of a win for us to ignore.\n>>\n>>> But anyway, it's not my call. I'll stop here.\n> \n> It's even less of my call, but since things seem to be stuck in\n> what-should-we-do state (as per Junio's comments on this patch in the\n> last two \"What's cooking\" emails), and since Ben and Duy obviously\n> have opposite opinions on Ben's patch, let's see if I might be able to\n> help at all. Here's my summary and my findings:\n> \n> == The pain ==\n> - For repositories with a really large number of entries (500K as Ben\n> says), some operations are much slower than it feels like they should\n> be.\n> - This does not seem to be GFVS-specific in any way, I can duplicate\n> slowness with a simple git-bomb[1]-like repo that has a sparse\n> checkout pattern ignoring the \"bomb\" side. (It has 1M+1 entries in\n> the index, and .git/info/sparse-checkout ignores the 1M so the working\n> copy only has 1 entry). The timings on my repo for \"git checkout -b\n> $NEWBRANCH\" are almost exactly double what Ben reports he gets on\n> their repo.\n> \n> [1] https://kate.io/blog/making-your-own-exploding-git-repos/\n> \n> == Short term solutions ==\n> - Alternative git commands exist today to do a fast checkout of a new\n> branch in a huge repo. I also get sub-second timings in my\n> even-bigger repo with this:\n> git branch $NEWBRANCH && git symbolic-ref HEAD $NEWBRANCH\n> But I do understand that wrapping this into a script or executable\n> (git-fast-new-branch?) and asking users to use it is a usability\n> problem and an uphill battle. (Sidenote: this isn't quite the same\n> operation; it's missing a reflog update. The -m option to\n> symbolic-ref doesn't seem to help; I guess the fact that HEAD's\n> sha1sum is not changing is viewed as not an update? However, that\n> could also be scripted around.)\n> - Ben's patch successfully drops the time for \"git checkout -b\n> $NEWBRANCH\" from 26+ seconds (in my cooked-up testcase) to sub-second\n> (in fact, under .1 seconds for me). That's a _huge_ win.\n> \n> == unpack_trees optimization notes ==\n> - Ben's patch is extremely focused. It only affects \"git checkout -b\n> $NEWBRANCH\". If someone runs \"git branch $NEWBRANCH && git checkout\n> $NEWBRANCH\", they get the old 26+ second timing. They also suddenly\n> get the really long timing if they add any other flags or checkout a\n> commit that differs in only a single entry in the entire tree. It\n> would be nice if we did general optimization for all issues rather\n> than just special casing such narrow cases.\n> - However, optimizing unpack_trees is hard. It's really easy to get\n> lost trying to look at the code. Time has been spent trying to\n> optimizing it. Ben really likes the speedup factors of 2-3 that Duy\n> has produced. But he's pessimistic we'll find enough to bridge the\n> gap for this case. And he's worried about breaking unrelated stuff\n> due to the complexity of unpack_trees.\n> - Duy is pretty sure we can optimize unpack_trees in at least one more\n> way. I've tried looking through the code and think there are others,\n> but then again I'm known to get lost and confused in unpack_trees.\n> \n> == The patch ==\n> - Ben's patch only affects the \"checkout -b $NEWBRANCH\" case. He\n> checks for it by looking for any other flag that would be a different\n> case, and using the old codepath if he finds any.\n> - This means there is a \"giant list of checks\" for this optimization,\n> and an ongoing maintenance burden because if anyone ever adds any\n> extra options, this optimization might suddenly break things if that\n> giant list of checks isn't updated. Ben even added a comment to the\n> code hoping to help alert others who come along and add extra options:\n> + /*\n> + * If new checkout options are added, skip_merge_working_tree\n> + * should be updated accordingly.\n> + */\n> \n> == Other notes ==\n> - In my cooked-up testcase, I also noticed that things like git add or\n> git status were slow in a repo with lots of index entries. There may\n> be several other areas in need of performance boosts too.\n> \n> \n> == My opinions ==\n> - The performance wins are big enough that I can see why Ben is pushing this.\n> - I totally see Duy's point that more general optimizations would be\n> really nice.\n> - I really dislike the \"giant list of checks\" and the ongoing\n> maintenance burden it implies.\n> \n> Overall, I have to side with Duy and say I don't think we should take\n> the patch as-is. Since that'll be frustrating for Ben to hear, I've\n> tried to come up with some additional alternatives:\n\nThank you for the thorough review and summary. From my perspective, it \nis both fair and accurate.\n\nThis is a performance improvement we've been using for nearly 2 years \n[1] because the win is so significant in our case, it makes living with \nthe maintenance burden worth while. That said, in the past 2 years, \nthere haven't been any new options added to the checkout command that \nrequired any maintenance but the possibility clearly does exist.\n\nMy purpose in pushing this patch to the mailing list is to improve git \nin general and to let others benefit from the performance improvement \nwe've been enjoying. If the community doesn't feel like the risk/reward \nis worth it, then it shouldn't be accepted and I'm perfectly fine with that.\n\n[1] \nhttps://github.com/git-for-windows/git/commit/0a64b9ae6df02d4df3e41227c43c4217a1a90bfa\n\n> \n> == Alternatives (both short and long term) ==\n> - Use the 'git branch $NEWBRANCH && git symbolic-ref HEAD $NEWBRANCH'\n> trick. It's essentially just as fast (well, you exec git twice so\n> it's slightly slower, but it's close). However, there's a difficult\n> get-it-to-the-users hurdle.\n> - Rewrite this patch so it instead does a very small set of checks at\n> the beginning of cmd_checkout(); e.g. check if argc == 3 and argv[1]\n> == \"-b\" and if so then perform the minimum operations needed to create\n> and checkout the new branch (maybe even calling in to cmd_branch() and\n> cmd_symbolic_ref() and calling some reflog update function). Preface\n> it with a comment that it's a performance hack that might eventually\n> be able to go away.\n\nI'm happy to do this if it would make the patch better/more acceptable \nto the community.\n\nOne other change I could do would be to address Duy's concern about the \nconfig option would be to remove that entirely. That means those using \nsparse-checkout don't have the option of getting the performance win but \nthe benefit is that there isn't any documentation or behavior changes \nthat need to be made - things just get a lot faster if you happen to use \nthe (rather common) \"checkout -b\" option.\n\nIf making these two changes would resolve the remaining concerns, I'm \nhappy to make them and send another patch series. If it will just \nresult in another round of \"I'm still not comfortable with this\" then \nI'd rather spend my time elsewhere. :)\n\n> - Look into the performance optimization(s) Duy mentioned.\n> - Other performance optimizations in unpack_trees(); it seems we have\n> an awful lot of loops over all cache entries, and it sure seems to me\n> like some of them could somehow be focused on just the things that are\n> changing instead of again checking everything. Or maybe I'm wrong and\n> they're all needed.\n\nI am absolutely in favor of any performance optimizations that can be \nmade in unpack_trees(). I am not very familiar with that code and only \nrecently started digging in when I started my recent RFC/patch series to \nmulti-thread it [2]. I'm extremely grateful for all the ideas and work \nthat thread has triggered - to me, it's an example of open source \nsoftware development at it's finest.\n\nI'd never even thought of using the cache-tree to simplify the traversal \nin that way and am grateful others have and especially to Duy who backed \nit up with a great patch series (I started the work but he was a lot \nfaster at getting working patches than I was!). In my opinion, this was \na much better solution than the complexity of multi-threading this \ncomplex piece of code (though that remains an option if it is still needed).\n\n[2] \nhttps://public-inbox.org/git/20180718204458.20936-1-benpeart@microsoft.com/\n\n> - Possibly crazy idea for massive global performance win: modify how\n> the cache works so that it can have tree entries instead of just file\n> entries; make use of that with sparse checkouts (and partial clones?)\n> so that the cache only stores a tree when that entire tree is\n> \"unwanted\". Suddenly, git status, git add, git checkout, etc., etc.\n> are all much faster. merge needs some special work to operate,\n> though.\n> \n\nNot crazy at all! It's something we've investigated and discussed \nseveral times over the past couple of years. This is the largest \npotential optimization that I am aware of as it has the potential to \ndramatically speed up most git operations as they become O(what I care \nabout) vs O(size of repo/tree) - especially when paired with partial clone.\n\nThe downside is pretty obvious, pulling this off means rewriting an \nawful lot of the code! The index is central to so much of git and the \nassumption is everywhere that the index is simply a (global) array of \ncache entries to be looped through and manipulated at will.\n\nThere would be a lot of refactoring work, creating a new index \nabstraction API, updating the code to utilize it, then updating the \ninternal representation of the index to optimize it around the concept \nof being sparse/partial. Likely, we'd want/need a new on disk format \nthat would support the new structures in an optimal way. Finally, the \nvarious algorithms (like unpack_trees()) could be rewritten to take \nadvantage of the sparseness.\n\nNot something that can be taken on lightly - but certainly something \nthat has huge potential to enable git to scale to much larger repos.\n\n> \n> I'm potentially interested in eventually looking into and/or helping\n> with optimization work in this area. I might even get more time to\n> work on git at $DAYJOB since sparse-checkouts/partial-clone/gvfs/etc.\n> all are looking more and more interesting as we have large repos that\n> are growing quickly (particularly as we consolidate many repos down to\n> fewer). But they've said that such work won't take priority for\n> several months down the road for them, it's not yet clear how much\n> they'd sponsor vs. just want to see what existed and how we can use\n> it, and I'd kind of like to clear a few other things off my plate\n> first too (such as the merge-recursive rewrite).\n> \n> Anyway, that's just my $0.02. I hope something I said helps.\n> \n> Elijah\n> \n"},{"id":"356111","messageId":"CABPp-BFKf2N6TYzCCneRwWUektMzRMnHLZ8JT64q=MGj5WQZkA@mail.gmail.com","threadId":"48948","inReplyTo":"f88299c5-1697-a7ef-87d6-69f5c19d5a68@gmail.com","subject":"Re: [PATCH v3] checkout: optimize \"git checkout -b \"","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-20T18:16:39Z","receivedAt":"2018-08-20T18:16:54Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Aug 20, 2018 at 6:40 AM Ben Peart wrote:\n> On 8/18/2018 9:44 PM, Elijah Newren wrote:\n...\n> > == My opinions ==\n> > - The performance wins are big enough that I can see why Ben is pushing this.\n> > - I totally see Duy's point that more general optimizations would be\n> > really nice.\n> > - I really dislike the \"giant list of checks\" and the ongoing\n> > maintenance burden it implies.\n> >\n> > Overall, I have to side with Duy and say I don't think we should take\n> > the patch as-is. Since that'll be frustrating for Ben to hear, I've\n> > tried to come up with some additional alternatives:\n>\n> Thank you for the thorough review and summary. From my perspective, it\n> is both fair and accurate.\n>\n...\n> > - Rewrite this patch so it instead does a very small set of checks at\n> > the beginning of cmd_checkout(); e.g. check if argc == 3 and argv[1]\n> > == \"-b\" and if so then perform the minimum operations needed to create\n> > and checkout the new branch (maybe even calling in to cmd_branch() and\n> > cmd_symbolic_ref() and calling some reflog update function). Preface\n> > it with a comment that it's a performance hack that might eventually\n> > be able to go away.\n>\n> I'm happy to do this if it would make the patch better/more acceptable\n> to the community.\n\nFor whatever it's worth, it'd change my vote. While I would like\ngeneral optimizations more, I do think that's overall a longer term\nprospect, and the wins here are big enough to justify a performance\nhack -- if the amount of new code and added maintenance overhead is\nminimized. I think that checking for the args matching checkout -b\nrather than checking all flags that might imply something other than\nargs being checkout -b would be a good way to help minimize both.\n\n> One other change I could do would be to address Duy's concern about the\n> config option would be to remove that entirely. That means those using\n> sparse-checkout don't have the option of getting the performance win but\n> the benefit is that there isn't any documentation or behavior changes\n> that need to be made - things just get a lot faster if you happen to use\n> the (rather common) \"checkout -b\" option.\n\nPlaying with sparse-checkout, it feels to me like a half-baked\nfeature. It seems like it required too much manual work, and it was\nsometimes hard to tell if I was misunderstanding configuration rules\nor I was just running into bugs in the code. I think I hit both but I\ndidn't really want to get side-tracked further, yet. (I do want to\neventually come back to it.) The only reason someone would go through\nthat pain is if it provided massive performance benefits.\n\nViewing things when-in-doubt through the lens of strict backwards\ncompatibility may inadvertently mean enforcing buggy and/or slow\nbehavior with sparse-checkouts. So, while git usually focuses\nstrongly on backwards compatibility (which is a good thing), that\nmight actually be wrong in the specific case of the sparse-checkout\nfeature. In particular, since the whole point of the feature is\nessentially a performance hack, if the initial implementation has ways\nin which it really hurts performance, it seems like it'd be more in\nkeeping with the point of the feature to take performance fixes and\nswallow subtle behavioral changes than to require special flags to get\ndecent performance.\n\nThat might sound like a long winded way of saying I'm totally in favor\nof dropping the config option in this case, but I'm also trying to\nbuild a case that I think there will be other situations where we want\nto subtly and maybe even not-so-subtly change behavior of\nsparse-checkouts to make them both more performant and far nicer for\nend users. I'm not sure others will agree with me on this argument,\nbut sparse checkouts felt painful to me and I think we need something\nbetter.\n\n> If making these two changes would resolve the remaining concerns, I'm\n> happy to make them and send another patch series. If it will just\n> result in another round of \"I'm still not comfortable with this\" then\n> I'd rather spend my time elsewhere. :)\n\nThat's fair. The first change would be enough to resolve the concern\nfor me if others strongly push back on the second\n(config-option-removal) change, but I'd definitely prefer both changes\n-- fewer options means less code, less ongoing maintenance work, and\nless manual setup needed by users.\n\n> > - Possibly crazy idea for massive global performance win: modify how\n> > the cache works so that it can have tree entries instead of just file\n> > entries; make use of that with sparse checkouts (and partial clones?)\n> > so that the cache only stores a tree when that entire tree is\n> > \"unwanted\". Suddenly, git status, git add, git checkout, etc., etc.\n> > are all much faster. merge needs some special work to operate,\n> > though.\n>\n> Not crazy at all! It's something we've investigated and discussed\n> several times over the past couple of years. This is the largest\n> potential optimization that I am aware of as it has the potential to\n> dramatically speed up most git operations as they become O(what I care\n> about) vs O(size of repo/tree) - especially when paired with partial clone.\n>\n> The downside is pretty obvious, pulling this off means rewriting an\n> awful lot of the code! The index is central to so much of git and the\n> assumption is everywhere that the index is simply a (global) array of\n> cache entries to be looped through and manipulated at will.\n>\n> There would be a lot of refactoring work, creating a new index\n> abstraction API, updating the code to utilize it, then updating the\n> internal representation of the index to optimize it around the concept\n> of being sparse/partial. Likely, we'd want/need a new on disk format\n> that would support the new structures in an optimal way. Finally, the\n> various algorithms (like unpack_trees()) could be rewritten to take\n> advantage of the sparseness.\n>\n> Not something that can be taken on lightly - but certainly something\n> that has huge potential to enable git to scale to much larger repos.\n\nWell, the amount of work involved might qualify it as a \"crazy\" idea.\n:-) But, I'm glad others think it sounds like a useful path to purse.\nNot only would it be a tremendous help performance-wise, it might also\nbe beneficial for another actually-crazy feature idea of mine.\n"},{"id":"356122","messageId":"xmqqh8joc2k1.fsf@gitster-ct.c.googlers.com","threadId":"48948","inReplyTo":"CABPp-BGir_5xyqEfwytDog0rZDydPHXjuqXCpNKk67dVPXjUjA@mail.gmail.com","subject":"Re: [PATCH v3] checkout: optimize \"git checkout -b \"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-20T18:31:42Z","receivedAt":"2018-08-20T18:31:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> == The patch ==\n> - Ben's patch only affects the \"checkout -b $NEWBRANCH\" case. He\n> checks for it by looking for any other flag that would be a different\n> case, and using the old codepath if he finds any.\n> - This means there is a \"giant list of checks\" for this optimization,\n> and an ongoing maintenance burden because if anyone ever adds any\n> extra options, this optimization might suddenly break things if that\n> giant list of checks isn't updated.\n\nCorrect. \n\nHaving said that, I do not mind leaving it in 'pu' for the rest of\nthe cycle, possibly merging it to 'next' after that and keeping it\nthere, to see how horrible a maintenance burden it would become in\nreal life, though.\n\n"},{"id":"356173","messageId":"CACsJy8ArUXD0cF2vQAVnzM_AGto2k2yQTFuTO7PhP4ffHM8dVQ@mail.gmail.com","threadId":"48948","inReplyTo":"CABPp-BFKf2N6TYzCCneRwWUektMzRMnHLZ8JT64q=MGj5WQZkA@mail.gmail.com","subject":"Re: [PATCH v3] checkout: optimize \"git checkout -b \"","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-21T14:51:49Z","receivedAt":"2018-08-21T14:52:18Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Aug 20, 2018 at 8:16 PM Elijah Newren wrote:\n> Playing with sparse-checkout, it feels to me like a half-baked\n> feature. It seems like it required too much manual work, and it was\n> sometimes hard to tell if I was misunderstanding configuration rules\n> or I was just running into bugs in the code. I think I hit both but I\n> didn't really want to get side-tracked further, yet. (I do want to\n> eventually come back to it.) The only reason someone would go through\n> that pain is if it provided massive performance benefits.\n\nIn my defense it was one of my first contribution when I was naiver\nand basically an evolution of \"git update-index --assume-unchanged\". I\nhave something in the queue to improve/complement sparse-checkout but\nmy last update on that branch was 2.5 years ago, so it's not coming\nsoon.\n\nI'd love to year how sparse checkout could be improved, or even\nreplaced. I think we still have to have some configuration rules, and\nyes the flexibility of sparse checkout (or gitignore to be precise)\nrules is a double-edged sword.\n-- \nDuy\n"},{"id":"356992","messageId":"CABPp-BGE-m_UFfUt_moXG-YR=ZW8hMzMwraD7fkFV-+sEHw36w@mail.gmail.com","threadId":"48948","inReplyTo":"CACsJy8ArUXD0cF2vQAVnzM_AGto2k2yQTFuTO7PhP4ffHM8dVQ@mail.gmail.com","subject":"Re: [PATCH v3] checkout: optimize \"git checkout -b \"","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-30T17:22:12Z","receivedAt":"2018-08-30T17:22:27Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Duy,\n\nOn Tue, Aug 21, 2018 at 7:52 AM Duy Nguyen wrote:\n>\n> On Mon, Aug 20, 2018 at 8:16 PM Elijah Newren wrote:\n> > Playing with sparse-checkout, it feels to me like a half-baked\n> > feature. It seems like it required too much manual work, and it was\n> > sometimes hard to tell if I was misunderstanding configuration rules\n> > or I was just running into bugs in the code. I think I hit both but I\n> > didn't really want to get side-tracked further, yet. (I do want to\n> > eventually come back to it.) The only reason someone would go through\n> > that pain is if it provided massive performance benefits.\n>\n> In my defense it was one of my first contribution when I was naiver\n> and basically an evolution of \"git update-index --assume-unchanged\". I\n> have something in the queue to improve/complement sparse-checkout but\n> my last update on that branch was 2.5 years ago, so it's not coming\n> soon.\n>\n> I'd love to year how sparse checkout could be improved, or even\n> replaced. I think we still have to have some configuration rules, and\n> yes the flexibility of sparse checkout (or gitignore to be precise)\n> rules is a double-edged sword.\n\nSorry for taking a while to respond, and if what I said came across\nharshly. I agree that the flexibility of the rules makes it more\ncomplicated, though I think a bigger issue may be that the feature is\nhard to make smooth unless coupled to something like partial clones.\nWork on that is ongoing. Anyway, in an attempt to be helpful, here\nwere some of the pain points I ran across:\n\n- The fact that documentation could only be found in a low-level\nplumbing command like read-tree made discoverability hard. Why would\nfolks think to look there? (I can't remember if I had to google it or\njust grepped around the git source code to find it.)\n\n- Needing to use read-tree, which isn't something most users are\nfamiliar with, makes for a learning curve. I may know what some of\nthe flags in read-tree do, but users will puzzle over which things on\nthe page happen to be relevant to them -- especially since the section\non sparse checkouts don't specify how read-tree should be invoked\nafter .git/info/sparse-checkout is populated. Even I couldn't guess\nwhat I was supposed to ran and just googled for hints. Here's some\npossible failures, as users guess which flags to pass:\n\n$ git read-tree\nwarning: read-tree: emptying the index with no arguments is\ndeprecated; use --empty\n\n$ git read-tree HEAD # Oops, doesn't update the working directory\n\n$ git read-tree -u HEAD # Doesn't do any updates either\n\n$ git read-tree -mu HEAD # Works...but make user think \"Why/what am I\nmerging?!?\"\n\n- I actually misunderstood or misread the documentation about undoing\nsparse checkouts and failed multiple times. I think I nuked the index\nthen did a 'git reset --hard HEAD'. Re-reading, it looks like you did\nexplain it, and I don't remember why/how I missed it the first time\naround, but I did.\n\n- I either failed to grasp how to specify negative refs or botched\nsomething else up. I tried digging for a while to figure out how to\nexclude my massive directory, but was always met with:\n error: Sparse checkout leaves no entry on working directory\nI spent a while trying to figure out what I did wrong, but gave up\nafter half an hour or so since I wasn't trying to use the feature for\nreal and just specified the files I wanted to keep instead.\nRe-reading the docs, it looks like you specified how to do this, and\nre-trying now it works, but I repeatedly passed over the '/*' in the\ndocs and read it as either bad formatting or a highlight of the next\nline rather than as important literal text. So, maybe part of my\nproblem is that I just can't read. :-)\n"},{"id":"357304","messageId":"CACsJy8CrRQ1sxh2rWowGCT5+mBDFJORbrsA6aYi6YxVwvYNV6g@mail.gmail.com","threadId":"48948","inReplyTo":"CABPp-BGE-m_UFfUt_moXG-YR=ZW8hMzMwraD7fkFV-+sEHw36w@mail.gmail.com","subject":"Re: [PATCH v3] checkout: optimize \"git checkout -b \"","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-09-04T16:46:13Z","receivedAt":"2018-09-04T16:46:42Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Aug 30, 2018 at 7:22 PM Elijah Newren wrote:\n>\n> Hi Duy,\n>\n> On Tue, Aug 21, 2018 at 7:52 AM Duy Nguyen wrote:\n> >\n> > On Mon, Aug 20, 2018 at 8:16 PM Elijah Newren wrote:\n> > > Playing with sparse-checkout, it feels to me like a half-baked\n> > > feature. It seems like it required too much manual work, and it was\n> > > sometimes hard to tell if I was misunderstanding configuration rules\n> > > or I was just running into bugs in the code. I think I hit both but I\n> > > didn't really want to get side-tracked further, yet. (I do want to\n> > > eventually come back to it.) The only reason someone would go through\n> > > that pain is if it provided massive performance benefits.\n> >\n> > In my defense it was one of my first contribution when I was naiver\n> > and basically an evolution of \"git update-index --assume-unchanged\". I\n> > have something in the queue to improve/complement sparse-checkout but\n> > my last update on that branch was 2.5 years ago, so it's not coming\n> > soon.\n> >\n> > I'd love to year how sparse checkout could be improved, or even\n> > replaced. I think we still have to have some configuration rules, and\n> > yes the flexibility of sparse checkout (or gitignore to be precise)\n> > rules is a double-edged sword.\n>\n> Sorry for taking a while to respond, and if what I said came across\n> harshly. I agree that the flexibility of the rules makes it more\n> complicated, though I think a bigger issue may be that the feature is\n> hard to make smooth unless coupled to something like partial clones.\n\nFor something like partial clones, we would need something like\npartial indexes. That is, the index does not record all paths in\nworktree. The problem is at write-tree time, how to create trees with\nsuch a partial index. So far the only option I see is record\ndirectories in the index (in the same way we record submodule's\ncommit), which reduces the index and we are still able to create trees\nfrom it.\n\n> Work on that is ongoing. Anyway, in an attempt to be helpful, here\n> were some of the pain points I ran across:\n>\n> ..\n\nThanks. I think this basically boils down to no good UI (and user\nfacing command also gives us a man page to describe stuff instead of\nhiding everything behind git-read-tree.txt). This is something I've\nbeen meaning to do but never got around to (also a couple of sparse\ncheckout optimizations). There's also \"git sparse-checkout\" [1] that\nwould be a good starting point for this, but I don't think the author\ngot to the point to submit it to git.\n\n[1] https://github.com/kjp/git-tools/blob/master/git-sparse-checkout\n-- \nDuy\n"},{"id":"358385","messageId":"20180918053449.14047-1-avarab@gmail.com","threadId":"48948","inReplyTo":"20180816182653.15580-1-benpeart@microsoft.com","subject":"[PATCH] config doc: add missing list separator for checkout.optimizeNewBranch","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-18T05:34:49Z","receivedAt":"2018-09-18T05:35:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The documentation added in fa655d8411 (\"checkout: optimize \"git\ncheckout -b \"\", 2018-08-16) didn't add the double-colon\nneeded for the labeled list separator, as a result the added\ndocumentation all got squashed into one paragraph. Fix that by adding\nthe list separator.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nJunio: Feel free to squash this in, but per your recent E-Mail it\ndoesn't seem you're planning to rewind \"next\", so this can go on top\nof gitster/bp/checkout-new-branch-optim.\n\n Documentation/config.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex ac71ade256..1546833213 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1153,7 +1153,7 @@ and by linkgit:git-worktree[1] when 'git worktree add' refers to a\n remote branch. This setting might be used for other checkout-like\n commands or functionality in the future.\n \n-checkout.optimizeNewBranch\n+checkout.optimizeNewBranch::\n \tOptimizes the performance of \"git checkout -b \" when\n \tusing sparse-checkout. When set to true, git will not update the\n \trepo based on the current sparse-checkout settings. This means it\n-- \n2.19.0.rc2.392.g5ba43deb5a\n\n"},{"id":"358398","messageId":"20180918165707.GC72102@syl","threadId":"48948","inReplyTo":"20180918053449.14047-1-avarab@gmail.com","subject":"Re: [PATCH] config doc: add missing list separator for checkout.optimizeNewBranch","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-09-18T16:57:07Z","receivedAt":"2018-09-18T16:57:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Ævar,\n\nOn Tue, Sep 18, 2018 at 05:34:49AM +0000, Ævar Arnfjörð Bjarmason wrote:\n> The documentation added in fa655d8411 (\"checkout: optimize \"git\n> checkout -b \"\", 2018-08-16) didn't add the double-colon\n> needed for the labeled list separator, as a result the added\n> documentation all got squashed into one paragraph. Fix that by adding\n> the list separator.\n\nLooks good. Here's my:\n\n Signed-off-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"358400","messageId":"20180918171317.GB15470@sigill.intra.peff.net","threadId":"48948","inReplyTo":"20180918053449.14047-1-avarab@gmail.com","subject":"Re: [PATCH] config doc: add missing list separator for checkout.optimizeNewBranch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-18T17:13:17Z","receivedAt":"2018-09-18T17:13:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 18, 2018 at 05:34:49AM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> The documentation added in fa655d8411 (\"checkout: optimize \"git\n> checkout -b \"\", 2018-08-16) didn't add the double-colon\n> needed for the labeled list separator, as a result the added\n> documentation all got squashed into one paragraph. Fix that by adding\n> the list separator.\n\nObviously the right thing to do, but your patch does not seem to apply\non that commit. Looks like you built it on a more recent commit that\nalso has checkout.defaultRemote (i.e., probably 'next')?\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index ac71ade256..1546833213 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1153,7 +1153,7 @@ and by linkgit:git-worktree[1] when 'git worktree add' refers to a\n> remote branch. This setting might be used for other checkout-like\n> commands or functionality in the future.\n> \n> -checkout.optimizeNewBranch\n> +checkout.optimizeNewBranch::\n> \tOptimizes the performance of \"git checkout -b \" when\n> \tusing sparse-checkout. When set to true, git will not update the\n> \trepo based on the current sparse-checkout settings. This means it\n\nNow the real reason I responded. :)\n\nI just want to plug the recently-added \"doc-diff\" script, which diffs a\nrendered view and makes it easier to catch cases like this. E.g., after\napplying your patch:\n\n $ cd Documentation\n $ ./doc-diff HEAD^ HEAD\n [...]\n diff --git a/6ad88fd5d99b8dcbb69c6f3239ad41e66516d8f7/none/share/man/man1/git-config.1 b/28e258682021a09cd7842e4edbb21ad231c8bb7a/none/share/man/man1/git-config.1\n index 7298292112..fa91b5f2d1 100644\n --- a/6ad88fd5d99b8dcbb69c6f3239ad41e66516d8f7/none/share/man/man1/git-config.1\n +++ b/28e258682021a09cd7842e4edbb21ad231c8bb7a/none/share/man/man1/git-config.1\n @@ -1533,12 +1533,13 @@ CONFIGURATION FILE\n branch. This setting might be used for other checkout-like commands\n or functionality in the future.\n \n - checkout.optimizeNewBranch Optimizes the performance of \"git checkout\n - -b \" when using sparse-checkout. When set to true, git will\n - not update the repo based on the current sparse-checkout settings. This\n - means it will not update the skip-worktree bit in the index nor\n - add/remove files in the working directory to reflect the current sparse\n - checkout settings nor will it show the local changes.\n + checkout.optimizeNewBranch\n + Optimizes the performance of \"git checkout -b \" when\n + using sparse-checkout. When set to true, git will not update the\n + repo based on the current sparse-checkout settings. This means it\n + will not update the skip-worktree bit in the index nor add/remove\n + files in the working directory to reflect the current sparse\n + checkout settings nor will it show the local changes.\n \n clean.requireForce\n A boolean to make git-clean do nothing unless given -f, -i or -n.\n\nObviously we knew this fix was going to help, but running it on the\noriginal fa655d8411 (versus its parent) and eyeballing the result might\nhave caught this sooner.\n\n(Though I would not be at all surprised to hear that the script does not\nrun on Windows, as it relies on things like \"man\"; patches certainly\nwelcome).\n\n-Peff\n"},{"id":"358401","messageId":"20180918171643.GC15470@sigill.intra.peff.net","threadId":"48948","inReplyTo":"20180918165707.GC72102@syl","subject":"Re: [PATCH] config doc: add missing list separator for checkout.optimizeNewBranch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-18T17:16:43Z","receivedAt":"2018-09-18T17:16:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 18, 2018 at 12:57:07PM -0400, Taylor Blau wrote:\n\n> Hi Ævar,\n> \n> On Tue, Sep 18, 2018 at 05:34:49AM +0000, Ævar Arnfjörð Bjarmason wrote:\n> > The documentation added in fa655d8411 (\"checkout: optimize \"git\n> > checkout -b \"\", 2018-08-16) didn't add the double-colon\n> > needed for the labeled list separator, as a result the added\n> > documentation all got squashed into one paragraph. Fix that by adding\n> > the list separator.\n> \n> Looks good. Here's my:\n> \n> Signed-off-by: Taylor Blau \n\nI'm confused here. The signoff is really about agreeing publicly to the\nDCO, and providing a chain of custody for the changes. So sometimes a\nsignoff from somebody besides the patch author is good if they\ncontributed content to the patch, but I don't see that here (or in any\nnearby thread).\n\nDid you mean \"Reviewed-by:\" ?\n\n-Peff\n"},{"id":"358402","messageId":"20180918172009.GA83552@syl","threadId":"48948","inReplyTo":"20180918171643.GC15470@sigill.intra.peff.net","subject":"Re: [PATCH] config doc: add missing list separator for checkout.optimizeNewBranch","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-09-18T17:20:09Z","receivedAt":"2018-09-18T17:20:15Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 18, 2018 at 01:16:43PM -0400, Jeff King wrote:\n> On Tue, Sep 18, 2018 at 12:57:07PM -0400, Taylor Blau wrote:\n>\n> > Hi Ævar,\n> >\n> > On Tue, Sep 18, 2018 at 05:34:49AM +0000, Ævar Arnfjörð Bjarmason wrote:\n> > > The documentation added in fa655d8411 (\"checkout: optimize \"git\n> > > checkout -b \"\", 2018-08-16) didn't add the double-colon\n> > > needed for the labeled list separator, as a result the added\n> > > documentation all got squashed into one paragraph. Fix that by adding\n> > > the list separator.\n> >\n> > Looks good. Here's my:\n> >\n> > Signed-off-by: Taylor Blau \n>\n> I'm confused here. The signoff is really about agreeing publicly to the\n> DCO, and providing a chain of custody for the changes. So sometimes a\n> signoff from somebody besides the patch author is good if they\n> contributed content to the patch, but I don't see that here (or in any\n> nearby thread).\n>\n> Did you mean \"Reviewed-by:\" ?\n\nIndeed, I meant \"Reviewed-by\" instead of \"Signed-off-by\". I grok the\ndifference between the two, but without thinking about it I typed one\ninstead of the other.\n\nInstead, here's my Reviewed-by:\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"358442","messageId":"87va72jdzp.fsf@evledraar.gmail.com","threadId":"48948","inReplyTo":"20180918171317.GB15470@sigill.intra.peff.net","subject":"Re: [PATCH] config doc: add missing list separator for checkout.optimizeNewBranch","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-19T04:41:14Z","receivedAt":"2018-09-19T04:41:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 18 2018, Jeff King wrote:\n\n> On Tue, Sep 18, 2018 at 05:34:49AM +0000, Ævar Arnfjörð Bjarmason wrote:\n>\n>> The documentation added in fa655d8411 (\"checkout: optimize \"git\n>> checkout -b \"\", 2018-08-16) didn't add the double-colon\n>> needed for the labeled list separator, as a result the added\n>> documentation all got squashed into one paragraph. Fix that by adding\n>> the list separator.\n>\n> Obviously the right thing to do, but your patch does not seem to apply\n> on that commit. Looks like you built it on a more recent commit that\n> also has checkout.defaultRemote (i.e., probably 'next')?\n\nYeah, it's based on top of next, which I was testing at the time and\ndidn't expect that conflict.\n"}]}