{"thread":{"id":"44072","subject":"[PATCH v3] checkout: eliminate unnecessary merge for trivial checkout","startedAt":"2016-09-13T14:27:00Z","lastAt":"2016-09-24T19:31:23Z","messageCount":11,"participants":["Ben Peart","Junio C Hamano","Oleg Taranenko","Philip Oakley"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"301842","messageId":"20160913142628.15440-1-benpeart@microsoft.com","threadId":"44072","inReplyTo":null,"subject":"[PATCH v3] checkout: eliminate unnecessary merge for trivial checkout","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2016-09-13T14:26:28Z","receivedAt":"2016-09-13T14:27:00Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Teach git to avoid unnecessary merge during trivial checkout.  When\nrunning 'git checkout -b foo' git follows a common code path through\nthe expensive merge_working_tree even when it is unnecessary.  As a\nresult, 95% of the time is spent in merge_working_tree doing the 2-way\nmerge between the new and old commit trees that is unneeded.\n\nThe time breakdown is as follows:\n\n    merge_working_tree <-- 95%\n        unpack_trees <-- 80%\n            traverse_trees <-- 50%\n            cache_tree_update <-- 17%\n            mark_new_skip_worktree <-- 10%\n\nWith a large repo, this cost is pronounced.  Using \"git checkout -b r\"\nto create and switch to a new branch costs 166 seconds (all times worst\ncase with a cold file system cache).\n\ngit.c:406               trace: built-in: git 'checkout' '-b' 'r'\nread-cache.c:1667       performance: 17.442926555 s: read_index_from\nname-hash.c:128         performance: 2.912145231 s: lazy_init_name_hash\nread-cache.c:2208       performance: 4.387713335 s: write_locked_index\ntrace.c:420             performance: 166.458921289 s: git command:\n                                        'c:\\Users\\benpeart\\bin\\git.exe' 'checkout' '-b' 'r'\nSwitched to a new branch 'r'\n\nBy adding a test to skip the unnecessary call to merge_working_tree in\nthis case reduces the cost to 16 seconds.\n\ngit.c:406               trace: built-in: git 'checkout' '-b' 's'\nread-cache.c:1667       performance: 16.100742476 s: read_index_from\ntrace.c:420             performance: 16.461547867 s: git command: 'c:\\Users\\benpeart\\bin\\git.exe' 'checkout' '-b' 's'\nSwitched to a new branch 's'\n\nSigned-off-by: Ben Peart <benpeart@microsoft.com>\n---\n builtin/checkout.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 92 insertions(+)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 8672d07..8b2f428 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -38,6 +38,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@@ -460,11 +464,99 @@ 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,\n+\tconst struct branch_info *new)\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->commit || !new->commit ||\n+\t\toidcmp(&old->commit->tree->object.oid, &new->commit->tree->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 * Checking out the requested commit may require updating the working\n+\t * directory and index, let the merge handle it.\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're not creating a new branch, by definition we're changing\n+\t * the existing one so need to do the merge\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 <start_point>\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 * that doesn't trigger the need for a merge.\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,\n \t\t\t      struct branch_info *new,\n \t\t\t      int *writeout_error)\n {\n+\t/*\n+\t * Optimize the performance of \"git checkout -b foo\" by avoiding\n+\t * the expensive merge, index and working directory updates if they\n+\t * are not needed.\n+\t */\n+\tif (!needs_working_tree_merge(opts, old, new))\n+\t\treturn 0;\n+\n \tint ret;\n \tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n \n-- \n2.10.0.windows.1\n\n"},{"id":"301878","messageId":"xmqq7fafv376.fsf@gitster.mtv.corp.google.com","threadId":"44072","inReplyTo":"20160913142628.15440-1-benpeart@microsoft.com","subject":"Re: [PATCH v3] checkout: eliminate unnecessary merge for trivial checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-13T22:34:53Z","receivedAt":"2016-09-13T22:35:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart <peartben@gmail.com> writes:\n\n> +static int needs_working_tree_merge(const struct checkout_opts *opts,\n> +\tconst struct branch_info *old,\n> +\tconst struct branch_info *new)\n> +{\n> +...\n> +}\n\nI do not think I need to repeat the same remarks on the conditions\nin this helper, which hasn't changed since v2.  Many \"comments\" in\nthe code do not explain why skipping is justified, or what they\nclaim to check looks to me just plain wrong.\n\nFor example, there is\n\n       /*\n        * If we're not creating a new branch, by definition we're changing\n        * the existing one so need to do the merge\n        */\n       if (!opts->new_branch)\n               return 1;\n\t\nbut \"git checkout\" (no other argument) hits this condition.  It\ndisables the most trivial optimization opportunity, because we are\nnot \"creating\".\n\n\"By definition, we're changing\"?  Really?  Not quite.\n\nIf you disable this bogus check, \"git checkout\" (no other argument)\nwould be allowed to skip the merge_working_tree(), and that in turn\nreveals another case that the helper is not checking when\nunpack_trees() MUST be called.\n\n    Note: namely, when sparse checkout is in effect, switching from\n    HEAD to HEAD can nuke existing working tree files outside the\n    sparse pattern -- YUCK!  See penultimate test in t1011 for\n    an example.\n\nThis yuckiness is not your fault, but needs_working_tree_merge()\nlogic you added needs to refrain from skipping unpack_trees() call\nwhen sparse thing is in effect.  I'd expect \"git checkout -b foo\"\ninstead of \"git checkout\" (no other argument) would fail to honor\nthe sparse thing and reveal this bug, because the above bogus\n\"!opts->new_branch\" check will not protect you for that case.\n\nIn other words, these random series of \"if (...) return 1\" are bugs\nhiding other real bugs and we need to reason about which ones are\nbugs that are hiding what other bugs that are not covered by this\nfunction.  As Peff said earlier for v1, this is still an unreadable\nmess.  We need to figure out a way to make sure we are skipping on\nthe right condition and not accidentally hiding a bug of failing to\ncheck the right condition.  I offhand do not have a good suggestion\non this; sorry.\n\n>  static int merge_working_tree(const struct checkout_opts *opts,\n>  \t\t\t      struct branch_info *old,\n>  \t\t\t      struct branch_info *new,\n>  \t\t\t      int *writeout_error)\n>  {\n> +\t/*\n> +\t * Optimize the performance of \"git checkout -b foo\" by avoiding\n> +\t * the expensive merge, index and working directory updates if they\n> +\t * are not needed.\n> +\t */\n> +\tif (!needs_working_tree_merge(opts, old, new))\n> +\t\treturn 0;\n> +\n>  \tint ret;\n>  \tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n\nWith the change you made at the beginning of this function, it no\nlonger compiles with -Wdecl-after-stmt, but that is the smallest of\nthe problems.\n\nIt is a small step in the right direction to move the call to the\nhelper from the caller to this function, but it is a bit too small.\n\nNotice that the lines after the above context look like this:\n\n\thold_locked_index(lock_file, 1);\n\tif (read_cache_preload(NULL) < 0)\n\t\treturn error(_(\"index file corrupt\"));\n\n\tresolve_undo_clear();\n\tif (opts->force) {\n\t\tret = reset_tree(new->commit->tree, opts, 1, writeout_error);\n\t\tif (ret)\n\t\t\treturn ret;\n\t} else {\n\t\tstruct tree_desc trees[2];\n\t\t...\n\nI would have expected that the check goes inside the \"else\" thing\nthat actually does a two-tree merge, and the helper loses the check\nwith opts->force, at least.  That would still be a change smaller\nthan desired, but at least a meaningful improvement compared to the\nprevious one.  As I have already pointed out, in the \"else\" clause\nthere is a check \"is the index free of conflicted entries? if so\nerror out\", and that must be honored in !opt->force case, no matter\nwhat your needs_working_tree_merge() says.  I also was hoping that\nyou would notice, when you were told about the unmerged check, by\nreading the remainder of the merge_working_tree(), that we need to\ncall show_local_changes() when we are not doing force and when we\nare not quiet---returning early like the above patch will never be\nable to call that one downstream in the function.\n\nRegardless of what the actual checks end up to be, the right place\nto do this \"optimization\" would look more like:\n\n builtin/checkout.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 2b50a49..a6b9e17 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -508,14 +508,19 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\ttopts.dir->flags |= DIR_SHOW_IGNORED;\n \t\t\tsetup_standard_excludes(topts.dir);\n \t\t}\n+\n+\t\tif ( we know we can skip the unpack ) {\n+\t\t\tret = 0;\n+\t\t} else {\n \t\t\ttree = parse_tree_indirect(old->commit ?\n \t\t\t\t\t\t   old->commit->object.oid.hash :\n \t\t\t\t\t\t   EMPTY_TREE_SHA1_BIN);\n \t\t\tinit_tree_desc(&trees[0], tree->buffer, tree->size);\n \t\t\ttree = parse_tree_indirect(new->commit->object.oid.hash);\n \t\t\tinit_tree_desc(&trees[1], tree->buffer, tree->size);\n-\n \t\t\tret = unpack_trees(2, trees, &topts);\n+\t\t}\n+\n \t\tif (ret == -1) {\n \t\t\t/*\n \t\t\t * Unpack couldn't do a trivial merge; either\n\nI'd think.  Note that the determination of \"we can skip\" would\ninvolve knowing the object names of the two trees involved, so for\nperformance reasons, some of the parse-tree calls may have to come\nbefore the call to \"do we know we can skip?\", but that does not\nfundamentally change the basic code structure.\n\nThanks.\n"},{"id":"301899","messageId":"CABEd3j_FrWhOe_jXcc+VJWiyy80SG1JfbZC9woRU2cqdzjkGyA@mail.gmail.com","threadId":"44072","inReplyTo":"xmqq7fafv376.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3] checkout: eliminate unnecessary merge for trivial checkout","fromName":"Oleg Taranenko","fromEmail":"olegtaranenko@gmail.com","sentAt":"2016-09-14T06:30:49Z","receivedAt":"2016-09-14T06:30:56Z","isPatch":true,"sender":{"key":"olegtaranenko@gmail.com","avatar":"https://gravatar.com/avatar/016106c4926412cef4141b2d7cea4378aef44db0ca56c9462a5f4a71c7a80955?d=mp&s=160"},"body":"Sorry for bothering, why not introduce a brand new option like git\ncheckout -b foo --skip-worktree-merge for such rare optimization use\ncase?\n\nOn Wed, Sep 14, 2016 at 12:34 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Ben Peart <peartben@gmail.com> writes:\n>\n>> +static int needs_working_tree_merge(const struct checkout_opts *opts,\n>> +     const struct branch_info *old,\n>> +     const struct branch_info *new)\n>> +{\n>> +...\n>> +}\n>\n> I do not think I need to repeat the same remarks on the conditions\n> in this helper, which hasn't changed since v2.  Many \"comments\" in\n> the code do not explain why skipping is justified, or what they\n> claim to check looks to me just plain wrong.\n>\n> For example, there is\n>\n>        /*\n>         * If we're not creating a new branch, by definition we're changing\n>         * the existing one so need to do the merge\n>         */\n>        if (!opts->new_branch)\n>                return 1;\n>\n> but \"git checkout\" (no other argument) hits this condition.  It\n> disables the most trivial optimization opportunity, because we are\n> not \"creating\".\n>\n> \"By definition, we're changing\"?  Really?  Not quite.\n>\n> If you disable this bogus check, \"git checkout\" (no other argument)\n> would be allowed to skip the merge_working_tree(), and that in turn\n> reveals another case that the helper is not checking when\n> unpack_trees() MUST be called.\n>\n>     Note: namely, when sparse checkout is in effect, switching from\n>     HEAD to HEAD can nuke existing working tree files outside the\n>     sparse pattern -- YUCK!  See penultimate test in t1011 for\n>     an example.\n>\n> This yuckiness is not your fault, but needs_working_tree_merge()\n> logic you added needs to refrain from skipping unpack_trees() call\n> when sparse thing is in effect.  I'd expect \"git checkout -b foo\"\n> instead of \"git checkout\" (no other argument) would fail to honor\n> the sparse thing and reveal this bug, because the above bogus\n> \"!opts->new_branch\" check will not protect you for that case.\n>\n> In other words, these random series of \"if (...) return 1\" are bugs\n> hiding other real bugs and we need to reason about which ones are\n> bugs that are hiding what other bugs that are not covered by this\n> function.  As Peff said earlier for v1, this is still an unreadable\n> mess.  We need to figure out a way to make sure we are skipping on\n> the right condition and not accidentally hiding a bug of failing to\n> check the right condition.  I offhand do not have a good suggestion\n> on this; sorry.\n>\n>>  static int merge_working_tree(const struct checkout_opts *opts,\n>>                             struct branch_info *old,\n>>                             struct branch_info *new,\n>>                             int *writeout_error)\n>>  {\n>> +     /*\n>> +      * Optimize the performance of \"git checkout -b foo\" by avoiding\n>> +      * the expensive merge, index and working directory updates if they\n>> +      * are not needed.\n>> +      */\n>> +     if (!needs_working_tree_merge(opts, old, new))\n>> +             return 0;\n>> +\n>>       int ret;\n>>       struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n>\n> With the change you made at the beginning of this function, it no\n> longer compiles with -Wdecl-after-stmt, but that is the smallest of\n> the problems.\n>\n> It is a small step in the right direction to move the call to the\n> helper from the caller to this function, but it is a bit too small.\n>\n> Notice that the lines after the above context look like this:\n>\n>         hold_locked_index(lock_file, 1);\n>         if (read_cache_preload(NULL) < 0)\n>                 return error(_(\"index file corrupt\"));\n>\n>         resolve_undo_clear();\n>         if (opts->force) {\n>                 ret = reset_tree(new->commit->tree, opts, 1, writeout_error);\n>                 if (ret)\n>                         return ret;\n>         } else {\n>                 struct tree_desc trees[2];\n>                 ...\n>\n> I would have expected that the check goes inside the \"else\" thing\n> that actually does a two-tree merge, and the helper loses the check\n> with opts->force, at least.  That would still be a change smaller\n> than desired, but at least a meaningful improvement compared to the\n> previous one.  As I have already pointed out, in the \"else\" clause\n> there is a check \"is the index free of conflicted entries? if so\n> error out\", and that must be honored in !opt->force case, no matter\n> what your needs_working_tree_merge() says.  I also was hoping that\n> you would notice, when you were told about the unmerged check, by\n> reading the remainder of the merge_working_tree(), that we need to\n> call show_local_changes() when we are not doing force and when we\n> are not quiet---returning early like the above patch will never be\n> able to call that one downstream in the function.\n>\n> Regardless of what the actual checks end up to be, the right place\n> to do this \"optimization\" would look more like:\n>\n>  builtin/checkout.c | 7 ++++++-\n>  1 file changed, 6 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 2b50a49..a6b9e17 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -508,14 +508,19 @@ static int merge_working_tree(const struct checkout_opts *opts,\n>                         topts.dir->flags |= DIR_SHOW_IGNORED;\n>                         setup_standard_excludes(topts.dir);\n>                 }\n> +\n> +               if ( we know we can skip the unpack ) {\n> +                       ret = 0;\n> +               } else {\n>                         tree = parse_tree_indirect(old->commit ?\n>                                                    old->commit->object.oid.hash :\n>                                                    EMPTY_TREE_SHA1_BIN);\n>                         init_tree_desc(&trees[0], tree->buffer, tree->size);\n>                         tree = parse_tree_indirect(new->commit->object.oid.hash);\n>                         init_tree_desc(&trees[1], tree->buffer, tree->size);\n> -\n>                         ret = unpack_trees(2, trees, &topts);\n> +               }\n> +\n>                 if (ret == -1) {\n>                         /*\n>                          * Unpack couldn't do a trivial merge; either\n>\n> I'd think.  Note that the determination of \"we can skip\" would\n> involve knowing the object names of the two trees involved, so for\n> performance reasons, some of the parse-tree calls may have to come\n> before the call to \"do we know we can skip?\", but that does not\n> fundamentally change the basic code structure.\n>\n> Thanks.\n"},{"id":"301908","messageId":"xmqqintyscs3.fsf@gitster.mtv.corp.google.com","threadId":"44072","inReplyTo":"CABEd3j_FrWhOe_jXcc+VJWiyy80SG1JfbZC9woRU2cqdzjkGyA@mail.gmail.com","subject":"Re: [PATCH v3] checkout: eliminate unnecessary merge for trivial checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-14T15:48:28Z","receivedAt":"2016-09-14T15:48:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oleg Taranenko <olegtaranenko@gmail.com> writes:\n\n> Sorry for bothering, why not introduce a brand new option like git\n> checkout -b foo --skip-worktree-merge for such rare optimization use\n> case?\n\nI am not sure what problem such a new option solves.  How would you\ndescribe and explain what \"--skip-worktree-merge\" option to the end\nuser?\n"},{"id":"302135","messageId":"007401d21278$445eba80$cd1c2f80$@gmail.com","threadId":"44072","inReplyTo":"BL2PR03MB323E1B2F810C63CB01AA234F4F30@BL2PR03MB323.namprd03.prod.outlook.com","subject":"RE: [PATCH v3] checkout: eliminate unnecessary merge for trivial checkout","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2016-09-19T13:18:08Z","receivedAt":"2016-09-19T13:19:05Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Let me see if I can better explain what I’m trying to accomplish with this\npatch.  \n \n\"git checkout -b foo\" (without -f -m or <start_point>) is defined in the\nmanual as being a shortcut for/equivalent to:\n \n        (1a) \"git branch foo\"\n        (1b) \"git checkout foo\"\n \nHowever, it has been our experience in our observed use cases and all the\nexisting git tests, that it can be treated as equivalent to:\n \n        (2a) \"git branch foo\"\n        (2b) \"git symbolic-ref HEAD refs/heads/foo\"\n \nThat is, the common perception (use case) is to just create a new branch\n\"foo\" (pointing at the current commit) and point HEAD at it WITHOUT making\nany changes to the index or worktree.\n \nHowever, the (1b) command has \"git reset\" connotations in that it should\nexamine and manipulate the trees, index, and worktree in the expectation\nthat there MIGHT be work to do.\n \nSince this additional work in (1b) takes minutes on large repos and (2b)\ntakes less than a second, my intent was to identify the conditions that this\nadditional work will have no affect and thereby avoid it.\n \nAlternatively, was the \"-b\" option just created as a shortcut only to avoid\ncalling the separate \"git branch foo\" command and we should not think about\nthe common perception and usage?\n \nMore comments inline...\n \n> -----Original Message-----\n> From: Junio C Hamano [mailto:gitster@pobox.com]\n> Sent: Tuesday, September 13, 2016 6:35 PM\n> To: Ben Peart <mailto:peartben@gmail.com>\n> Cc: mailto:git@vger.kernel.org; mailto:pclouds@gmail.com; Ben Peart\n> <mailto:Ben.Peart@microsoft.com>\n> Subject: Re: [PATCH v3] checkout: eliminate unnecessary merge for trivial\n> checkout\n> \n> Ben Peart <mailto:peartben@gmail.com> writes:\n> \n> > +static int needs_working_tree_merge(const struct checkout_opts *opts,\n> > +      const struct branch_info *old,\n> > +      const struct branch_info *new)\n> > +{\n> > +...\n> > +}\n> \n> I do not think I need to repeat the same remarks on the conditions in this\n> helper, which hasn't changed since v2.  Many \"comments\" in the code do not\n> explain why skipping is justified, or what they claim to check looks to me\njust\n> plain wrong.\n> \n> For example, there is\n> \n>        /*\n>         * If we're not creating a new branch, by definition we're changing\n>         * the existing one so need to do the merge\n>         */\n>        if (!opts->new_branch)\n>                return 1;\n> \n> but \"git checkout\" (no other argument) hits this condition.  It disables\nthe\n> most trivial optimization opportunity, because we are not \"creating\".\n> \n \nDisabling the optimization for \"git checkout\" with no argument was\nintentional. This command does not create a new branch but instead, performs\na \"soft reset\" which will update the index and working directory to reflect\nchanges to the sparse-checkout (for example).  If this was not disabled,\nmany tests fail as they expect this behavior.  Because \"git checkout\" does\nnot actually change the refs, if we skipped the merge/index/working\ndirectory update, this command becomes a no-op.\n \n> \"By definition, we're changing\"?  Really?  Not quite.\n> \n \nWhat I was attempting to communicate is that if we aren't creating a new\nbranch any changes or updates will happen in the existing branch.  Since\nthat could only be updating the index and working directory, we don't want\nto skip those steps or we've defeated any purpose in running the command. \n \n> If you disable this bogus check, \"git checkout\" (no other argument) would\nbe\n> allowed to skip the merge_working_tree(), and that in turn reveals another\n> case that the helper is not checking when\n> unpack_trees() MUST be called.\n> \n>     Note: namely, when sparse checkout is in effect, switching from\n>     HEAD to HEAD can nuke existing working tree files outside the\n>     sparse pattern -- YUCK!  See penultimate test in t1011 for\n>     an example.\n> \n> This yuckiness is not your fault, but needs_working_tree_merge() logic you\n> added needs to refrain from skipping unpack_trees() call when sparse thing\n> is in effect.  I'd expect \"git checkout -b foo\"\n> instead of \"git checkout\" (no other argument) would fail to honor the\nsparse\n> thing and reveal this bug, because the above bogus \"!opts->new_branch\"\n> check will not protect you for that case.\n> \n \nIt is correct that this optimization will skip updating the tree to honor\nany changes to the sparse-checkout in the case of creating a new branch.\nUnfortunately, I don't know of any way to detect the changes other than\nactually doing all the work to update the skip work tree bit in the index.\nIf this behavior is required, then this optimization will need to check  if\nsparse-checkout is enabled and skip the optimization just in case there have\nbeen changes.\n \n> In other words, these random series of \"if (...) return 1\" are bugs hiding\n> other real bugs and we need to reason about which ones are bugs that are\n> hiding what other bugs that are not covered by this function.  As Peff\nsaid\n> earlier for v1, this is still an unreadable mess.  We need to figure out a\nway to\n> make sure we are skipping on the right condition and not accidentally\nhiding\n> a bug of failing to check the right condition.  I offhand do not have a\ngood\n> suggestion on this; sorry.\n> \n \nBeyond this code review process and testing, I don't know how else we make\nsure we're caught all the conditions where we are OK skipping some of the\nsteps.  Any change has inherent risk - a change in behavior even more so.\n \n> >  static int merge_working_tree(const struct checkout_opts *opts,\n> >                                                struct branch_info *old,\n> >                                                struct branch_info *new,\n> >                                                int *writeout_error)\n> >  {\n> > +      /*\n> > +      * Optimize the performance of \"git checkout -b foo\" by avoiding\n> > +      * the expensive merge, index and working directory updates if\nthey\n> > +      * are not needed.\n> > +      */\n> > +      if (!needs_working_tree_merge(opts, old, new))\n> > +                      return 0;\n> > +\n> >          int ret;\n> >          struct lock_file *lock_file = xcalloc(1, sizeof(struct\nlock_file));\n> \n> With the change you made at the beginning of this function, it no longer\n> compiles with -Wdecl-after-stmt, but that is the smallest of the problems.\n \nI apologize, I didn't realize this was a requirement.  It built and passed\nall existing tests on Windows but I will reorder the declarations to prevent\ncausing issues with other platforms/compilers.\n \n> \n> It is a small step in the right direction to move the call to the helper\nfrom the\n> caller to this function, but it is a bit too small.\n> \n> Notice that the lines after the above context look like this:\n> \n>             hold_locked_index(lock_file, 1);\n>             if (read_cache_preload(NULL) < 0)\n>                             return error(_(\"index file corrupt\"));\n> \n>             resolve_undo_clear();\n>             if (opts->force) {\n>                             ret = reset_tree(new->commit->tree, opts, 1,\n> writeout_error);\n>                             if (ret)\n>                                             return ret;\n>             } else {\n>                             struct tree_desc trees[2];\n>                             ...\n> \n> I would have expected that the check goes inside the \"else\" thing that\n> actually does a two-tree merge, and the helper loses the check with opts-\n> >force, at least.  That would still be a change smaller than desired, but\nat\n> least a meaningful improvement compared to the previous one.  \n \nI'll restructure it that way.\n \n> As I have\n> already pointed out, in the \"else\" clause there is a check \"is the index\nfree of\n> conflicted entries? if so error out\", and that must be honored in\n!opt->force\n> case, no matter what your needs_working_tree_merge() says.  \n \nGiven we're not merging trees, updating the index, or work tree, why do we\nneed to error out in this case?  We aren't attempting this optimization if\nthey pass \"-m.\"  If there are conflicted entries that haven't been fixed,\nthey will still exist.  We're essentially just creating a new reference for\nthe existing commit/index/work tree.\n \n> I also was\n> hoping that you would notice, when you were told about the unmerged\n> check, by reading the remainder of the merge_working_tree(), that we need\n> to call show_local_changes() when we are not doing force and when we are\n> not quiet---returning early like the above patch will never be able to\ncall that\n> one downstream in the function.\n \nIt is a good point that my optimization skipped the call to\nshow_local_changes.  Thanks for catching that, I've fixed it for my next\niteration.\n \n> \n> Regardless of what the actual checks end up to be, the right place to do\nthis\n> \"optimization\" would look more like:\n> \n>  builtin/checkout.c | 7 ++++++-\n>  1 file changed, 6 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/checkout.c b/builtin/checkout.c index\n2b50a49..a6b9e17\n> 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -508,14 +508,19 @@ static int merge_working_tree(const struct\n> checkout_opts *opts,\n>                                             topts.dir->flags |=\nDIR_SHOW_IGNORED;\n>                                            \nsetup_standard_excludes(topts.dir);\n>                             }\n> +\n> +                          if ( we know we can skip the unpack ) {\n> +                                          ret = 0;\n> +                          } else {\n>                                             tree =\nparse_tree_indirect(old->commit ?\n> \n                                                                            \n                  old->commit-\n> >object.oid.hash :\n> \n                                                                            \n                  EMPTY_TREE_SHA1_BIN);\n>                                             init_tree_desc(&trees[0],\ntree->buffer, tree->size);\n>                                             tree =\nparse_tree_indirect(new->commit-\n> >object.oid.hash);\n>                                             init_tree_desc(&trees[1],\ntree->buffer, tree->size);\n> -\n>                                             ret = unpack_trees(2, trees,\n&topts);\n> +                          }\n> +\n>                             if (ret == -1) {\n>                                             /*\n>                                              * Unpack couldn't do a\ntrivial merge; either\n> \n \nI'll restructure it to be like you suggest above however, given we will not\nbe merging the tress, we won't have any index changes to write out. I will\nalso skip the calls to cache_tree_update and write_locked_index.\n \n> I'd think.  Note that the determination of \"we can skip\" would involve\n> knowing the object names of the two trees involved, so for performance\n> reasons, some of the parse-tree calls may have to come before the call to\n> \"do we know we can skip?\", but that does not fundamentally change the\n> basic code structure.\n> \n> Thanks.\n \nI don't understand why we'd need to know the object names of the two trees\ngiven we have the IDs.  What did you have in mind that would need those?\n \n\n"},{"id":"302150","messageId":"xmqqzin3g8di.fsf@gitster.mtv.corp.google.com","threadId":"44072","inReplyTo":"007401d21278$445eba80$cd1c2f80$@gmail.com","subject":"Re: [PATCH v3] checkout: eliminate unnecessary merge for trivial checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-19T16:30:17Z","receivedAt":"2016-09-19T16:30:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ben Peart\" <peartben@gmail.com> writes:\n\n> Let me see if I can better explain what I’m trying to accomplish with this\n> patch.  \n>  \n> \"git checkout -b foo\" (without -f -m or <start_point>) is defined in the\n> manual as being a shortcut for/equivalent to:\n>  \n>         (1a) \"git branch foo\"\n>         (1b) \"git checkout foo\"\n>  \n> However, it has been our experience in our observed use cases and all the\n> existing git tests, that it can be treated as equivalent to:\n>  \n>         (2a) \"git branch foo\"\n>         (2b) \"git symbolic-ref HEAD refs/heads/foo\"\n>  \n> That is, the common perception (use case) is to just create a new branch\n> \"foo\" (pointing at the current commit) and point HEAD at it WITHOUT making\n> any changes to the index or worktree.\n>  \n> However, the (1b) command has \"git reset\" connotations in that it should\n> examine and manipulate the trees, index, and worktree in the expectation\n> that there MIGHT be work to do.\n>  \n> Since this additional work in (1b) takes minutes on large repos and (2b)\n> takes less than a second, my intent was to identify the conditions that this\n> additional work will have no affect and thereby avoid it.\n>  \n> Alternatively, was the \"-b\" option just created as a shortcut only to avoid\n> calling the separate \"git branch foo\" command and we should not think about\n> the common perception and usage?\n\nIf you are trying to change the definition of \"checkout -b\" from 1\nto 2 above, that is a completely different issue.  I thought this\nwas an attempt to optimize for the performance without changing the\nbehaviour.\n\nSo if you did not apologize like this...\n\n> It is correct that this optimization will skip updating the tree to honor\n> any changes to the sparse-checkout in the case of creating a new branch.\n> Unfortunately, I don't know of any way to detect the changes other than\n> actually doing all the work to update the skip work tree bit in the index.\n\n... but insisted that skipping the yucky sparse-checkout adjustment\nin this case was an intended behaviour change, I would have\nunderstood (not necessarily agreed, though) what you were trying to\ndo.\n\n> Beyond this code review process and testing, I don't know how else we make\n> sure we're caught all the conditions where we are OK skipping some of the\n> steps. Any change has inherent risk - a change in behavior even more so.\n\nAt least we made one-step progress today.  I now know that you are\ntrying to change the behaviour, but I didn't know that last week,\nwhen I was primarily reacting that your claim that this was\nperformance thing and assuming you meant no change in behaviour, but\nthere was clearly behaviour change, and it was apparent that the\ndenseness of the code made it almost impossible to see if there are\nunintended changes.\n\nI am still not sure if I like the change of what \"checkout -b\" is\nthis late in the game, though.\n"},{"id":"302152","messageId":"xmqqr38fg6tk.fsf@gitster.mtv.corp.google.com","threadId":"44072","inReplyTo":"xmqqzin3g8di.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3] checkout: eliminate unnecessary merge for trivial checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-19T17:03:51Z","receivedAt":"2016-09-19T17:04:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> \"git checkout -b foo\" (without -f -m or <start_point>) is defined in the\n>> manual as being a shortcut for/equivalent to:\n>>  \n>>         (1a) \"git branch foo\"\n>>         (1b) \"git checkout foo\"\n>>  \n>> However, it has been our experience in our observed use cases and all the\n>> existing git tests, that it can be treated as equivalent to:\n>>  \n>>         (2a) \"git branch foo\"\n>>         (2b) \"git symbolic-ref HEAD refs/heads/foo\"\n>> ...\n>\n> I am still not sure if I like the change of what \"checkout -b\" is\n> this late in the game, though.\n\nHaving said all that.\n\nI do see the merit of having a shorthand way to invoke your 2 above.\nIt is just that I am not convinced that it is the best way to\nachieve that goal to redefine what \"git checkout -b <new-name>\" (no\nother parameters) does.\n"},{"id":"302330","messageId":"BL2PR03MB323ADC371E49EFD1CBBC566F4F60@BL2PR03MB323.namprd03.prod.outlook.com","threadId":"44072","inReplyTo":"xmqqr38fg6tk.fsf@gitster.mtv.corp.google.com","subject":"RE: [PATCH v3] checkout: eliminate unnecessary merge for trivial checkout","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2016-09-21T18:32:19Z","receivedAt":"2016-09-21T21:06:16Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"I understand the reluctance to change the existing behavior of the \"git checkout -b <new-name>\" command.\n\nI see this as a tradeoff between taking advantage of the muscle memory for the existing command and coming up with a new shortcut command and training people to use it instead.\n\nThe fact that all the use cases we've observed and all the git test cases actually produce the same results but significantly faster with that change in behavior made me hope we could redefine the command to take advantage of the muscle memory.\n\nThat said, you're much more on the frontline of receiving negative feedback about doing that than I am. :)  How would you like to proceed?\n\nBen\n\n-----Original Message-----\nFrom: Junio C Hamano [mailto:gitster@pobox.com] \nSent: Monday, September 19, 2016 1:04 PM\nTo: Ben Peart <peartben@gmail.com>\nCc: pclouds@gmail.com; Ben Peart <Ben.Peart@microsoft.com>; git@vger.kernel.org\nSubject: Re: [PATCH v3] checkout: eliminate unnecessary merge for trivial checkout\n\nJunio C Hamano <gitster@pobox.com> writes:\n\n>> \"git checkout -b foo\" (without -f -m or <start_point>) is defined in \n>> the manual as being a shortcut for/equivalent to:\n>>  \n>>         (1a) \"git branch foo\"\n>>         (1b) \"git checkout foo\"\n>>  \n>> However, it has been our experience in our observed use cases and all \n>> the existing git tests, that it can be treated as equivalent to:\n>>  \n>>         (2a) \"git branch foo\"\n>>         (2b) \"git symbolic-ref HEAD refs/heads/foo\"\n>> ...\n>\n> I am still not sure if I like the change of what \"checkout -b\" is this \n> late in the game, though.\n\nHaving said all that.\n\nI do see the merit of having a shorthand way to invoke your 2 above.\nIt is just that I am not convinced that it is the best way to achieve that goal to redefine what \"git checkout -b <new-name>\" (no other parameters) does.\n"},{"id":"302479","messageId":"99A866BEF8944598A50C6E061A703297@PhilipOakley","threadId":"44072","inReplyTo":"BL2PR03MB323ADC371E49EFD1CBBC566F4F60@BL2PR03MB323.namprd03.prod.outlook.com","subject":"Re: [PATCH v3] checkout: eliminate unnecessary merge for trivial checkout","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2016-09-24T14:28:11Z","receivedAt":"2016-09-24T14:28:18Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"Ben,\nUsing a 'bottom / in-line' posting flow is much preferred, which may require \nsome manual editing[1], hopefully I have it about right...\nPhilip\n--\n[1] this is massaged and mangled Outlook Express, sometimes one has to work \nwith the tools at hand...\n\nFrom: \"Ben Peart\" <Ben.Peart@microsoft.com>\n> From: Junio C Hamano [mailto:gitster@pobox.com]\n> > Junio C Hamano <gitster@pobox.comwrites:\n> >\n> > >\"git checkout -b foo\" (without -f -m or <start_point>) is defined in\n> > >the manual as being a shortcut for/equivalent to:\n> > >\n> > >        (1a) \"git branch foo\"\n> > >        (1b) \"git checkout foo\"\n> > >\n> > >However, it has been our experience in our observed use cases and all\n> > >the existing git tests, that it can be treated as equivalent to:\n> > >\n> > >        (2a) \"git branch foo\"\n> > >        (2b) \"git symbolic-ref HEAD refs/heads/foo\"\n> > >...\n> > >\n> > I am still not sure if I like the change of what \"checkout -b\" is this\n> > late in the game, though.\n> >\n> > Having said all that.\n> >\n> > I do see the merit of having a shorthand way to invoke your 2 above.\n> > It is just that I am not convinced that it is the best way to achieve \n> > that goal to redefine what \"git checkout -b <new-name>\" (no other \n> > parameters) does.\n> >\n> ---\n>\n> I understand the reluctance to change the existing behavior of the \"git \n> checkout -b <new-name>\" command.\n>\n> I see this as a tradeoff between taking advantage of the muscle memory for \n> the existing command and coming up with a new shortcut command and \n> training people to use it instead.\n>\n> The fact that all the use cases we've observed and all the git test cases \n> actually produce the same results but significantly faster with that \n> change in behavior made me hope we could redefine the command to take \n> advantage of the muscle memory.\n>\n> That said, you're much more on the frontline of receiving negative \n> feedback about doing that than I am. :)  How would you like to proceed?\n\nThe discussion can often feel harsh [2], especially if there is accidental \n'talking past each other', which is usually because of differing \nperspectives on the issues.\n\nI didn't see an initial confirmation as to what the issue really was. You \nindicated the symptom ('a long checkout time'), but then we missed out on \nhard facts and example repos, so that the issue was replicable.\n\nIs there an example public repo that you can show the issue on? (or \nanonymise a private one - there is a script for that [3])\n\nCan you give local timings (and indication of the hardware and software \nversions used for the test, and if appropriate, network setup)?\n\nI know at my work that sometime our home drives are multiply mapped to H:, a \nC:/homedrive directory and a $netshare/me network directory via the \nMicrosofy roaming profiles, and if there is hard synchronization (or \nwhatever term is appropriate) there can be sudden slowdowns as local C: \nwrites drop from 'instant' to 'forever'...\n\nIs there anything special about the repos that have the delays? Is it a \nlocal process issue that causes the repos to develop those symptoms (see \nabove about not being sure why you have these issues), in which case it \ncould be local self inflicted issues, or it could be that you have a \nregulatory issue for that domain that requires such symptoms, which would \nshift the problem from a 'don't do that' response to a 'hmm, how to cover \nthis'.\n\n\nAt the moment there is the simple workaround of an alias that executes that \ntwo step command dance to achieve what you needed, and Junio has outlined \nthe issues he needed to be covered from his maintainer perspective (e.g. the \ndetection of sparse checkouts). Confirming the root causes would help in \nsetting a baseline.\n\nI hope that is of help - I'd seen that the discussion had gone quiet.\n\n--\nPhilip\n\n\n[2] Been there, feel your pain. It's not in any way malicious, just a \nreflection that email can be a poor medium for such discussions.\n[3] https://public-inbox.org/git/20140827170127.GA6138@peff.net/ suggest \nthat the `git fast-export --anonymize --all` maybe the approach. \n\n"},{"id":"302483","messageId":"xmqqd1jtyx01.fsf@gitster.mtv.corp.google.com","threadId":"44072","inReplyTo":"99A866BEF8944598A50C6E061A703297@PhilipOakley","subject":"Re: [PATCH v3] checkout: eliminate unnecessary merge for trivial checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-24T18:26:54Z","receivedAt":"2016-09-24T18:27:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philip Oakley\" <philipoakley@iee.org> writes:\n\n>> > >\"git checkout -b foo\" (without -f -m or <start_point>) is defined in\n>> > >the manual as being a shortcut for/equivalent to:\n>> > >\n>> > >        (1a) \"git branch foo\"\n>> > >        (1b) \"git checkout foo\"\n>> > >\n>> > >However, it has been our experience in our observed use cases and all\n>> > >the existing git tests, that it can be treated as equivalent to:\n>> > >\n>> > >        (2a) \"git branch foo\"\n>> > >        (2b) \"git symbolic-ref HEAD refs/heads/foo\"\n>> > >...\n>> > >\n>> > I am still not sure if I like the change of what \"checkout -b\" is this\n>> > late in the game, though.\n>>\n>> ...\n>> That said, you're much more on the frontline of receiving negative\n>> feedback about doing that than I am. :)  How would you like to\n>> proceed?\n>\n> I didn't see an initial confirmation as to what the issue really\n> was. You indicated the symptom ('a long checkout time'), but then we\n> missed out on hard facts and example repos, so that the issue was\n> replicable.\n\nI took it as a given, trivial and obvious optimization opportunity,\nthat it is wasteful having to traverse two trees to consolidate and\nreflect their differences into the working tree when we know upfront\nthat these two trees are identical, no matter what the overhead for\ndoing so is.\n\n> At the moment there is the simple workaround of an alias that executes\n> that two step command dance to achieve what you needed, and Junio has\n> outlined the issues he needed to be covered from his maintainer\n> perspective (e.g. the detection of sparse checkouts). Confirming the\n> root causes would help in setting a baseline.\n>\n> I hope that is of help - I'd seen that the discussion had gone quiet.\n\nSome of the problems I have are:\n\n (1) \"git checkout -b NEW\", \"git checkout\", \"git checkout HEAD^0\"\n     and \"git checkout HEAD\" (no other parameters to any of them)\n     ought to give identical index and working tree.  It is too\n     confusing to leave subtly different results that will lead to\n     hard to diagnose bugs for only one of them.\n\n (2) The proposed log message talks only about \"performance\n     optimization\", while the purpose of the change is more about\n     changing the definition of what \"git checkout -b NEW\" is from\n     \"git branch NEW && git checkout NEW\" to \"git branch NEW && git\n     symbolic-ref HEAD refs/heads/NEW\".  The explanation in a Ben's\n     later message <007401d21278$445eba80$cd1c2f80$@gmail.com> does\n     a much better job contrasting the two.\n\n (3) I identified only one difference as an example sufficient to\n     point out why the patch provided is not a pure optimization but\n     behaviour change.  Fixing that example alone to avoid change in\n     the behaviour is trivial (see if the \"info/sparse-checkout\"\n     file is present and refrain from skipping the proper checkout),\n     but a much larger problem is that I do not know (and Ben does\n     not, I suspect) know what other behaviour changes the patch is\n     introducing, and worse, the checks are sufficiently dense too\n     detailed and intimate to the implementation of unpack_trees()\n     that it is impossible for anybody to make sure the exceptions\n     defined in this patch and updates to other parts of the system\n     will be kept in sync.\n\nSo my inclination at this point, unless we see somebody invents a\nclever way to solve (3), is that any change that violates (1),\ni.e. as long as the patch does \"Are we doing '-b NEW'?  Then we do\nsomething subtly different\", is not acceptable, and solving (3) in a\nmaintainable way smells like quite a hard thing to do.  But it would\nbe ideal if (3) is solved cleanly, as we will then not have to worry\nabout changing behaviour at all and can apply the optimization for\nall of the four cases equally.  As a side effect, that approach\nwould solve problem (2) above.\n\nIf we were to punt on keeping the sanity (1) and introduce a subtly\ndifferent \"create a new branch and point the HEAD at it\", an easier\nway out may be be one of\n\n 1. a totally new command, e.g. \"git branch-switch NEW\" that takes\n    only a single argument and no other \"checkout\" options, or\n\n 2. a new option to \"git checkout\" that takes _ONLY_ a single\n    argument and incompatible with any other option or command line\n    argument, or\n\n 3. an alias that does \"git branch\" followed by \"git symbolic-ref\".\n\nNeither of the first two sounds palatable, though.\n\n\n\n\n        \n"},{"id":"302487","messageId":"35DEF8289BD84B7AB5C3CD53E8BCF83B@PhilipOakley","threadId":"44072","inReplyTo":"xmqqd1jtyx01.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3] checkout: eliminate unnecessary merge for trivial checkout","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2016-09-24T19:31:09Z","receivedAt":"2016-09-24T19:31:23Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"Hi Junio,\n\nFrom: \"Junio C Hamano\" <gitster@pobox.com>\n> \"Philip Oakley\" <philipoakley@iee.org> writes:\n>\n>>> > >\"git checkout -b foo\" (without -f -m or <start_point>) is defined in\n>>> > >the manual as being a shortcut for/equivalent to:\n>>> > >\n>>> > >        (1a) \"git branch foo\"\n>>> > >        (1b) \"git checkout foo\"\n>>> > >\n>>> > >However, it has been our experience in our observed use cases and all\n>>> > >the existing git tests, that it can be treated as equivalent to:\n>>> > >\n>>> > >        (2a) \"git branch foo\"\n>>> > >        (2b) \"git symbolic-ref HEAD refs/heads/foo\"\n>>> > >...\n>>> > >\n>>> > I am still not sure if I like the change of what \"checkout -b\" is this\n>>> > late in the game, though.\n>>>\n>>> ...\n>>> That said, you're much more on the frontline of receiving negative\n>>> feedback about doing that than I am. :)  How would you like to\n>>> proceed?\n>>\n>> I didn't see an initial confirmation as to what the issue really\n>> was. You indicated the symptom ('a long checkout time'), but then we\n>> missed out on hard facts and example repos, so that the issue was\n>> replicable.\n>\n> I took it as a given, trivial and obvious optimization opportunity,\n> that it is wasteful having to traverse two trees to consolidate and\n> reflect their differences into the working tree when we know upfront\n> that these two trees are identical, no matter what the overhead for\n> doing so is.\n\nI agree, and I believe Ben agrees.\n\n>\n>> At the moment there is the simple workaround of an alias that executes\n>> that two step command dance to achieve what you needed, and Junio has\n>> outlined the issues he needed to be covered from his maintainer\n>> perspective (e.g. the detection of sparse checkouts). Confirming the\n>> root causes would help in setting a baseline.\n>>\n>> I hope that is of help - I'd seen that the discussion had gone quiet.\n>\n> Some of the problems I have are:\n>\n> (1) \"git checkout -b NEW\", \"git checkout\", \"git checkout HEAD^0\"\n>     and \"git checkout HEAD\" (no other parameters to any of them)\n>     ought to give identical index and working tree.  It is too\n>     confusing to leave subtly different results that will lead to\n>     hard to diagnose bugs for only one of them.\n>\n> (2) The proposed log message talks only about \"performance\n>     optimization\",\n\n>                                while the purpose of the change is more \n> about\n>     changing the definition\n\nHere I think is the misunderstanding. His purpose is NOT to change the \ndefinition (IIUC). As I read the message you reference below (and Ben's \nother messages), I understood that he was trying to achieve what you said \n(i.e. optimise the trivial and obvious opportunity) of selecting for the \ncommon case (underlying conditions) where the two command sequences are \nidentical. If the selected case / conditions is not identical then it is \ndefined wrongly...\n\nI suspect that it was Ben's 'soft' explanation that allowed the discussion \nto diverge.\n\n\n>                                                 of what \"git checkout -b \n> NEW\" is from\n>     \"git branch NEW && git checkout NEW\" to \"git branch NEW && git\n>     symbolic-ref HEAD refs/heads/NEW\".  The explanation in a Ben's\n>     later message <007401d21278$445eba80$cd1c2f80$@gmail.com> does\n>     a much better job contrasting the two.\n>\n> (3) I identified only one difference as an example sufficient to\n>     point out why the patch provided is not a pure optimization but\n>     behaviour change.  Fixing that example alone to avoid change in\n>     the behaviour is trivial (see if the \"info/sparse-checkout\"\n>     file is present and refrain from skipping the proper checkout),\n\nThis is probably the point Ben needs to take on board to narrow the \nconditions down. There may be others.\n\n>     but a much larger problem is that I do not know (and Ben does\n>     not, I suspect) know what other behaviour changes the patch is\n>     introducing, and worse, the checks are sufficiently dense too\n>     detailed and intimate to the implementation of unpack_trees()\n>     that it is impossible for anybody to make sure the exceptions\n>     defined in this patch and updates to other parts of the system\n>     will be kept in sync.\n\nI did not believe he was proposing such a change to behaviour, hence his \ndifficulty in responding (or at least that is my perception). I.e. he was \ndigging a hole in the wrong place.\n\nIt is possible that he had accidentally introduced a behavious change, and \nhaving failed to explictly say \"This patch (should) produces no behavious \nchange\", which then continued to re-inforce the misunderstanding.\n\n>\n> So my inclination at this point, unless we see somebody invents a\n> clever way to solve (3), is that any change that violates (1),\n> i.e. as long as the patch does \"Are we doing '-b NEW'?  Then we do\n> something subtly different\", is not acceptable, and solving (3) in a\n> maintainable way smells like quite a hard thing to do.  But it would\n> be ideal if (3) is solved cleanly, as we will then not have to worry\n> about changing behaviour at all and can apply the optimization for\n> all of the four cases equally.  As a side effect, that approach\n> would solve problem (2) above.\n>\n> If we were to punt on keeping the sanity (1) and introduce a subtly\n> different \"create a new branch and point the HEAD at it\", an easier\n> way out may be be one of\n>\n> 1. a totally new command, e.g. \"git branch-switch NEW\" that takes\n>    only a single argument and no other \"checkout\" options, or\n>\n> 2. a new option to \"git checkout\" that takes _ONLY_ a single\n>    argument and incompatible with any other option or command line\n>    argument, or\n>\n> 3. an alias that does \"git branch\" followed by \"git symbolic-ref\".\n>\n> Neither of the first two sounds palatable, though.\n\nIt will need Ben to come back and clarify, if he did, or did not, want any \nbehaviour change (beyond speed of action;-)\n\nThanks\n\nPhilip \n\n"}]}