Re: [PATCH] switch: fix errors and comments related to -c and -C

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 29, 2020, 16:35 UTC
Message-ID: <xmqqv9lixnax.fsf@gitster.c.googlers.com>
In-Reply-To: <0f7f9eefc056dd4d9b11dab737e00235b3243a2f.1588150804.git.liu.denton@gmail.com>

Denton Liu <liu.denton@gmail.com> writes:

Show 7 quoted lines

> An alternative implementation which was considered involved inserting
> option name variants into a struct which is passed in by each command
> variant. Even though this approach is more general and could be
> applicable for future differing option names, it seemed like an
> over-engineered solution when the current pair of options are the only
> differing ones. We should probably avoid adding options which have
> different names anyway.

Sure. Or another alternative is to take "-B/-b" silently without advertising. It's not like the reason why we introduced "-C/-c" was because we wanted to reuse them in "switch" for other purposes.

Show 7 quoted lines

> Reported-by: Robert Simpson <no-reply@MailScreen.com>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>
> Notes:
>     Robert, is the email listed above correct? If not, please let me know
>     which email to use. (I hope that this reaches you somehow.)

If we do not get any response, it is OK to remove the fake e-mail address and recording only the namee (this is only OK for trailers that are not SoB; the Signed-off-by: trailers want to be more strict).

Show 5 quoted lines

> +enum cmd_variant {
> +	CMD_CHECKOUT,
> +	CMD_SWITCH,
> +	CMD_RESTORE
> +};

Yuck, but OK. Does "git restore" even take -b/-c/--orphan option? I somehow doubt it.

This is too invasive for what it achieves.

How about having a file-scope global

/* create-branch option (either b or c) */ static char cb_option = 'b';

and then ...

Show 17 quoted lines

> +
>  static int checkout_main(int argc, const char **argv, const char *prefix,
>  			 struct checkout_opts *opts, struct option *options,
> -			 const char * const usagestr[])
> +			 const char * const usagestr[],
> +			 enum cmd_variant variant)
>  {
>  	struct branch_info new_branch_info;
>  	int parseopt_flags = 0;
> @@ -1586,7 +1593,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
>  	}
>  
>  	if ((!!opts->new_branch + !!opts->new_branch_force + !!opts->new_orphan_branch) > 1)
> -		die(_("-b, -B and --orphan are mutually exclusive"));
> +		die(variant == CMD_CHECKOUT ?
> +				_("-b, -B and --orphan are mutually exclusive") :
> +				_("-c, -C and --orphan are mutually exclusive"));

... use it here like

		die(_("-%c, -%c and --orphan are mutually exclusive"),
		      cb_option, toupper(cb_option));

Show 17 quoted lines

>  	if (opts->overlay_mode == 1 && opts->patch_mode)
>  		die(_("-p and --overlay are mutually exclusive"));
> @@ -1614,7 +1623,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
>  	/*
>  	 * From here on, new_branch will contain the branch to be checked out,
>  	 * and new_branch_force and new_orphan_branch will tell us which one of
> -	 * -b/-B/--orphan is being used.
> +	 * -b/-B/-c/-C/--orphan is being used.
>  	 */
>  	if (opts->new_branch_force)
>  		opts->new_branch = opts->new_branch_force;
> @@ -1622,7 +1631,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
>  	if (opts->new_orphan_branch)
>  		opts->new_branch = opts->new_orphan_branch;
>  
> -	/* --track without -b/-B/--orphan should DWIM */
> +	/* --track without -b/-B/--orphan for checkout or -c/-C/--orphan for switch should DWIM */

Way overlong comment.  Just

	/* --track without -b/-B/-c/-C/--orphan should DWIM */

is sufficient, no?

Show 10 quoted lines

>  	if (opts->track != BRANCH_TRACK_UNSPECIFIED && !opts->new_branch) {
>  		const char *argv0 = argv[0];
>  		if (!argc || !strcmp(argv0, "--"))
> @@ -1631,7 +1640,8 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
>  		skip_prefix(argv0, "remotes/", &argv0);
>  		argv0 = strchr(argv0, '/');
>  		if (!argv0 || !argv0[1])
> -			die(_("missing branch name; try -b"));
> +			die(_("missing branch name; try -%c"),
> +					variant == CMD_CHECKOUT ? 'b' : 'c');

Likewise,

			die(_("missing branch name; try -%c"), cb_option);

>  		opts->new_branch = argv0 + 1;
>  	}

And override cb_option in one of these helpers, perhaps like ...

Show 12 quoted lines

> @@ -1785,7 +1795,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  	options = add_checkout_path_options(&opts, options);
>  
>  	ret = checkout_main(argc, argv, prefix, &opts,
> -			    options, checkout_usage);
> +			    options, checkout_usage, CMD_CHECKOUT);
>  	FREE_AND_NULL(options);
>  	return ret;
>  }
> @@ -1823,7 +1833,7 @@ int cmd_switch(int argc, const char **argv, const char *prefix)
>  	options = add_common_switch_branch_options(&opts, options);
>

... here (the other one uses 'b')

	cb_option = 'c';

Show 6 quoted lines

>  	ret = checkout_main(argc, argv, prefix, &opts,
> -			    options, switch_branch_usage);
> +			    options, switch_branch_usage, CMD_SWITCH);
>  	FREE_AND_NULL(options);
>  	return ret;
>  }

... and you do not have to change function signature of checkout_main() at all.

Show 9 quoted lines

> @@ -1860,7 +1870,7 @@ int cmd_restore(int argc, const char **argv, const char *prefix)
>  	options = add_checkout_path_options(&opts, options);
>  
>  	ret = checkout_main(argc, argv, prefix, &opts,
> -			    options, restore_usage);
> +			    options, restore_usage, CMD_RESTORE);
>  	FREE_AND_NULL(options);
>  	return ret;
>  }