| Message ID | 20211209184928.71413-4-chooglen@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | implement branch --recurse-submodules | expand |
Glen Choo <chooglen@google.com> writes: > Incidentally, fix an incorrect usage string that combined the 'list' > usage of git branch (-l) with the 'create' usage; this string has been > incorrect since its inception, a8dfd5eac4 (Make builtin-branch.c use > parse_options., 2007-10-07). I think that we implement such incidental fixes only when we're touching the relevant lines, but this change looks correct. > - int delete = 0, rename = 0, copy = 0, force = 0, list = 0; > - int show_current = 0; > - int reflog = 0, edit_description = 0; > - int quiet = 0, unset_upstream = 0; > + /* possible actions */ > + int delete = 0, rename = 0, copy = 0, force = 0, list = 0, > + unset_upstream = 0, show_current = 0, edit_description = 0; > + int noncreate_actions = 0; > + /* possible options */ > + int reflog = 0, quiet = 0, icase = 0; [snip] > - if (!!delete + !!rename + !!copy + !!new_upstream + !!show_current + > - list + edit_description + unset_upstream > 1) > + noncreate_actions = !!delete + !!rename + !!copy + !!new_upstream + > + !!show_current + !!list + !!edit_description + > + !!unset_upstream; > + if (noncreate_actions > 1) > usage_with_options(builtin_branch_usage, options); Overall this change looks good, although if you're going to rearrange the variable declarations (e.g. the positions of show_current, edit_description, and unset_upstream have moved), you might as well make them consistent with the noncreate_actions statement, I guess. Also maybe move new_upstream closer.
Jonathan Tan <jonathantanmy@google.com> writes: >> Incidentally, fix an incorrect usage string that combined the 'list' >> usage of git branch (-l) with the 'create' usage; this string has been >> incorrect since its inception, a8dfd5eac4 (Make builtin-branch.c use >> parse_options., 2007-10-07). > > I think that we implement such incidental fixes only when we're touching > the relevant lines, but this change looks correct. That's fair. This fix is such low-hanging fruit that I don't think it deserves its patch, but if others agree, I'll separate it. > >> - int delete = 0, rename = 0, copy = 0, force = 0, list = 0; >> - int show_current = 0; >> - int reflog = 0, edit_description = 0; >> - int quiet = 0, unset_upstream = 0; >> + /* possible actions */ >> + int delete = 0, rename = 0, copy = 0, force = 0, list = 0, >> + unset_upstream = 0, show_current = 0, edit_description = 0; >> + int noncreate_actions = 0; >> + /* possible options */ >> + int reflog = 0, quiet = 0, icase = 0; > > [snip] > >> - if (!!delete + !!rename + !!copy + !!new_upstream + !!show_current + >> - list + edit_description + unset_upstream > 1) >> + noncreate_actions = !!delete + !!rename + !!copy + !!new_upstream + >> + !!show_current + !!list + !!edit_description + >> + !!unset_upstream; >> + if (noncreate_actions > 1) >> usage_with_options(builtin_branch_usage, options); > > Overall this change looks good, although if you're going to rearrange > the variable declarations (e.g. the positions of show_current, > edit_description, and unset_upstream have moved), you might as well make > them consistent with the noncreate_actions statement, I guess. Also > maybe move new_upstream closer. Yeah this is obviously inconsistent, thanks for the catch. * force isn't an action * new_upstream is an action * for QoL, all of the actions should be listed in the same order at both sites
diff --git a/builtin/branch.c b/builtin/branch.c index e19aab5356..14aff33a50 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -27,7 +27,7 @@ static const char * const builtin_branch_usage[] = { N_("git branch [<options>] [-r | -a] [--merged] [--no-merged]"), - N_("git branch [<options>] [-l] [-f] <branch-name> [<start-point>]"), + N_("git branch [<options>] [-l] [<pattern>...]"), N_("git branch [<options>] [-r] (-d | -D) <branch-name>..."), N_("git branch [<options>] (-m | -M) [<old-branch>] <new-branch>"), N_("git branch [<options>] (-c | -C) [<old-branch>] <new-branch>"), @@ -616,14 +616,15 @@ static int edit_branch_description(const char *branch_name) int cmd_branch(int argc, const char **argv, const char *prefix) { - int delete = 0, rename = 0, copy = 0, force = 0, list = 0; - int show_current = 0; - int reflog = 0, edit_description = 0; - int quiet = 0, unset_upstream = 0; + /* possible actions */ + int delete = 0, rename = 0, copy = 0, force = 0, list = 0, + unset_upstream = 0, show_current = 0, edit_description = 0; + int noncreate_actions = 0; + /* possible options */ + int reflog = 0, quiet = 0, icase = 0; const char *new_upstream = NULL; enum branch_track track; struct ref_filter filter; - int icase = 0; static struct ref_sorting *sorting; struct string_list sorting_options = STRING_LIST_INIT_DUP; struct ref_format format = REF_FORMAT_INIT; @@ -708,8 +709,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix) filter.reachable_from || filter.unreachable_from || filter.points_at.nr) list = 1; - if (!!delete + !!rename + !!copy + !!new_upstream + !!show_current + - list + edit_description + unset_upstream > 1) + noncreate_actions = !!delete + !!rename + !!copy + !!new_upstream + + !!show_current + !!list + !!edit_description + + !!unset_upstream; + if (noncreate_actions > 1) usage_with_options(builtin_branch_usage, options); if (filter.abbrev == -1) @@ -847,7 +850,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) strbuf_addf(&buf, "branch.%s.merge", branch->name); git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE); strbuf_release(&buf); - } else if (argc > 0 && argc <= 2) { + } else if (!noncreate_actions && argc > 0 && argc <= 2) { if (filter.kind != FILTER_REFS_BRANCHES) die(_("The -a, and -r, options to 'git branch' do not take a branch name.\n" "Did you mean to use: -a|-r --list <pattern>?"));
Add a variable to cmd_branch() that will tell us whether or not cmd_branch() will default to creating a branch (instead of performing another action). Besides making the function more explicit, this allows us to validate options that can only be used when creating a branch. Such an option does not exist yet, but one will be introduced in a subsequent commit. Incidentally, fix an incorrect usage string that combined the 'list' usage of git branch (-l) with the 'create' usage; this string has been incorrect since its inception, a8dfd5eac4 (Make builtin-branch.c use parse_options., 2007-10-07). Signed-off-by: Glen Choo <chooglen@google.com> --- builtin/branch.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)