{"thread":{"id":"39472","subject":"[PATCH] checkout: don't check worktrees when not necessary","startedAt":"2015-05-31T23:16:29Z","lastAt":"2015-06-12T22:24:37Z","messageCount":5,"participants":["Spencer Baugh","Duy Nguyen","Nguyễn Thái Ngọc Duy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"262548","messageId":"1433114189-9779-1-git-send-email-sbaugh@catern.com","threadId":"39472","inReplyTo":null,"subject":"[PATCH] checkout: don't check worktrees when not necessary","fromName":"Spencer Baugh","fromEmail":"sbaugh@catern.com","sentAt":"2015-05-31T23:16:29Z","receivedAt":"2015-05-31T23:16:29Z","isPatch":true,"sender":{"key":"sbaugh@catern.com","avatar":null},"body":"When --patch or pathspecs are passed to git checkout, the working tree\nwill not be switching branch, so there's no need to check if the branch\nthat we are running checkout on is already checked out.\n\nSigned-off-by: Spencer Baugh <sbaugh@catern.com>\n---\n builtin/checkout.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 2f92328..7039c5c 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1237,6 +1237,7 @@ static int parse_branchname_arg(int argc, const char **argv,\n \t\tchar *head_ref = resolve_refdup(\"HEAD\", 0, sha1, &flag);\n \t\tif (head_ref &&\n \t\t    (!(flag & REF_ISSYMREF) || strcmp(head_ref, new->path)) &&\n+\t\t    !(opts->patch_mode || opts->pathspec.nr) &&\n \t\t    !opts->ignore_other_worktrees)\n \t\t\tcheck_linked_checkouts(new);\n \t\tfree(head_ref);\n-- \n2.4.2.339.g77bd3ea\n"},{"id":"262576","messageId":"20150601102317.GA20607@lanh","threadId":"39472","inReplyTo":"1433114189-9779-1-git-send-email-sbaugh@catern.com","subject":"Re: [PATCH] checkout: don't check worktrees when not necessary","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-06-01T10:23:17Z","receivedAt":"2015-06-01T10:23:17Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, May 31, 2015 at 07:16:29PM -0400, Spencer Baugh wrote:\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -1237,6 +1237,7 @@ static int parse_branchname_arg(int argc, const char **argv,\n>  \t\tchar *head_ref = resolve_refdup(\"HEAD\", 0, sha1, &flag);\n>  \t\tif (head_ref &&\n>  \t\t    (!(flag & REF_ISSYMREF) || strcmp(head_ref, new->path)) &&\n> +\t\t    !(opts->patch_mode || opts->pathspec.nr) &&\n>  \t\t    !opts->ignore_other_worktrees)\n>  \t\t\tcheck_linked_checkouts(new);\n>  \t\tfree(head_ref);\n\nSimple and effective. But if in future we add more options for\nnon-switching-branch checkout, we need to update both places, here and\nnear the end of cmd_checkout().\n\nPerhaps we can move all this block inside checkout_branch() so we only\nneed to test \"opts->patch_mode || opts->pathspec.nr\" once, at the end\nof cmd_checkout(). Something like below?\n\nI'm not opposed to your change, but if you go with it, you should\ncherry pick my test in the below patch. Or create a similar test.\n\n-- 8< --\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 2f92328..e9aee58 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1110,7 +1110,6 @@ static int parse_branchname_arg(int argc, const char **argv,\n {\n \tstruct tree **source_tree = &opts->source_tree;\n \tconst char **new_branch = &opts->new_branch;\n-\tint force_detach = opts->force_detach;\n \tint argcount = 0;\n \tunsigned char branch_rev[20];\n \tconst char *arg;\n@@ -1231,17 +1230,6 @@ static int parse_branchname_arg(int argc, const char **argv,\n \telse\n \t\tnew->path = NULL; /* not an existing branch */\n \n-\tif (new->path && !force_detach && !*new_branch) {\n-\t\tunsigned char sha1[20];\n-\t\tint flag;\n-\t\tchar *head_ref = resolve_refdup(\"HEAD\", 0, sha1, &flag);\n-\t\tif (head_ref &&\n-\t\t    (!(flag & REF_ISSYMREF) || strcmp(head_ref, new->path)) &&\n-\t\t    !opts->ignore_other_worktrees)\n-\t\t\tcheck_linked_checkouts(new);\n-\t\tfree(head_ref);\n-\t}\n-\n \tnew->commit = lookup_commit_reference_gently(rev, 1);\n \tif (!new->commit) {\n \t\t/* not a commit */\n@@ -1321,6 +1309,17 @@ static int checkout_branch(struct checkout_opts *opts,\n \t\tdie(_(\"Cannot switch branch to a non-commit '%s'\"),\n \t\t    new->name);\n \n+\tif (new->path && !opts->force_detach && !opts->new_branch) {\n+\t\tunsigned char sha1[20];\n+\t\tint flag;\n+\t\tchar *head_ref = resolve_refdup(\"HEAD\", 0, sha1, &flag);\n+\t\tif (head_ref &&\n+\t\t    (!(flag & REF_ISSYMREF) || strcmp(head_ref, new->path)) &&\n+\t\t    !opts->ignore_other_worktrees)\n+\t\t\tcheck_linked_checkouts(new);\n+\t\tfree(head_ref);\n+\t}\n+\n \tif (opts->new_worktree)\n \t\treturn prepare_linked_checkout(opts, new);\n \ndiff --git a/t/t2025-checkout-to.sh b/t/t2025-checkout-to.sh\nindex f8e4df4..a8d9336 100755\n--- a/t/t2025-checkout-to.sh\n+++ b/t/t2025-checkout-to.sh\n@@ -28,6 +28,14 @@ test_expect_success 'checkout --to refuses to checkout locked branch' '\n \t! test -d .git/worktrees/zere\n '\n \n+test_expect_success 'checking out paths not complaining about linked checkouts' '\n+\t(\n+\tcd existing_empty &&\n+\techo dirty >>init.t &&\n+\tgit checkout master -- init.t\n+\t)\n+'\n+\n test_expect_success 'checkout --to a new worktree' '\n \tgit rev-parse HEAD >expect &&\n \tgit checkout --detach --to here master &&\n-- 8< --\n--\nDuy\n"},{"id":"263526","messageId":"87zj47gyif.fsf@earth.i-did-not-set--mail-host-address--so-tickle-me","threadId":"39472","inReplyTo":"20150601102317.GA20607@lanh","subject":"Re: [PATCH] checkout: don't check worktrees when not necessary","fromName":"Spencer Baugh","fromEmail":"catern@catern.com","sentAt":"2015-06-10T19:21:44Z","receivedAt":"2015-06-10T19:21:44Z","isPatch":true,"sender":{"key":"catern@catern.com","avatar":null},"body":"\nDuy Nguyen <pclouds@gmail.com> writes:\n> On Sun, May 31, 2015 at 07:16:29PM -0400, Spencer Baugh wrote:\n>> --- a/builtin/checkout.c\n>> +++ b/builtin/checkout.c\n>> @@ -1237,6 +1237,7 @@ static int parse_branchname_arg(int argc, const char **argv,\n>>  \t\tchar *head_ref = resolve_refdup(\"HEAD\", 0, sha1, &flag);\n>>  \t\tif (head_ref &&\n>>  \t\t    (!(flag & REF_ISSYMREF) || strcmp(head_ref, new->path)) &&\n>> +\t\t    !(opts->patch_mode || opts->pathspec.nr) &&\n>>  \t\t    !opts->ignore_other_worktrees)\n>>  \t\t\tcheck_linked_checkouts(new);\n>>  \t\tfree(head_ref);\n>\n> Simple and effective. But if in future we add more options for\n> non-switching-branch checkout, we need to update both places, here and\n> near the end of cmd_checkout().\n>\n> Perhaps we can move all this block inside checkout_branch() so we only\n> need to test \"opts->patch_mode || opts->pathspec.nr\" once, at the end\n> of cmd_checkout(). Something like below?\n>\n> I'm not opposed to your change, but if you go with it, you should\n> cherry pick my test in the below patch. Or create a similar test.\n\nSorry for late reply, but I think your change is much better than mine\nso I'd suggest just using that instead.\n"},{"id":"263654","messageId":"1434106164-28699-1-git-send-email-pclouds@gmail.com","threadId":"39472","inReplyTo":"87zj47gyif.fsf@earth.i-did-not-set--mail-host-address--so-tickle-me","subject":"[PATCH] checkout: don't check worktrees when not necessary","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-06-12T10:49:24Z","receivedAt":"2015-06-12T10:49:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"When --patch or pathspecs are passed to git checkout, the working tree\nwill not be switching branch, so there's no need to check if the branch\nthat we are running checkout on is already checked out.\n\nOriginal-patch-by: Spencer Baugh <sbaugh@catern.com>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/checkout.c     | 23 +++++++++++------------\n t/t2025-checkout-to.sh |  8 ++++++++\n 2 files changed, 19 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 9b49f0e..e227f64 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1110,7 +1110,6 @@ static int parse_branchname_arg(int argc, const char **argv,\n {\n \tstruct tree **source_tree = &opts->source_tree;\n \tconst char **new_branch = &opts->new_branch;\n-\tint force_detach = opts->force_detach;\n \tint argcount = 0;\n \tunsigned char branch_rev[20];\n \tconst char *arg;\n@@ -1231,17 +1230,6 @@ static int parse_branchname_arg(int argc, const char **argv,\n \telse\n \t\tnew->path = NULL; /* not an existing branch */\n \n-\tif (new->path && !force_detach && !*new_branch) {\n-\t\tunsigned char sha1[20];\n-\t\tint flag;\n-\t\tchar *head_ref = resolve_refdup(\"HEAD\", 0, sha1, &flag);\n-\t\tif (head_ref &&\n-\t\t    (!(flag & REF_ISSYMREF) || strcmp(head_ref, new->path)) &&\n-\t\t    !opts->ignore_other_worktrees)\n-\t\t\tcheck_linked_checkouts(new);\n-\t\tfree(head_ref);\n-\t}\n-\n \tnew->commit = lookup_commit_reference_gently(rev, 1);\n \tif (!new->commit) {\n \t\t/* not a commit */\n@@ -1321,6 +1309,17 @@ static int checkout_branch(struct checkout_opts *opts,\n \t\tdie(_(\"Cannot switch branch to a non-commit '%s'\"),\n \t\t    new->name);\n \n+\tif (new->path && !opts->force_detach && !opts->new_branch) {\n+\t\tunsigned char sha1[20];\n+\t\tint flag;\n+\t\tchar *head_ref = resolve_refdup(\"HEAD\", 0, sha1, &flag);\n+\t\tif (head_ref &&\n+\t\t    (!(flag & REF_ISSYMREF) || strcmp(head_ref, new->path)) &&\n+\t\t    !opts->ignore_other_worktrees)\n+\t\t\tcheck_linked_checkouts(new);\n+\t\tfree(head_ref);\n+\t}\n+\n \tif (opts->new_worktree)\n \t\treturn prepare_linked_checkout(opts, new);\n \ndiff --git a/t/t2025-checkout-to.sh b/t/t2025-checkout-to.sh\nindex f8e4df4..a8d9336 100755\n--- a/t/t2025-checkout-to.sh\n+++ b/t/t2025-checkout-to.sh\n@@ -28,6 +28,14 @@ test_expect_success 'checkout --to refuses to checkout locked branch' '\n \t! test -d .git/worktrees/zere\n '\n \n+test_expect_success 'checking out paths not complaining about linked checkouts' '\n+\t(\n+\tcd existing_empty &&\n+\techo dirty >>init.t &&\n+\tgit checkout master -- init.t\n+\t)\n+'\n+\n test_expect_success 'checkout --to a new worktree' '\n \tgit rev-parse HEAD >expect &&\n \tgit checkout --detach --to here master &&\n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"263713","messageId":"xmqqr3pgbm56.fsf@gitster.dls.corp.google.com","threadId":"39472","inReplyTo":"1434106164-28699-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] checkout: don't check worktrees when not necessary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-12T22:24:37Z","receivedAt":"2015-06-12T22:24:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> When --patch or pathspecs are passed to git checkout, the working tree\n> will not be switching branch, so there's no need to check if the branch\n> that we are running checkout on is already checked out.\n\nYeah, I agree that having this check in parse_branchname_arg() does\nnot make any sense, as that function is still trying to decide if we\nare switching to a different branch or not.  The new location looks\nmuch more sensible.\n\nWill queue this on nd/multiple-work-trees (which has been in\n'master' for about a month).  Good thing that we caught it before it\ngot in any tagged release.\n\nThanks.\n\n>\n> Original-patch-by: Spencer Baugh <sbaugh@catern.com>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  builtin/checkout.c     | 23 +++++++++++------------\n>  t/t2025-checkout-to.sh |  8 ++++++++\n>  2 files changed, 19 insertions(+), 12 deletions(-)\n>\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 9b49f0e..e227f64 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -1110,7 +1110,6 @@ static int parse_branchname_arg(int argc, const char **argv,\n>  {\n>  \tstruct tree **source_tree = &opts->source_tree;\n>  \tconst char **new_branch = &opts->new_branch;\n> -\tint force_detach = opts->force_detach;\n>  \tint argcount = 0;\n>  \tunsigned char branch_rev[20];\n>  \tconst char *arg;\n> @@ -1231,17 +1230,6 @@ static int parse_branchname_arg(int argc, const char **argv,\n>  \telse\n>  \t\tnew->path = NULL; /* not an existing branch */\n>  \n> -\tif (new->path && !force_detach && !*new_branch) {\n> -\t\tunsigned char sha1[20];\n> -\t\tint flag;\n> -\t\tchar *head_ref = resolve_refdup(\"HEAD\", 0, sha1, &flag);\n> -\t\tif (head_ref &&\n> -\t\t    (!(flag & REF_ISSYMREF) || strcmp(head_ref, new->path)) &&\n> -\t\t    !opts->ignore_other_worktrees)\n> -\t\t\tcheck_linked_checkouts(new);\n> -\t\tfree(head_ref);\n> -\t}\n> -\n>  \tnew->commit = lookup_commit_reference_gently(rev, 1);\n>  \tif (!new->commit) {\n>  \t\t/* not a commit */\n> @@ -1321,6 +1309,17 @@ static int checkout_branch(struct checkout_opts *opts,\n>  \t\tdie(_(\"Cannot switch branch to a non-commit '%s'\"),\n>  \t\t    new->name);\n>  \n> +\tif (new->path && !opts->force_detach && !opts->new_branch) {\n> +\t\tunsigned char sha1[20];\n> +\t\tint flag;\n> +\t\tchar *head_ref = resolve_refdup(\"HEAD\", 0, sha1, &flag);\n> +\t\tif (head_ref &&\n> +\t\t    (!(flag & REF_ISSYMREF) || strcmp(head_ref, new->path)) &&\n> +\t\t    !opts->ignore_other_worktrees)\n> +\t\t\tcheck_linked_checkouts(new);\n> +\t\tfree(head_ref);\n> +\t}\n> +\n>  \tif (opts->new_worktree)\n>  \t\treturn prepare_linked_checkout(opts, new);\n>  \n> diff --git a/t/t2025-checkout-to.sh b/t/t2025-checkout-to.sh\n> index f8e4df4..a8d9336 100755\n> --- a/t/t2025-checkout-to.sh\n> +++ b/t/t2025-checkout-to.sh\n> @@ -28,6 +28,14 @@ test_expect_success 'checkout --to refuses to checkout locked branch' '\n>  \t! test -d .git/worktrees/zere\n>  '\n>  \n> +test_expect_success 'checking out paths not complaining about linked checkouts' '\n> +\t(\n> +\tcd existing_empty &&\n> +\techo dirty >>init.t &&\n> +\tgit checkout master -- init.t\n> +\t)\n> +'\n> +\n>  test_expect_success 'checkout --to a new worktree' '\n>  \tgit rev-parse HEAD >expect &&\n>  \tgit checkout --detach --to here master &&\n"}]}