| Message ID | 0f7f9eefc056dd4d9b11dab737e00235b3243a2f.1588150804.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | switch: fix errors and comments related to -c and -C | expand |
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 <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.) I'll be shocked if this is his real email address ;). > 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. > 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? > 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
On Wed, Apr 29, 2020 at 12:10 PM Taylor Blau <me@ttaylorr.com> wrote: > 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 s/these the/the/ > > 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'. I had no problem groking Denton's wording but had to re-read this proposal several times before understanding it. I could try providing yet another proposal, however, I think the entire sentence can simply be dropped (after all, it's just stating the obvious). > > + 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. We don't know the grammatical or syntactic rules of each language, so hard-coding untranslatable "-b, -B" is contraindicated. Since the option letters ought not be translated (just as "--orphan" shouldn't be), the letters themselves could be interpolated into the string. However, that's probably less helpful for translators since it eliminates contextual clues. Therefore, it seems like a good idea to leave it as two separate translatable strings (as the patch already does).
Denton Liu <liu.denton@gmail.com> writes: > 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. > 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). > +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 ... > + > 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)); > 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? > 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 ... > @@ -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'; > 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. > @@ -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; > }
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")); 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 */ 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; }
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. 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. 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.) builtin/checkout.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-)