{"thread":{"id":"52368","subject":"[PATCH 1/5] parse_branchname_arg(): fix dash_dash_pos, drop argcount","startedAt":"2019-11-28T19:32:26Z","lastAt":"2019-12-19T18:03:26Z","messageCount":10,"participants":["Alexandr Miloslavskiy via GitGitGadget","Junio C Hamano","Alexandr Miloslavskiy"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"387285","messageId":"771947d01f759a72b9f8999e929bf1f7765327aa.1574969538.git.gitgitgadget@gmail.com","threadId":"52368","inReplyTo":"pull.479.git.1574969538.gitgitgadget@gmail.com","subject":"[PATCH 1/5] parse_branchname_arg(): fix dash_dash_pos, drop argcount","fromName":"Alexandr Miloslavskiy via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-28T19:32:14Z","receivedAt":"2019-11-28T19:32:26Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"From: Alexandr Miloslavskiy \n\n`dash_dash_pos` was only calculated under `opts->accept_pathspec`. This\nis unexpected to readers and made it harder to reason about the code.\nFix this by restoring the expected meaning.\n\nSimplify the code by dropping `argcount` and useless `argc` / `argv`\nmanipulations.\n\nThis should not change behavior in any way.\n\nSigned-off-by: Alexandr Miloslavskiy \n---\n builtin/checkout.c | 34 ++++++++++++++--------------------\n 1 file changed, 14 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 3634a3dac1..655b389756 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1121,7 +1121,6 @@ static int parse_branchname_arg(int argc, const char **argv,\n \t\t\t\tint *dwim_remotes_matched)\n {\n \tconst char **new_branch = &opts->new_branch;\n-\tint argcount = 0;\n \tconst char *arg;\n \tint dash_dash_pos;\n \tint has_dash_dash = 0;\n@@ -1180,17 +1179,21 @@ static int parse_branchname_arg(int argc, const char **argv,\n \targ = argv[0];\n \tdash_dash_pos = -1;\n \tfor (i = 0; i < argc; i++) {\n-\t\tif (opts->accept_pathspec && !strcmp(argv[i], \"--\")) {\n+\t\tif (!strcmp(argv[i], \"--\")) {\n \t\t\tdash_dash_pos = i;\n \t\t\tbreak;\n \t\t}\n \t}\n-\tif (dash_dash_pos == 0)\n-\t\treturn 1; /* case (2) */\n-\telse if (dash_dash_pos == 1)\n-\t\thas_dash_dash = 1; /* case (3) or (1) */\n-\telse if (dash_dash_pos >= 2)\n-\t\tdie(_(\"only one reference expected, %d given.\"), dash_dash_pos);\n+\n+\tif (opts->accept_pathspec) {\n+\t if (dash_dash_pos == 0)\n+\t\t return 1; /* case (2) */\n+\t else if (dash_dash_pos == 1)\n+\t\t has_dash_dash = 1; /* case (3) or (1) */\n+\t else if (dash_dash_pos >= 2)\n+\t\t die(_(\"only one reference expected, %d given.\"), dash_dash_pos);\n+\t}\n+\n \topts->count_checkout_paths = !opts->quiet && !has_dash_dash;\n \n \tif (!strcmp(arg, \"-\"))\n@@ -1241,15 +1244,10 @@ static int parse_branchname_arg(int argc, const char **argv,\n \t\tif (!recover_with_dwim) {\n \t\t\tif (has_dash_dash)\n \t\t\t\tdie(_(\"invalid reference: %s\"), arg);\n-\t\t\treturn argcount;\n+\t\t\treturn 0;\n \t\t}\n \t}\n \n-\t/* we can't end up being in (2) anymore, eat the argument */\n-\targcount++;\n-\targv++;\n-\targc--;\n-\n \tsetup_new_branch_info_and_source_tree(new_branch_info, opts, rev, arg);\n \n \tif (!opts->source_tree) /* case (1): want a tree */\n@@ -1262,15 +1260,11 @@ static int parse_branchname_arg(int argc, const char **argv,\n \t\t * even if there happen to be a file called 'branch';\n \t\t * it would be extremely annoying.\n \t\t */\n-\t\tif (argc)\n+\t\tif (argc > 1)\n \t\t\tverify_non_filename(opts->prefix, arg);\n-\t} else if (opts->accept_pathspec) {\n-\t\targcount++;\n-\t\targv++;\n-\t\targc--;\n \t}\n \n-\treturn argcount;\n+\treturn (dash_dash_pos == 1) ? 2 : 1;\n }\n \n static int switch_unborn_to_new_branch(const struct checkout_opts *opts)\n-- \ngitgitgadget\n\n"},{"id":"387286","messageId":"a84633a44474aa25bd1101a9ca2a5d9687900bf2.1574969538.git.gitgitgadget@gmail.com","threadId":"52368","inReplyTo":"pull.479.git.1574969538.gitgitgadget@gmail.com","subject":"[PATCH 2/5] parse_branchname_arg(): introduce expect_commit_only","fromName":"Alexandr Miloslavskiy via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-28T19:32:15Z","receivedAt":"2019-11-28T19:32:26Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"From: Alexandr Miloslavskiy \n\n`has_dash_dash` unexpectedly takes `opts->accept_pathspec` into account.\nWhile this may sound clever at first sight, it becomes pretty hard to\nreason (and not be a victim) about code, especially in combination with\n`argc` here:\n\n\tif (!(argc == 1 && !has_dash_dash) &&\n\t !(argc == 2 && has_dash_dash) &&\n\t opts->accept_pathspec)\n\t\trecover_with_dwim = 0;\n\nIntroduce a new non-obfuscated variable to reduce the amount of diffs in\nnext patch.\n\nThis should not change behavior in any way.\n\nSigned-off-by: Alexandr Miloslavskiy \n---\n builtin/checkout.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 655b389756..5c6131dbe6 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1123,7 +1123,7 @@ static int parse_branchname_arg(int argc, const char **argv,\n \tconst char **new_branch = &opts->new_branch;\n \tconst char *arg;\n \tint dash_dash_pos;\n-\tint has_dash_dash = 0;\n+\tint has_dash_dash = 0, expect_commit_only = 0;\n \tint i;\n \n \t/*\n@@ -1194,7 +1194,10 @@ static int parse_branchname_arg(int argc, const char **argv,\n \t\t die(_(\"only one reference expected, %d given.\"), dash_dash_pos);\n \t}\n \n-\topts->count_checkout_paths = !opts->quiet && !has_dash_dash;\n+\tif (has_dash_dash)\n+\t expect_commit_only = 1;\n+\n+\topts->count_checkout_paths = !opts->quiet && !expect_commit_only;\n \n \tif (!strcmp(arg, \"-\"))\n \t\targ = \"@{-1}\";\n@@ -1210,10 +1213,10 @@ static int parse_branchname_arg(int argc, const char **argv,\n \t\t */\n \t\tint recover_with_dwim = dwim_new_local_branch_ok;\n \n-\t\tint could_be_checkout_paths = !has_dash_dash &&\n+\t\tint could_be_checkout_paths = !expect_commit_only &&\n \t\t\tcheck_filename(opts->prefix, arg);\n \n-\t\tif (!has_dash_dash && !no_wildcard(arg))\n+\t\tif (!expect_commit_only && !no_wildcard(arg))\n \t\t\trecover_with_dwim = 0;\n \n \t\t/*\n@@ -1242,7 +1245,7 @@ static int parse_branchname_arg(int argc, const char **argv,\n \t\t}\n \n \t\tif (!recover_with_dwim) {\n-\t\t\tif (has_dash_dash)\n+\t\t\tif (expect_commit_only)\n \t\t\t\tdie(_(\"invalid reference: %s\"), arg);\n \t\t\treturn 0;\n \t\t}\n@@ -1253,7 +1256,7 @@ static int parse_branchname_arg(int argc, const char **argv,\n \tif (!opts->source_tree) /* case (1): want a tree */\n \t\tdie(_(\"reference is not a tree: %s\"), arg);\n \n-\tif (!has_dash_dash) {\t/* case (3).(d) -> (1) */\n+\tif (!expect_commit_only) {\t/* case (3).(d) -> (1) */\n \t\t/*\n \t\t * Do not complain the most common case\n \t\t *\tgit checkout branch\n-- \ngitgitgadget\n\n"},{"id":"387287","messageId":"pull.479.git.1574969538.gitgitgadget@gmail.com","threadId":"52368","inReplyTo":null,"subject":"[PATCH 0/5] parse_branchname_arg(): make code easier to understand","fromName":"Alexandr Miloslavskiy via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-28T19:32:13Z","receivedAt":"2019-11-28T19:32:26Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"My bigger goal is to complete my --pathspec-from-file series of patches.\n\nFor this, I needed to evaluate whether parse_branchname_arg() heuristics\nneeds any changes when pathspec is passed, but NOT in args.\n\nI found it surprisingly hard to reason about the code in this function. This\nmostly happens due to \"obfuscated\" variables, where they have a clear name\nand a different actual meaning. Ultimately, it was hard to mentally expand\nthem to true meaning AND see all possible combinations of branches at once.\n\nI have split this refactoring in 4 patches, so that diffs are not too big in\nevery single patch.\n\nTo my understanding, there should be no changes in git's behavior, except\nfor a couple better die() messages.\n\nAlexandr Miloslavskiy (5):\n parse_branchname_arg(): fix dash_dash_pos, drop argcount\n parse_branchname_arg(): introduce expect_commit_only\n parse_branchname_arg(): update code comments\n parse_branchname_arg(): refactor the decision making\n t2024: cover more cases\n\n builtin/checkout.c | 174 ++++++++++++++++-----------------------\n t/t2024-checkout-dwim.sh | 27 ++++++\n 2 files changed, 97 insertions(+), 104 deletions(-)\n\n\nbase-commit: d9f6f3b6195a0ca35642561e530798ad1469bd41\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-479%2FSyntevoAlex%2F%230225(git)_refactor_parse_branchname_arg-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-479/SyntevoAlex/#0225(git)_refactor_parse_branchname_arg-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/479\n-- \ngitgitgadget\n"},{"id":"387288","messageId":"ded493ee802805f831a498aea43e3e72a16c21bc.1574969538.git.gitgitgadget@gmail.com","threadId":"52368","inReplyTo":"pull.479.git.1574969538.gitgitgadget@gmail.com","subject":"[PATCH 5/5] t2024: cover more cases","fromName":"Alexandr Miloslavskiy via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-28T19:32:18Z","receivedAt":"2019-11-28T19:32:27Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"From: Alexandr Miloslavskiy \n\nSigned-off-by: Alexandr Miloslavskiy \n---\n t/t2024-checkout-dwim.sh | 27 +++++++++++++++++++++++++++\n 1 file changed, 27 insertions(+)\n\ndiff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh\nindex fa0718c730..2fe77387a6 100755\n--- a/t/t2024-checkout-dwim.sh\n+++ b/t/t2024-checkout-dwim.sh\n@@ -132,6 +132,33 @@ test_expect_success 'checkout of branch from a single remote succeeds #2' '\n \ttest_branch_upstream baz repo_b baz\n '\n \n+test_expect_success 'checkout of branch from a single remote succeeds with --' '\n+\tgit checkout -B master &&\n+\ttest_might_fail git branch -D baz &&\n+\n+\tgit checkout baz -- &&\n+\tstatus_uno_is_clean &&\n+\ttest_branch baz &&\n+\ttest_cmp_rev remotes/other_b/baz HEAD &&\n+\ttest_branch_upstream baz repo_b baz\n+'\n+\n+test_expect_success 'dont DWIM with pathspec #1' '\n+\tgit checkout -B master &&\n+\ttest_might_fail git branch -D baz &&\n+\n+\ttest_must_fail git checkout baz nonExistingFile 2>err &&\n+\ttest_i18ngrep \"did not match any file(s) known to git\" err\n+'\n+\n+test_expect_success 'dont DWIM with pathspec #2' '\n+\tgit checkout -B master &&\n+\ttest_might_fail git branch -D baz &&\n+\n+\ttest_must_fail git checkout baz -- nonExistingFile 2>err &&\n+\ttest_i18ngrep \"fatal: invalid reference: baz\" err\n+'\n+\n test_expect_success '--no-guess suppresses branch auto-vivification' '\n \tgit checkout -B master &&\n \tstatus_uno_is_clean &&\n-- \ngitgitgadget\n"},{"id":"387290","messageId":"45db345304d5771017026a32d96251c5c5c47f11.1574969538.git.gitgitgadget@gmail.com","threadId":"52368","inReplyTo":"pull.479.git.1574969538.gitgitgadget@gmail.com","subject":"[PATCH 4/5] parse_branchname_arg(): refactor the decision making","fromName":"Alexandr Miloslavskiy via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-28T19:32:17Z","receivedAt":"2019-11-28T19:32:27Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"From: Alexandr Miloslavskiy \n\nMake it easier to understand which branches handle which cases.\n\nDrop obfuscated variable `has_dash_dash` which also took\n`opts->accept_pathspec` into account, making it pretty hard to reason\nabout code, especially when used together with `argc` and\n`opts->accept_pathspec` here:\n\n\tif (!(argc == 1 && !has_dash_dash) &&\n\t\t!(argc == 2 && has_dash_dash) &&\n\t\topts->accept_pathspec)\n\t\trecover_with_dwim = 0;\n\nAvoid double-negation in the code mentioned above (\"it is not OK to\nproceed if it's not one of those cases\").\n\nAvoid hard-to-understand condition `opts->accept_pathspec` in the code\nmentioned above.\n\nThere are some minor die() message changes for:\n`git switch `\n`git switch -- `\n\nSigned-off-by: Alexandr Miloslavskiy \n---\n builtin/checkout.c | 71 +++++++++++++++++++++++++++-------------------\n 1 file changed, 42 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 723aaca0ef..8f679d1b6a 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1122,9 +1122,8 @@ static int parse_branchname_arg(int argc, const char **argv,\n {\n \tconst char **new_branch = &opts->new_branch;\n \tconst char *arg;\n-\tint dash_dash_pos;\n-\tint has_dash_dash = 0, expect_commit_only = 0;\n-\tint i;\n+\tint dash_dash_pos, i;\n+\tint recover_with_dwim, expect_commit_only;\n \n \t/*\n \t * Resolve ambiguity where argv[0] may be or .\n@@ -1143,15 +1142,7 @@ static int parse_branchname_arg(int argc, const char **argv,\n \t * do that and proceed with (2)(3).\n \t * 5) Otherwise, let caller proceed with interpretation.\n \t */\n-\tif (!argc)\n-\t\treturn 0;\n-\n-\tif (!opts->accept_pathspec) {\n-\t\tif (argc > 1)\n-\t\t\tdie(_(\"only one reference expected\"));\n-\t\thas_dash_dash = 1; /* helps disambiguate */\n-\t}\n-\n+\t \n \targ = argv[0];\n \tdash_dash_pos = -1;\n \tfor (i = 0; i < argc; i++) {\n@@ -1161,17 +1152,46 @@ static int parse_branchname_arg(int argc, const char **argv,\n \t\t}\n \t}\n \n-\tif (opts->accept_pathspec) {\n-\t if (dash_dash_pos == 0)\n-\t\t return 1;\n-\t else if (dash_dash_pos == 1)\n-\t\t has_dash_dash = 1;\n-\t else if (dash_dash_pos >= 2)\n-\t\t die(_(\"only one reference expected, %d given.\"), dash_dash_pos);\n-\t}\n+\tif (dash_dash_pos == -1) {\n+\t\tif (argc == 0) {\n+\t\t\t/* 'git checkout/switch/restore' */\n+\t\t\treturn 0;\n+\t\t} else if (argc == 1) {\n+\t\t\t/* 'git checkout/switch/restore ' */\n+\t\t\trecover_with_dwim = dwim_new_local_branch_ok;\n+\t\t} else if (!opts->accept_pathspec) {\n+\t\t\t/* 'git switch [...]' */\n+\t\t\tdie(_(\"only one reference expected, %d given.\"), argc);\n+\t\t} else {\n+\t\t\t/* 'git checkout/restore [...]' */\n+\t\t\trecover_with_dwim = 0;\n+\t\t}\n+\n+\t\t/* Absence of '--' leaves / ambiguity */\n+\t\texpect_commit_only = !opts->accept_pathspec;\n+\t} else if (dash_dash_pos == 0) {\n+\t\t/* 'git checkout/switch/restore -- [...]' */\n+\t\treturn 1; /* Eat '--' */\n+\t} else if (dash_dash_pos == 1) {\n+\t\tif (!opts->accept_pathspec) {\n+\t\t\t/* 'git switch -- [...]' */\n+\t\t\tdie(_(\"incompatible with pathspec arguments\"));\n+\t\t}\n \n-\tif (has_dash_dash)\n-\t expect_commit_only = 1;\n+\t\tif (argc == 2) {\n+\t\t\t/* 'git checkout/restore --' */\n+\t\t\trecover_with_dwim = dwim_new_local_branch_ok;\n+\t\t} else {\n+\t\t\t/* 'git checkout/restore -- [...]' */\n+\t\t\trecover_with_dwim = 0;\n+\t\t}\n+\n+\t\t/* Presence of '--' makes it certain that arg is */\n+\t\texpect_commit_only = 1;\n+\t} else {\n+\t\t/* 'git checkout/switch/restore [...] -- [...]' */\n+\t\tdie(_(\"only one reference expected, %d given.\"), dash_dash_pos);\n+\t}\n \n \topts->count_checkout_paths = !opts->quiet && !expect_commit_only;\n \n@@ -1179,19 +1199,12 @@ static int parse_branchname_arg(int argc, const char **argv,\n \t\targ = \"@{-1}\";\n \n \tif (get_oid_mb(arg, rev)) {\n-\t\tint recover_with_dwim = dwim_new_local_branch_ok;\n-\n \t\tint could_be_checkout_paths = !expect_commit_only &&\n \t\t\tcheck_filename(opts->prefix, arg);\n \n \t\tif (!expect_commit_only && !no_wildcard(arg))\n \t\t\trecover_with_dwim = 0;\n \n-\t\tif (!(argc == 1 && !has_dash_dash) &&\n-\t\t !(argc == 2 && has_dash_dash) &&\n-\t\t opts->accept_pathspec)\n-\t\t\trecover_with_dwim = 0;\n-\n \t\tif (recover_with_dwim) {\n \t\t\tconst char *remote = unique_tracking_name(arg, rev,\n \t\t\t\t\t\t\t\t dwim_remotes_matched);\n-- \ngitgitgadget\n\n"},{"id":"387289","messageId":"3918aafdbd6ae63ddb9b4ea7328b9ec2e0f76fe9.1574969538.git.gitgitgadget@gmail.com","threadId":"52368","inReplyTo":"pull.479.git.1574969538.gitgitgadget@gmail.com","subject":"[PATCH 3/5] parse_branchname_arg(): update code comments","fromName":"Alexandr Miloslavskiy via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-28T19:32:16Z","receivedAt":"2019-11-28T19:32:28Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"From: Alexandr Miloslavskiy \n\nThese parts repeat git documentation:\n ... if is A...B <...>\n ... remote named in checkout.defaultRemote ...\n\nSome parts repeat the code below. With next patch, code will be easier\nto understand, so this is no longer needed.\n\nThis is a separate patch to reduce the amount of diffs in next patch.\n\nSigned-off-by: Alexandr Miloslavskiy \n---\n builtin/checkout.c | 86 +++++++++++-----------------------------------\n 1 file changed, 21 insertions(+), 65 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 5c6131dbe6..723aaca0ef 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1127,45 +1127,21 @@ static int parse_branchname_arg(int argc, const char **argv,\n \tint i;\n \n \t/*\n-\t * case 1: git checkout -- []\n-\t *\n-\t * must be a valid tree, everything after the '--' must be\n-\t * a path.\n-\t *\n-\t * case 2: git checkout -- []\n-\t *\n-\t * everything after the '--' must be paths.\n-\t *\n-\t * case 3: git checkout [--]\n-\t *\n-\t * (a) If is a commit, that is to\n-\t * switch to the branch or detach HEAD at it. As a special case,\n-\t * if is A...B (missing A or B means HEAD but you can\n-\t * omit at most one side), and if there is a unique merge base\n-\t * between A and B, A...B names that merge base.\n-\t *\n-\t * (b) If is _not_ a commit, either \"--\" is present\n-\t * or is not a path, no -t or -b was given, and\n-\t * and there is a tracking branch whose name is \n-\t * in one and only one remote (or if the branch exists on the\n-\t * remote named in checkout.defaultRemote), then this is a\n-\t * short-hand to fork local from that\n-\t * remote-tracking branch.\n-\t *\n-\t * (c) Otherwise, if \"--\" is present, treat it like case (1).\n-\t *\n-\t * (d) Otherwise :\n-\t * - if it's a reference, treat it like case (1)\n-\t * - else if it's a path, treat it like case (2)\n-\t * - else: fail.\n-\t *\n-\t * case 4: git checkout \n-\t *\n-\t * The first argument must not be ambiguous.\n-\t * - If it's *only* a reference, treat it like case (1).\n-\t * - If it's only a path, treat it like case (2).\n-\t * - else: fail.\n-\t *\n+\t * Resolve ambiguity where argv[0] may be or .\n+\t * High-level approach is:\n+\t * 1) Use various things to reduce ambiguity, examples:\n+\t * * '--' is present\n+\t * * command doesn't accept \n+\t * * additional options like '-b' were given\n+\t * 2) If ambiguous and matches both existing and existing\n+\t * file, complain. However, in 1-argument 'git checkout '\n+\t * treat as to avoid annoying users.\n+\t * 3) Otherwise, if it matches some existing , treat as\n+\t * .\n+\t * 4) Otherwise, if it matches a remote branch, and it's considered\n+\t * reasonable to DWIM to create a local branch from remote branch,\n+\t * do that and proceed with (2)(3).\n+\t * 5) Otherwise, let caller proceed with interpretation.\n \t */\n \tif (!argc)\n \t\treturn 0;\n@@ -1187,9 +1163,9 @@ static int parse_branchname_arg(int argc, const char **argv,\n \n \tif (opts->accept_pathspec) {\n \t if (dash_dash_pos == 0)\n-\t\t return 1; /* case (2) */\n+\t\t return 1;\n \t else if (dash_dash_pos == 1)\n-\t\t has_dash_dash = 1; /* case (3) or (1) */\n+\t\t has_dash_dash = 1;\n \t else if (dash_dash_pos >= 2)\n \t\t die(_(\"only one reference expected, %d given.\"), dash_dash_pos);\n \t}\n@@ -1203,14 +1179,6 @@ static int parse_branchname_arg(int argc, const char **argv,\n \t\targ = \"@{-1}\";\n \n \tif (get_oid_mb(arg, rev)) {\n-\t\t/*\n-\t\t * Either case (3) or (4), with not being\n-\t\t * a commit, or an attempt to use case (1) with an\n-\t\t * invalid ref.\n-\t\t *\n-\t\t * It's likely an error, but we need to find out if\n-\t\t * we should auto-create the branch, case (3).(b).\n-\t\t */\n \t\tint recover_with_dwim = dwim_new_local_branch_ok;\n \n \t\tint could_be_checkout_paths = !expect_commit_only &&\n@@ -1219,10 +1187,6 @@ static int parse_branchname_arg(int argc, const char **argv,\n \t\tif (!expect_commit_only && !no_wildcard(arg))\n \t\t\trecover_with_dwim = 0;\n \n-\t\t/*\n-\t\t * Accept \"git checkout foo\", \"git checkout foo --\"\n-\t\t * and \"git switch foo\" as candidates for dwim.\n-\t\t */\n \t\tif (!(argc == 1 && !has_dash_dash) &&\n \t\t !(argc == 2 && has_dash_dash) &&\n \t\t opts->accept_pathspec)\n@@ -1238,7 +1202,7 @@ static int parse_branchname_arg(int argc, const char **argv,\n \t\t\t\t\t arg);\n \t\t\t\t*new_branch = arg;\n \t\t\t\targ = remote;\n-\t\t\t\t/* DWIMmed to create local branch, case (3).(b) */\n+\t\t\t\t/* DWIMmed to create local branch */\n \t\t\t} else {\n \t\t\t\trecover_with_dwim = 0;\n \t\t\t}\n@@ -1253,19 +1217,11 @@ static int parse_branchname_arg(int argc, const char **argv,\n \n \tsetup_new_branch_info_and_source_tree(new_branch_info, opts, rev, arg);\n \n-\tif (!opts->source_tree) /* case (1): want a tree */\n+\tif (!opts->source_tree)\n \t\tdie(_(\"reference is not a tree: %s\"), arg);\n \n-\tif (!expect_commit_only) {\t/* case (3).(d) -> (1) */\n-\t\t/*\n-\t\t * Do not complain the most common case\n-\t\t *\tgit checkout branch\n-\t\t * even if there happen to be a file called 'branch';\n-\t\t * it would be extremely annoying.\n-\t\t */\n-\t\tif (argc > 1)\n-\t\t\tverify_non_filename(opts->prefix, arg);\n-\t}\n+\tif (!expect_commit_only && argc > 1)\n+\t\tverify_non_filename(opts->prefix, arg);\n \n \treturn (dash_dash_pos == 1) ? 2 : 1;\n }\n-- \ngitgitgadget\n\n"},{"id":"388454","messageId":"xmqqk16tea9u.fsf@gitster-ct.c.googlers.com","threadId":"52368","inReplyTo":"771947d01f759a72b9f8999e929bf1f7765327aa.1574969538.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/5] parse_branchname_arg(): fix dash_dash_pos, drop argcount","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-18T18:52:45Z","receivedAt":"2019-12-18T18:52:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Alexandr Miloslavskiy via GitGitGadget\" \nwrites:\n\n> From: Alexandr Miloslavskiy \n>\n> `dash_dash_pos` was only calculated under `opts->accept_pathspec`. This\n> is unexpected to readers and made it harder to reason about the code.\n> Fix this by restoring the expected meaning.\n\nPerhaps. I think the original reasoning was to compute only where\ndash_dash_pos will be needed, but the code changes over time, and\nplaces that need the value of dash_dash_pos would change over time,\nso from that point of view, I think it makes sense to make sure it\ngets always computed like this patch does.\n\n> Simplify the code by dropping `argcount` and useless `argc` / `argv`\n> manipulations.\n\nI am not sure if this is a good change, though. It goes against the\nreasoning that makes the above \"dash-dash-pos\" change is a good one,\ndoesn't it? When the code changes over time, wouldn't it make the\ncode more clear to keep track of the count of args it saw in a\nseparate variable, than relying on the invariant that currently\nhappens to hold which is \"if dash-dash is after the first argument,\nreturn 2 and otherwise return 1\"?\n\n> @@ -1121,7 +1121,6 @@ static int parse_branchname_arg(int argc, const char **argv,\n> \t\t\t\tint *dwim_remotes_matched)\n> {\n> \tconst char **new_branch = &opts->new_branch;\n> -\tint argcount = 0;\n> \tconst char *arg;\n> \tint dash_dash_pos;\n> \tint has_dash_dash = 0;\n> @@ -1180,17 +1179,21 @@ static int parse_branchname_arg(int argc, const char **argv,\n> \targ = argv[0];\n> \tdash_dash_pos = -1;\n> \tfor (i = 0; i < argc; i++) {\n> -\t\tif (opts->accept_pathspec && !strcmp(argv[i], \"--\")) {\n> +\t\tif (!strcmp(argv[i], \"--\")) {\n> \t\t\tdash_dash_pos = i;\n> \t\t\tbreak;\n> \t\t}\n> \t}\n> -\tif (dash_dash_pos == 0)\n> -\t\treturn 1; /* case (2) */\n> -\telse if (dash_dash_pos == 1)\n> -\t\thas_dash_dash = 1; /* case (3) or (1) */\n> -\telse if (dash_dash_pos >= 2)\n> -\t\tdie(_(\"only one reference expected, %d given.\"), dash_dash_pos);\n> +\n> +\tif (opts->accept_pathspec) {\n> +\t if (dash_dash_pos == 0)\n> +\t\t return 1; /* case (2) */\n> +\t else if (dash_dash_pos == 1)\n> +\t\t has_dash_dash = 1; /* case (3) or (1) */\n> +\t else if (dash_dash_pos >= 2)\n> +\t\t die(_(\"only one reference expected, %d given.\"), dash_dash_pos);\n\nNon-standard indentation? In our code, a level of indent is another\nhorizontal tab.\n\n> +\t}\n"},{"id":"388456","messageId":"xmqqfthhe92v.fsf@gitster-ct.c.googlers.com","threadId":"52368","inReplyTo":"a84633a44474aa25bd1101a9ca2a5d9687900bf2.1574969538.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/5] parse_branchname_arg(): introduce expect_commit_only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-18T19:18:32Z","receivedAt":"2019-12-18T19:18:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Alexandr Miloslavskiy via GitGitGadget\" \nwrites:\n\n> From: Alexandr Miloslavskiy \n>\n> `has_dash_dash` unexpectedly takes `opts->accept_pathspec` into account.\n\nYou also touched the code that depends on opts->accept_pathspec in\nthe earlier step 1/5; these two steps seem harder to reason about\nthan necessary---I wonder if it is easier to explain and understand\nif these two steps are merged into one and explained together?\n\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 655b389756..5c6131dbe6 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -1123,7 +1123,7 @@ static int parse_branchname_arg(int argc, const char **argv,\n> \tconst char **new_branch = &opts->new_branch;\n> \tconst char *arg;\n> \tint dash_dash_pos;\n> -\tint has_dash_dash = 0;\n> +\tint has_dash_dash = 0, expect_commit_only = 0;\n> \tint i;\n> \n> \t/*\n> @@ -1194,7 +1194,10 @@ static int parse_branchname_arg(int argc, const char **argv,\n> \t\t die(_(\"only one reference expected, %d given.\"), dash_dash_pos);\n> \t}\n> \n> -\topts->count_checkout_paths = !opts->quiet && !has_dash_dash;\n> +\tif (has_dash_dash)\n> +\t expect_commit_only = 1;\n\nNon-standard indentation here.\n\n> +\topts->count_checkout_paths = !opts->quiet && !expect_commit_only;\n\nOK. count_checkout_paths is relevant only when checking out paths\nout of a tree-ish, so \"expect-commit-only\" would be false in such a\nsituation. On the other hand, if we were checking out a branch (or\ndetaching), we must have a commit and a tree-ish is insufficient,\nso expect_commit_only would be true in such a case.\n\nMakes sense. I am wondering if we still need has_dash_dash, and\nalso if expect_commit_only is the best name for the variable.\n\nThanks.\n\n> @@ -1210,10 +1213,10 @@ static int parse_branchname_arg(int argc, const char **argv,\n> \t\t */\n> \t\tint recover_with_dwim = dwim_new_local_branch_ok;\n> \n> -\t\tint could_be_checkout_paths = !has_dash_dash &&\n> +\t\tint could_be_checkout_paths = !expect_commit_only &&\n> \t\t\tcheck_filename(opts->prefix, arg);\n> \n> -\t\tif (!has_dash_dash && !no_wildcard(arg))\n> +\t\tif (!expect_commit_only && !no_wildcard(arg))\n> \t\t\trecover_with_dwim = 0;\n> \n> \t\t/*\n> @@ -1242,7 +1245,7 @@ static int parse_branchname_arg(int argc, const char **argv,\n> \t\t}\n> \n> \t\tif (!recover_with_dwim) {\n> -\t\t\tif (has_dash_dash)\n> +\t\t\tif (expect_commit_only)\n> \t\t\t\tdie(_(\"invalid reference: %s\"), arg);\n> \t\t\treturn 0;\n> \t\t}\n> @@ -1253,7 +1256,7 @@ static int parse_branchname_arg(int argc, const char **argv,\n> \tif (!opts->source_tree) /* case (1): want a tree */\n> \t\tdie(_(\"reference is not a tree: %s\"), arg);\n> \n> -\tif (!has_dash_dash) {\t/* case (3).(d) -> (1) */\n> +\tif (!expect_commit_only) {\t/* case (3).(d) -> (1) */\n> \t\t/*\n> \t\t * Do not complain the most common case\n> \t\t *\tgit checkout branch\n"},{"id":"388552","messageId":"713f8aea-cdd6-3fcd-8e9f-c9b909454ef5@syntevo.com","threadId":"52368","inReplyTo":"xmqqfthhe92v.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/5] parse_branchname_arg(): introduce expect_commit_only","fromName":"Alexandr Miloslavskiy","fromEmail":"alexandr.miloslavskiy@syntevo.com","sentAt":"2019-12-19T18:03:23Z","receivedAt":"2019-12-19T18:03:25Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"On 18.12.2019 20:18, Junio C Hamano wrote:\n\n>> `has_dash_dash` unexpectedly takes `opts->accept_pathspec` into account.\n> \n> You also touched the code that depends on opts->accept_pathspec in\n> the earlier step 1/5; these two steps seem harder to reason about\n> than necessary---I wonder if it is easier to explain and understand\n> if these two steps are merged into one and explained together?\n\nYes, that sounds like a good idea! In V3 in other topic [1] I have \nshuffled the changes between commits for easier understanding.\n\n>> +\tif (has_dash_dash)\n>> +\t expect_commit_only = 1;\n> \n> Non-standard indentation here.\n\nSorry!\n\n>> +\topts->count_checkout_paths = !opts->quiet && !expect_commit_only;\n> \n> OK. count_checkout_paths is relevant only when checking out paths\n> out of a tree-ish, so \"expect-commit-only\" would be false in such a\n> situation. On the other hand, if we were checking out a branch (or\n> detaching), we must have a commit and a tree-ish is insufficient,\n> so expect_commit_only would be true in such a case.\n> \n> Makes sense. I am wondering if we still need has_dash_dash, and\n> also if expect_commit_only is the best name for the variable.\n\nIt seems that indeed, the variable could use an even better name. It's \n as opposed to , and not as opposed to . I \nhave renamed the variable again in V3 in other topic [1].\n\n----\n[1] \nhttps://lore.kernel.org/git/pull.490.v3.git.1576778515.gitgitgadget@gmail.com/\n"},{"id":"388553","messageId":"807d4e5c-89b9-298b-c022-08d330fbd3c9@syntevo.com","threadId":"52368","inReplyTo":"xmqqk16tea9u.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/5] parse_branchname_arg(): fix dash_dash_pos, drop argcount","fromName":"Alexandr Miloslavskiy","fromEmail":"alexandr.miloslavskiy@syntevo.com","sentAt":"2019-12-19T18:03:20Z","receivedAt":"2019-12-19T18:03:26Z","isPatch":true,"sender":{"key":"alexandr.miloslavskiy@syntevo.com","avatar":null},"body":"Thanks for having a look at my patches!\n\nOn 18.12.2019 19:52, Junio C Hamano wrote:\n>> Simplify the code by dropping `argcount` and useless `argc` / `argv`\n>> manipulations.\n> \n> I am not sure if this is a good change, though. It goes against the\n> reasoning that makes the above \"dash-dash-pos\" change is a good one,\n> doesn't it? When the code changes over time, wouldn't it make the\n> code more clear to keep track of the count of args it saw in a\n> separate variable, than relying on the invariant that currently\n> happens to hold which is \"if dash-dash is after the first argument,\n> return 2 and otherwise return 1\"?\n\nMy bad that I forgot to write a detailed explanation for that. In V3 in \nother topic [2] I have separated this change as a separate commit with a \ngood message.\n\n>> +\tif (opts->accept_pathspec) {\n>> +\t if (dash_dash_pos == 0)\n>> +\t\t return 1; /* case (2) */\n>> +\t else if (dash_dash_pos == 1)\n>> +\t\t has_dash_dash = 1; /* case (3) or (1) */\n>> +\t else if (dash_dash_pos >= 2)\n>> +\t\t die(_(\"only one reference expected, %d given.\"), dash_dash_pos);\n> \n> Non-standard indentation? In our code, a level of indent is another\n> horizontal tab.\n\nSorry, my Visual Studio was acting up again. It's doing pretty weird \nthings when I have tab size 4 (due to other projects), which is \noverridden by git repo settings to 8.\n\nFixed it in V3 in other topic [2]\n\n----\n[1] Commit 09ebad6f (\"checkout: split off a function to peel away \nbranchname arg\", 2011-02-08)\n[2] \nhttps://lore.kernel.org/git/pull.490.v3.git.1576778515.gitgitgadget@gmail.com/\n"}]}