| Message ID | a84633a44474aa25bd1101a9ca2a5d9687900bf2.1574969538.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | parse_branchname_arg(): make code easier to understand | expand |
"Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com> > > `has_dash_dash` unexpectedly takes `opts->accept_pathspec` into 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? > diff --git a/builtin/checkout.c b/builtin/checkout.c > index 655b389756..5c6131dbe6 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -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; Non-standard indentation here. > + 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. > @@ -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
On 18.12.2019 20:18, Junio C Hamano wrote: >> `has_dash_dash` unexpectedly takes `opts->accept_pathspec` into 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? Yes, that sounds like a good idea! In V3 in other topic [1] I have shuffled the changes between commits for easier understanding. >> + if (has_dash_dash) >> + expect_commit_only = 1; > > Non-standard indentation here. Sorry! >> + 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. 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]. ---- [1] https://lore.kernel.org/git/pull.490.v3.git.1576778515.gitgitgadget@gmail.com/
diff --git a/builtin/checkout.c b/builtin/checkout.c index 655b389756..5c6131dbe6 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -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