parse_branchname_arg(): make code easier to understand
cover parse_branchname_arg(): make code easier to understand
My bigger goal is to complete my --pathspec-from-file series of patches.
For this, I needed to evaluate whether parse_branchname_arg() heuristics needs any changes when pathspec is passed, but NOT in args.
I found it surprisingly hard to reason about the code in this function. This mostly happens due to "obfuscated" variables, where they have a clear name and a different actual meaning. Ultimately, it was hard to mentally expand them to true meaning AND see all possible combinations of branches at once.
I have split this refactoring in 4 patches, so that diffs are not too big in every single patch.
To my understanding, there should be no changes in git's behavior, except for a couple better die() messages.
Alexandr Miloslavskiy (5):
parse_branchname_arg(): fix dash_dash_pos, drop argcount parse_branchname_arg(): introduce expect_commit_only parse_branchname_arg(): update code comments parse_branchname_arg(): refactor the decision making
t2024: cover more cases
builtin/checkout.c | 174 ++++++++++++++++----------------------- t/t2024-checkout-dwim.sh | 27 ++++++ 2 files changed, 97 insertions(+), 104 deletions(-)
base-commit: d9f6f3b6195a0ca35642561e530798ad1469bd41
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-479%2FSyntevoAlex%2F%230225(git)_refactor_parse_branchname_arg-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-479/SyntevoAlex/#0225(git)_refactor_parse_branchname_arg-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/479
Signature · 1 line
gitgitgadget
PATCH 1/5 parse_branchname_arg(): fix dash_dash_pos, drop argcount
From: Alexandr Miloslavskiy <[email protected]>
dash_dash_pos was only calculated under opts->accept_pathspec. This is unexpected to readers and made it harder to reason about the code. Fix this by restoring the expected meaning.
Simplify the code by dropping argcount and useless argc / argv manipulations.
This should not change behavior in any way.
--- builtin/checkout.c | 34 ++++++++++++++-------------------- 1 file changed, 14 insertions(+), 20 deletions(-)
builtin/checkout.c +14 -20
@@ -1121,7 +1121,6 @@ static int parse_branchname_arg(int argc, const char **argv, int *dwim_remotes_matched) {const char **new_branch = &opts->new_branch; -int argcount =0 ;const char *arg;int dash_dash_pos;int has_dash_dash =0 ;
@@ -1180,17 +1179,21 @@ static int parse_branchname_arg(int argc, const char **argv, arg = argv[0 ]; dash_dash_pos =-1 ;for (i =0 ; i < argc; i++) { -if (opts->accept_pathspec && !strcmp (argv[i],"--" )) { +if (!strcmp (argv[i],"--" )) { dash_dash_pos = i;break ; } } -if (dash_dash_pos ==0 ) -return 1 ;/* case (2) */ -else if (dash_dash_pos ==1 ) - has_dash_dash =1 ;/* case (3) or (1) */ -else if (dash_dash_pos >=2 ) -die (_ ("only one reference expected, %d given." ), dash_dash_pos); + +if (opts->accept_pathspec) { +if (dash_dash_pos ==0 ) +return 1 ;/* case (2) */ +else if (dash_dash_pos ==1 ) + has_dash_dash =1 ;/* case (3) or (1) */ +else if (dash_dash_pos >=2 ) +die (_ ("only one reference expected, %d given." ), dash_dash_pos); + } + opts->count_checkout_paths = !opts->quiet && !has_dash_dash;if (!strcmp (arg,"-" ))
@@ -1241,15 +1244,10 @@ static int parse_branchname_arg(int argc, const char **argv,if (!recover_with_dwim) {if (has_dash_dash)die (_ ("invalid reference: %s" ), arg); -return argcount; +return 0 ; } } -/* we can't end up being in (2) anymore, eat the argument */ - argcount++; - argv++; - argc--; -setup_new_branch_info_and_source_tree (new_branch_info, opts, rev, arg);if (!opts->source_tree)/* case (1): want a tree */
@@ -1262,15 +1260,11 @@ static int parse_branchname_arg(int argc, const char **argv,* even if there happen to be a file called 'branch'; * it would be extremely annoying. */ -if (argc) +if (argc >1 )verify_non_filename (opts->prefix, arg); - }else if (opts->accept_pathspec) { - argcount++; - argv++; - argc--; } -return argcount; +return (dash_dash_pos ==1 ) ?2 :1 ; }static int switch_unborn_to_new_branch (const struct checkout_opts *opts)
Signature · 1 line
gitgitgadget
PATCH 2/5 parse_branchname_arg(): introduce expect_commit_only
In reply to Alexandr Miloslavskiy via GitGitGadgetFrom: Alexandr Miloslavskiy <[email protected]>
has_dash_dash unexpectedly takes opts->accept_pathspec into account. While this may sound clever at first sight, it becomes pretty hard to reason (and not be a victim) about code, especially in combination with argc here:
if (!(argc == 1 && !has_dash_dash) && !(argc == 2 && has_dash_dash) && opts->accept_pathspec) recover_with_dwim = 0;
Introduce a new non-obfuscated variable to reduce the amount of diffs in next patch.
This should not change behavior in any way.
--- builtin/checkout.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
builtin/checkout.c +9 -6
@@ -1123,7 +1123,7 @@ static int parse_branchname_arg(int argc, const char **argv,const char **new_branch = &opts->new_branch;const char *arg;int dash_dash_pos; -int has_dash_dash =0 ; +int has_dash_dash =0 , expect_commit_only =0 ;int i;/*
@@ -1194,7 +1194,10 @@ static int parse_branchname_arg(int argc, const char **argv,die (_ ("only one reference expected, %d given." ), dash_dash_pos); } - opts->count_checkout_paths = !opts->quiet && !has_dash_dash; +if (has_dash_dash) + expect_commit_only =1 ; + + opts->count_checkout_paths = !opts->quiet && !expect_commit_only;if (!strcmp (arg,"-" )) arg ="@{-1}" ;
@@ -1210,10 +1213,10 @@ static int parse_branchname_arg(int argc, const char **argv,*/ int recover_with_dwim = dwim_new_local_branch_ok; -int could_be_checkout_paths = !has_dash_dash && +int could_be_checkout_paths = !expect_commit_only &&check_filename (opts->prefix, arg); -if (!has_dash_dash && !no_wildcard (arg)) +if (!expect_commit_only && !no_wildcard (arg)) recover_with_dwim =0 ;/*
@@ -1242,7 +1245,7 @@ static int parse_branchname_arg(int argc, const char **argv, }if (!recover_with_dwim) { -if (has_dash_dash) +if (expect_commit_only)die (_ ("invalid reference: %s" ), arg);return 0 ; }
@@ -1253,7 +1256,7 @@ static int parse_branchname_arg(int argc, const char **argv,if (!opts->source_tree)/* case (1): want a tree */ die (_ ("reference is not a tree: %s" ), arg); -if (!has_dash_dash) {/* case (3).(d) -> (1) */ +if (!expect_commit_only) {/* case (3).(d) -> (1) */ /* * Do not complain the most common case * git checkout branch
Signature · 1 line
gitgitgadget
PATCH 4/5 parse_branchname_arg(): refactor the decision making
In reply to Alexandr Miloslavskiy via GitGitGadgetFrom: Alexandr Miloslavskiy <[email protected]>
Make it easier to understand which branches handle which cases.
Drop obfuscated variable has_dash_dash which also took opts->accept_pathspec into account, making it pretty hard to reason about code, especially when used together with argc and opts->accept_pathspec here:
if (!(argc == 1 && !has_dash_dash) && !(argc == 2 && has_dash_dash) && opts->accept_pathspec) recover_with_dwim = 0;
Avoid double-negation in the code mentioned above ("it is not OK to proceed if it's not one of those cases").
Avoid hard-to-understand condition opts->accept_pathspec in the code mentioned above.
There are some minor die() message changes for:git switch <commit> <unexpected>git switch <commit> -- <unexpected>
--- builtin/checkout.c | 71 +++++++++++++++++++++++++++------------------- 1 file changed, 42 insertions(+), 29 deletions(-)
builtin/checkout.c +42 -29
@@ -1122,9 +1122,8 @@ static int parse_branchname_arg(int argc, const char **argv, {const char **new_branch = &opts->new_branch;const char *arg; -int dash_dash_pos; -int has_dash_dash =0 , expect_commit_only =0 ; -int i; +int dash_dash_pos, i; +int recover_with_dwim, expect_commit_only;/* * Resolve ambiguity where argv[0] may be <pathspec> or <commit>.
@@ -1143,15 +1142,7 @@ static int parse_branchname_arg(int argc, const char **argv,* do that and proceed with (2)(3). * 5) Otherwise, let caller proceed with <pathspec> interpretation. */ -if (!argc) -return 0 ; - -if (!opts->accept_pathspec) { -if (argc >1 ) -die (_ ("only one reference expected" )); - has_dash_dash =1 ;/* helps disambiguate */ - } - + arg = argv[0 ]; dash_dash_pos =-1 ;for (i =0 ; i < argc; i++) {
@@ -1161,17 +1152,46 @@ static int parse_branchname_arg(int argc, const char **argv, } } -if (opts->accept_pathspec) { -if (dash_dash_pos ==0 ) -return 1 ; -else if (dash_dash_pos ==1 ) - has_dash_dash =1 ; -else if (dash_dash_pos >=2 ) -die (_ ("only one reference expected, %d given." ), dash_dash_pos); - } +if (dash_dash_pos ==-1 ) { +if (argc ==0 ) { +/* 'git checkout/switch/restore' */ +return 0 ; + }else if (argc ==1 ) { +/* 'git checkout/switch/restore <something>' */ + recover_with_dwim = dwim_new_local_branch_ok; + }else if (!opts->accept_pathspec) { +/* 'git switch <commit> <unexpected> [...]' */ +die (_ ("only one reference expected, %d given." ), argc); + }else { +/* 'git checkout/restore <something> <pathspec> [...]' */ + recover_with_dwim =0 ; + } + +/* Absence of '--' leaves <pathspec>/<commit> ambiguity */ + expect_commit_only = !opts->accept_pathspec; + }else if (dash_dash_pos ==0 ) { +/* 'git checkout/switch/restore -- [...]' */ +return 1 ;/* Eat '--' */ + }else if (dash_dash_pos ==1 ) { +if (!opts->accept_pathspec) { +/* 'git switch <commit> -- [...]' */ +die (_ ("incompatible with pathspec arguments" )); + } -if (has_dash_dash) - expect_commit_only =1 ; +if (argc ==2 ) { +/* 'git checkout/restore <commit> --' */ + recover_with_dwim = dwim_new_local_branch_ok; + }else { +/* 'git checkout/restore <commit> -- <pathspec> [...]' */ + recover_with_dwim =0 ; + } + +/* Presence of '--' makes it certain that arg is <commit> */ + expect_commit_only =1 ; + }else { +/* 'git checkout/switch/restore <commit> <unxpected> [...] -- [...]' */ +die (_ ("only one reference expected, %d given." ), dash_dash_pos); + } opts->count_checkout_paths = !opts->quiet && !expect_commit_only;
@@ -1179,19 +1199,12 @@ static int parse_branchname_arg(int argc, const char **argv, arg ="@{-1}" ;if (get_oid_mb (arg, rev)) { -int recover_with_dwim = dwim_new_local_branch_ok; -int could_be_checkout_paths = !expect_commit_only &&check_filename (opts->prefix, arg);if (!expect_commit_only && !no_wildcard (arg)) recover_with_dwim =0 ; -if (!(argc ==1 && !has_dash_dash) && - !(argc ==2 && has_dash_dash) && - opts->accept_pathspec) - recover_with_dwim =0 ; -if (recover_with_dwim) {const char *remote =unique_tracking_name (arg, rev, dwim_remotes_matched);
Signature · 1 line
gitgitgadget
PATCH 5/5 t2024: cover more cases
In reply to Alexandr Miloslavskiy via GitGitGadgetFrom: Alexandr Miloslavskiy <[email protected]>
--- t/t2024-checkout-dwim.sh | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
t/t2024-checkout-dwim.sh +27 -0
@@ -132,6 +132,33 @@ test_expect_success 'checkout of branch from a single remote succeeds #2' ' test_branch_upstream baz repo_b baz ' +test_expect_success 'checkout of branch from a single remote succeeds with --' ' + git checkout -B master && + test_might_fail git branch -D baz && + + git checkout baz -- && + status_uno_is_clean && + test_branch baz && + test_cmp_rev remotes/other_b/baz HEAD && + test_branch_upstream baz repo_b baz +' + +test_expect_success 'dont DWIM with pathspec #1' ' + git checkout -B master && + test_might_fail git branch -D baz && + + test_must_fail git checkout baz nonExistingFile 2>err && + test_i18ngrep "did not match any file(s) known to git" err +' + +test_expect_success 'dont DWIM with pathspec #2' ' + git checkout -B master && + test_might_fail git branch -D baz && + + test_must_fail git checkout baz -- nonExistingFile 2>err && + test_i18ngrep "fatal: invalid reference: baz" err +' + test_expect_success '--no-guess suppresses branch auto-vivification' ' git checkout -B master && status_uno_is_clean &&
Signature · 1 line
gitgitgadget
PATCH 3/5 parse_branchname_arg(): update code comments
In reply to Alexandr Miloslavskiy via GitGitGadgetFrom: Alexandr Miloslavskiy <[email protected]>
These parts repeat git documentation:
... if <something> is A...B <...>
... remote named in checkout.defaultRemote ...
Some parts repeat the code below. With next patch, code will be easier to understand, so this is no longer needed.
This is a separate patch to reduce the amount of diffs in next patch.
--- builtin/checkout.c | 86 +++++++++++----------------------------------- 1 file changed, 21 insertions(+), 65 deletions(-)
builtin/checkout.c +21 -65
@@ -1127,45 +1127,21 @@ static int parse_branchname_arg(int argc, const char **argv,int i;/* -* case 1: git checkout <ref> -- [<paths>] -* -* <ref> must be a valid tree, everything after the '--' must be -* a path. -* -* case 2: git checkout -- [<paths>] -* -* everything after the '--' must be paths. -* -* case 3: git checkout <something> [--] -* -* (a) If <something> is a commit, that is to -* switch to the branch or detach HEAD at it. As a special case, -* if <something> is A...B (missing A or B means HEAD but you can -* omit at most one side), and if there is a unique merge base -* between A and B, A...B names that merge base. -* -* (b) If <something> is _not_ a commit, either "--" is present -* or <something> is not a path, no -t or -b was given, and -* and there is a tracking branch whose name is <something> -* in one and only one remote (or if the branch exists on the -* remote named in checkout.defaultRemote), then this is a -* short-hand to fork local <something> from that -* remote-tracking branch. -* -* (c) Otherwise, if "--" is present, treat it like case (1). -* -* (d) Otherwise : -* - if it's a reference, treat it like case (1) -* - else if it's a path, treat it like case (2) -* - else: fail. -* -* case 4: git checkout <something> <paths> -* -* The first argument must not be ambiguous. -* - If it's *only* a reference, treat it like case (1). -* - If it's only a path, treat it like case (2). -* - else: fail. -* +* Resolve ambiguity where argv[0] may be <pathspec> or <commit>. +* High-level approach is: +* 1) Use various things to reduce ambiguity, examples: +* * '--' is present +* * command doesn't accept <pathspec> +* * additional options like '-b' were given +* 2) If ambiguous and matches both existing <commit> and existing +* file, complain. However, in 1-argument 'git checkout <arg>' +* treat as <commit> to avoid annoying users. +* 3) Otherwise, if it matches some existing <commit>, treat as +* <commit>. +* 4) Otherwise, if it matches a remote branch, and it's considered +* reasonable to DWIM to create a local branch from remote branch, +* do that and proceed with (2)(3). +* 5) Otherwise, let caller proceed with <pathspec> interpretation. */ if (!argc)return 0 ;
@@ -1187,9 +1163,9 @@ static int parse_branchname_arg(int argc, const char **argv,if (opts->accept_pathspec) {if (dash_dash_pos ==0 ) -return 1 ;/* case (2) */ +return 1 ;else if (dash_dash_pos ==1 ) - has_dash_dash =1 ;/* case (3) or (1) */ + has_dash_dash =1 ;else if (dash_dash_pos >=2 )die (_ ("only one reference expected, %d given." ), dash_dash_pos); }
@@ -1203,14 +1179,6 @@ static int parse_branchname_arg(int argc, const char **argv, arg ="@{-1}" ;if (get_oid_mb (arg, rev)) { -/* -* Either case (3) or (4), with <something> not being -* a commit, or an attempt to use case (1) with an -* invalid ref. -* -* It's likely an error, but we need to find out if -* we should auto-create the branch, case (3).(b). -*/ int recover_with_dwim = dwim_new_local_branch_ok; int could_be_checkout_paths = !expect_commit_only &&
@@ -1219,10 +1187,6 @@ static int parse_branchname_arg(int argc, const char **argv,if (!expect_commit_only && !no_wildcard (arg)) recover_with_dwim =0 ; -/* -* Accept "git checkout foo", "git checkout foo --" -* and "git switch foo" as candidates for dwim. -*/ if (!(argc ==1 && !has_dash_dash) && !(argc ==2 && has_dash_dash) && opts->accept_pathspec)
@@ -1238,7 +1202,7 @@ static int parse_branchname_arg(int argc, const char **argv, arg); *new_branch = arg; arg = remote; -/* DWIMmed to create local branch, case (3).(b) */ +/* DWIMmed to create local branch */ }else { recover_with_dwim =0 ; }
@@ -1253,19 +1217,11 @@ static int parse_branchname_arg(int argc, const char **argv,setup_new_branch_info_and_source_tree (new_branch_info, opts, rev, arg); -if (!opts->source_tree)/* case (1): want a tree */ +if (!opts->source_tree)die (_ ("reference is not a tree: %s" ), arg); -if (!expect_commit_only) {/* case (3).(d) -> (1) */ -/* -* Do not complain the most common case -* git checkout branch -* even if there happen to be a file called 'branch'; -* it would be extremely annoying. -*/ -if (argc >1 ) -verify_non_filename (opts->prefix, arg); - } +if (!expect_commit_only && argc >1 ) +verify_non_filename (opts->prefix, arg);return (dash_dash_pos ==1 ) ?2 :1 ; }
Signature · 1 line
gitgitgadget
Re: [PATCH 1/5] parse_branchname_arg(): fix dash_dash_pos, drop argcount
In reply to Alexandr Miloslavskiy via GitGitGadget"Alexandr Miloslavskiy via GitGitGadget" <[email protected]>
Quoted 5 lines · writes:
From: Alexandr Miloslavskiy <[email protected]>
dash_dash_poswas only calculated underopts->accept_pathspec. This is unexpected to readers and made it harder to reason about the code. Fix this by restoring the expected meaning.
Perhaps. I think the original reasoning was to compute only where dash_dash_pos will be needed, but the code changes over time, and places that need the value of dash_dash_pos would change over time, so from that point of view, I think it makes sense to make sure it gets always computed like this patch does.
Quoted 2 lines
Simplify the code by dropping
argcountand uselessargc/argvmanipulations.
I am not sure if this is a good change, though. It goes against the reasoning that makes the above "dash-dash-pos" change is a good one, doesn't it? When the code changes over time, wouldn't it make the code more clear to keep track of the count of args it saw in a separate variable, than relying on the invariant that currently happens to hold which is "if dash-dash is after the first argument, return 2 and otherwise return 1"?
Quoted 22 lines
builtin/checkout.c +1
-6@@ -1121,7 +1121,6 @@ static int parse_branchname_arg(int argc, const char **argv, int *dwim_remotes_matched) {const char **new_branch = &opts->new_branch; -int argcount =0 ;const char *arg;int dash_dash_pos;int has_dash_dash =0 ;@@ -1180,17 +1179,21 @@ static int parse_branchname_arg(int argc, const char **argv, arg = argv[0 ]; dash_dash_pos =-1 ;for (i =0 ; i < argc; i++) { -if (opts->accept_pathspec && !strcmp (argv[i],"--" )) { +if (!strcmp (argv[i],"--" )) { dash_dash_pos = i;break ; } } -if (dash_dash_pos ==0 ) -return 1 ;/* case (2) */ -else if (dash_dash_pos ==1 ) - has_dash_dash =1 ;/* case (3) or (1) */
builtin/checkout.c static int parse_branchname_arg(int argc, const char **argv, · line 1194
Quoted 10 lines
builtin/checkout.c +8
-2-else if (dash_dash_pos >=2 ) -die (_ ("only one reference expected, %d given." ), dash_dash_pos); + +if (opts->accept_pathspec) { +if (dash_dash_pos ==0 ) +return 1 ;/* case (2) */ +else if (dash_dash_pos ==1 ) + has_dash_dash =1 ;/* case (3) or (1) */ +else if (dash_dash_pos >=2 ) +die (_ ("only one reference expected, %d given." ), dash_dash_pos);
Non-standard indentation? In our code, a level of indent is another horizontal tab.
Quoted 1 line
builtin/checkout.c +1
-0+ }
Re: [PATCH 2/5] parse_branchname_arg(): introduce expect_commit_only
In reply to Alexandr Miloslavskiy via GitGitGadget"Alexandr Miloslavskiy via GitGitGadget" <[email protected]>
Quoted 3 lines · writes:
From: Alexandr Miloslavskiy <[email protected]>
has_dash_dashunexpectedly takesopts->accept_pathspecinto account.
You also touched the code that depends on opts->accept_pathspec in the earlier step 1/5; these two steps seem harder to reason about than necessary---I wonder if it is easier to explain and understand if these two steps are merged into one and explained together?
Quoted 14 lines
builtin/checkout.c +1
-1@@ -1123,7 +1123,7 @@ static int parse_branchname_arg(int argc, const char **argv,const char **new_branch = &opts->new_branch;const char *arg;int dash_dash_pos; -int has_dash_dash =0 ; +int has_dash_dash =0 , expect_commit_only =0 ;int i;/*
builtin/checkout.c static int parse_branchname_arg(int argc, const char **argv, · line 1198
Quoted 6 lines
builtin/checkout.c +2
-1@@ -1194,7 +1194,10 @@ static int parse_branchname_arg(int argc, const char **argv,die (_ ("only one reference expected, %d given." ), dash_dash_pos); } - opts->count_checkout_paths = !opts->quiet && !has_dash_dash; +if (has_dash_dash) + expect_commit_only =1 ;
Non-standard indentation here.
builtin/checkout.c static int parse_branchname_arg(int argc, const char **argv, · line 1200
Quoted 1 line
builtin/checkout.c +1
-0+ opts->count_checkout_paths = !opts->quiet && !expect_commit_only;
OK. count_checkout_paths is relevant only when checking out paths out of a tree-ish, so "expect-commit-only" would be false in such a situation. On the other hand, if we were checking out a branch (or detaching), we must have a commit and a tree-ish is insufficient, so expect_commit_only would be true in such a case.
Makes sense. I am wondering if we still need has_dash_dash, and also if expect_commit_only is the best name for the variable.
Thanks.
Quoted 31 lines
builtin/checkout.c +4
-4@@ -1210,10 +1213,10 @@ static int parse_branchname_arg(int argc, const char **argv,*/ int recover_with_dwim = dwim_new_local_branch_ok; -int could_be_checkout_paths = !has_dash_dash && +int could_be_checkout_paths = !expect_commit_only &&check_filename (opts->prefix, arg); -if (!has_dash_dash && !no_wildcard (arg)) +if (!expect_commit_only && !no_wildcard (arg)) recover_with_dwim =0 ;/* @@ -1242,7 +1245,7 @@ static int parse_branchname_arg(int argc, const char **argv, }if (!recover_with_dwim) { -if (has_dash_dash) +if (expect_commit_only)die (_ ("invalid reference: %s" ), arg);return 0 ; }@@ -1253,7 +1256,7 @@ static int parse_branchname_arg(int argc, const char **argv,if (!opts->source_tree)/* case (1): want a tree */ die (_ ("reference is not a tree: %s" ), arg); -if (!has_dash_dash) {/* case (3).(d) -> (1) */ +if (!expect_commit_only) {/* case (3).(d) -> (1) */ /* * Do not complain the most common case * git checkout branch
Re: [PATCH 2/5] parse_branchname_arg(): introduce expect_commit_only
Quoted 6 lines · Junio C Hamano
Quoted 1 line
has_dash_dashunexpectedly takesopts->accept_pathspecinto account.You also touched the code that depends on
opts->accept_pathspecin the earlier step 1/5; these two steps seem harder to reason about than necessary---I wonder if it is easier to explain and understand if these two steps are merged into one and explained together?
Yes, that sounds like a good idea! In V3 in other topic [1] I have shuffled the changes between commits for easier understanding.
Quoted 4 lines
builtin/checkout.c static int parse_branchname_arg(int argc, const char **argv, · line 1198
Quoted 2 lines
builtin/checkout.c +2
-0+if (has_dash_dash) + expect_commit_only =1 ;Non-standard indentation here.
Sorry!
Quoted 10 lines
builtin/checkout.c static int parse_branchname_arg(int argc, const char **argv, · line 1200
Quoted 1 line
builtin/checkout.c +1
-0+ opts->count_checkout_paths = !opts->quiet && !expect_commit_only;OK.
count_checkout_pathsis relevant only when checking out paths out of a tree-ish, so "expect-commit-only" would be false in such a situation. On the other hand, if we were checking out a branch (or detaching), we must have a commit and a tree-ish is insufficient, soexpect_commit_onlywould be true in such a case.Makes sense. I am wondering if we still need
has_dash_dash, and also ifexpect_commit_onlyis the best name for the variable.
It seems that indeed, the variable could use an even better name. It's <commit> as opposed to <pathspec>, and not as opposed to <tree-ish>. I have renamed the variable again in V3 in other topic [1].
Re: [PATCH 1/5] parse_branchname_arg(): fix dash_dash_pos, drop argcount
In reply to Junio C HamanoThanks for having a look at my patches!
Quoted 10 lines · Junio C Hamano
Quoted 2 lines
Simplify the code by dropping
argcountand uselessargc/argvmanipulations.I am not sure if this is a good change, though. It goes against the reasoning that makes the above "dash-dash-pos" change is a good one, doesn't it? When the code changes over time, wouldn't it make the code more clear to keep track of the count of args it saw in a separate variable, than relying on the invariant that currently happens to hold which is "if dash-dash is after the first argument, return 2 and otherwise return 1"?
My bad that I forgot to write a detailed explanation for that. In V3 in other topic [2] I have separated this change as a separate commit with a good message.
Quoted 10 lines
builtin/checkout.c static int parse_branchname_arg(int argc, const char **argv, · line 1194
Quoted 7 lines
builtin/checkout.c +7
-0+if (opts->accept_pathspec) { +if (dash_dash_pos ==0 ) +return 1 ;/* case (2) */ +else if (dash_dash_pos ==1 ) + has_dash_dash =1 ;/* case (3) or (1) */ +else if (dash_dash_pos >=2 ) +die (_ ("only one reference expected, %d given." ), dash_dash_pos);Non-standard indentation? In our code, a level of indent is another horizontal tab.
Sorry, my Visual Studio was acting up again. It's doing pretty weird things when I have tab size 4 (due to other projects), which is overridden by git repo settings to 8.
Fixed it in V3 in other topic [2]
----
[1] Commit 09ebad6f ("checkout: split off a function to peel away branchname arg", 2011-02-08)
[2] https://lore.kernel.org/git/[email protected]/