Re: [PATCH 2/5] branch: introduce --list argument
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 25, 2011, 19:52 UTC
- Message-ID
- <7vmxexjmk6.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <32d0954697da9ac96cc11af0d8cc1390d93fd037.1314259226.git.git@drmicha.warpmail.net>
Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 11 quoted lines
> @@ -695,12 +696,15 @@ int cmd_branch(int argc, const char **argv, const char *prefix) > > argc = parse_options(argc, argv, prefix, options, builtin_branch_usage, > 0); > - if (!!delete + !!rename + !!force_create > 1) > + if (!!delete + !!rename + !!force_create + !!list > 1) > usage_with_options(builtin_branch_usage, options); > > + if (argc == 0 || (verbose && argc == 1)) > + list = 1; > +
I am afraid this is not quite right.
What happens if one of delete/rename/force-create can take a single argument, or more importantly, if we someday gain another option that can take zero or one argument and is incompatible with other operating mode?
For example, what happens to this command line with your "git branch":
$ git branch -v -m boo
The first test passes (no explicit --list), and then you violate the "only one of these operating mode can be set" assertion you made yourself by flipping list on, no?
It is a good practice to always run the defaulting tweak *before* checking options that are mutually incompatible, to catch your own mistake in the tweaking code.
On top of my previous "multiple patterns allowed" tweak, a replacement patch to this file may look like this. I suspect there should be a better way to represent the mode in a single variable and enforce that there is only one chosen, but that is left for another day.
builtin/branch.c | 12 +++++++++--- 1 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c index 0fa62bd..4243e7c 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -629,7 +629,7 @@ static int opt_parse_merge_filter(const struct option *opt, const char *arg, int int cmd_branch(int argc, const char **argv, const char *prefix) { - int delete = 0, rename = 0, force_create = 0; + int delete = 0, rename = 0, force_create = 0, list = 0; int verbose = 0, abbrev = DEFAULT_ABBREV, detached = 0; int reflog = 0; enum branch_track track; @@ -668,6 +668,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) OPT_BIT('D', NULL, &delete, "delete branch (even if not merged)", 2), OPT_BIT('m', NULL, &rename, "move/rename a branch and its reflog", 1), OPT_BIT('M', NULL, &rename, "move/rename a branch, even if target exists", 2), + OPT_BOOLEAN(0, "list", &list, "list branch names"), OPT_BOOLEAN('l', NULL, &reflog, "create the branch's reflog"), OPT__FORCE(&force_create, "force creation (when already exists)"), { @@ -710,12 +711,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, options, builtin_branch_usage, 0); - if (!!delete + !!rename + !!force_create > 1) + + if (!delete && !rename && !force_create && + (argc == 0 || (verbose && argc))) + list = 1; + + if (!!delete + !!rename + !!force_create + !!list > 1) usage_with_options(builtin_branch_usage, options); if (delete) return delete_branches(argc, argv, delete > 1, kinds); - else if (argc == 0 || (verbose && argc)) + else if (list) return print_ref_list(kinds, detached, verbose, abbrev, with_commit, argv); else if (rename && (argc == 1))