Re: [PATCH 2/5] branch: introduce --list argument

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Michael J Gruber <git@xxxxxxxxxxxxxxxxxxxx> writes:

> @@ -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))

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]