Re: [PATCH] switch: fix errors and comments related to -c and -C
From: Taylor Blau <hidden>
Date: 2020-04-29 16:09:52
Hi Denton, On Wed, Apr 29, 2020 at 05:00:19AM -0400, Denton Liu wrote:
In d787d311db (checkout: split part of it to new command 'switch', 2019-03-29), the `git switch` command was created by extracting the common functionality of cmd_checkout() in checkout_main(). However, in b7b5fce270 (switch: better names for -b and -B, 2019-03-29), these the branch creation and force creation options for 'switch' were changed to -c and -C, respectively. As a result of this, error messages and comments that previously referred to `-b` and `-B` became invalid for `git switch`. For comments that refer to `-b` and `-B`, add `-c` and `-C` to the comment.
I had to read this sentence a couple of times more than I would have liked to in order to grok it fully. Would it be perhaps clearer as: Update comments in 'cmd_checkout()' that mention `-b` or `-B` to instead refer to `-c` or `-C` when invoked from 'git switch'. ?
For error messages that refer to `-b`, introduce `enum cmd_variant` and use it to differentiate between `checkout` and `switch` when printing out error messages. 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.
Yeah, I don't think we should spend much time trying to figure out a general solution here when these are the only differing pair.
Reported-by: Robert Simpson <redacted>
Signed-off-by: Denton Liu <redacted>
---
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.)I'll be shocked if this is his real email address ;).
quoted hunk ↗ jump to hunk
builtin/checkout.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-)diff --git a/builtin/checkout.c b/builtin/checkout.c index 8bc94d392b..0ca74cde08 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c@@ -1544,9 +1544,16 @@ static struct option *add_checkout_path_options(struct checkout_opts *opts, return newopts; } +enum cmd_variant { + CMD_CHECKOUT, + CMD_SWITCH, + CMD_RESTORE +}; + 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"));
Hmm. Do we need to generate an extra string for translation here? If the
string was instead:
_("%s and --orphan are mutually exclusive")
where the first format string is filled in something like:
die(_("%s and --orphan are mutually exclusive"),
variant == CMD_CHECKOUT ? "-b, -B" : "-c, -C");
may save translators some work.
quoted hunk ↗ jump to hunk
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 */
This line is getting a little long. Would you mind wrapping this as a multi-line comment instead?
quoted hunk ↗ jump to hunk
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'); opts->new_branch = argv0 + 1; }@@ -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); ret = checkout_main(argc, argv, prefix, &opts, - options, switch_branch_usage); + options, switch_branch_usage, CMD_SWITCH); FREE_AND_NULL(options); return ret; }@@ -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; } --2.26.2.548.gbb00c8a0a9
All of the rest makes sense, thanks. Thanks, Taylor