* Re: [PATCH] switch: fix errors and comments related to -c and -C
2020-04-29 9:00 ` [PATCH] switch: fix errors and comments related to -c and -C Denton Liu
@ 2020-04-29 16:09 ` Taylor Blau
2020-04-29 16:31 ` Eric Sunshine
2020-04-29 16:35 ` Junio C Hamano
2020-04-30 11:54 ` [PATCH v2] " Denton Liu
2 siblings, 1 reply; 8+ messages in thread
From: Taylor Blau @ 2020-04-29 16:09 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List, Robert Simpson
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] switch: fix errors and comments related to -c and -C
2020-04-29 16:09 ` Taylor Blau
@ 2020-04-29 16:31 ` Eric Sunshine
0 siblings, 0 replies; 8+ messages in thread
From: Eric Sunshine @ 2020-04-29 16:31 UTC (permalink / raw)
To: Taylor Blau; +Cc: Denton Liu, Git Mailing List, Robert Simpson
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).
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] switch: fix errors and comments related to -c and -C
2020-04-29 9:00 ` [PATCH] switch: fix errors and comments related to -c and -C Denton Liu
2020-04-29 16:09 ` Taylor Blau
@ 2020-04-29 16:35 ` Junio C Hamano
2020-04-30 11:54 ` [PATCH v2] " Denton Liu
2 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2020-04-29 16:35 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List, Robert Simpson
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;
> }
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] switch: fix errors and comments related to -c and -C
2020-04-29 9:00 ` [PATCH] switch: fix errors and comments related to -c and -C Denton Liu
2020-04-29 16:09 ` Taylor Blau
2020-04-29 16:35 ` Junio C Hamano
@ 2020-04-30 11:54 ` Denton Liu
2020-04-30 16:21 ` Taylor Blau
2 siblings, 1 reply; 8+ messages in thread
From: Denton Liu @ 2020-04-30 11:54 UTC (permalink / raw)
To: Git Mailing List
Cc: Robert Simpson, Taylor Blau, Eric Sunshine, Junio C Hamano
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), 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 error messages that refer to `-b` and `-B`, use a format string
instead so that `-c` and `-C` can be printed when `git switch` is
invoked.
Reported-by: Robert Simpson
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.)
Range-diff against v1:
1: 0f7f9eefc0 ! 1: 75c9cf6ce9 switch: fix errors and comments related to -c and -C
@@ Commit message
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`.
+ b7b5fce270 (switch: better names for -b and -B, 2019-03-29), 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` and `-B`, use a format string
+ instead so that `-c` and `-C` can be printed when `git switch` is
+ invoked.
- 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>
+ Reported-by: Robert Simpson
## Notes ##
@@ builtin/checkout.c: static struct option *add_checkout_path_options(struct check
return newopts;
}
-+enum cmd_variant {
-+ CMD_CHECKOUT,
-+ CMD_SWITCH,
-+ CMD_RESTORE
-+};
++/* create-branch option (either b or c) */
++static char cb_option = 'b';
+
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;
+ const char * const usagestr[])
@@ builtin/checkout.c: 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"));
++ 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"));
@@ builtin/checkout.c: static int checkout_main(int argc, const char **argv, const
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 */
++ /* --track without -c/-C/-b/-B/--orphan should DWIM */
if (opts->track != BRANCH_TRACK_UNSPECIFIED && !opts->new_branch) {
const char *argv0 = argv[0];
if (!argc || !strcmp(argv0, "--"))
@@ builtin/checkout.c: static int checkout_main(int argc, const char **argv, const
if (!argv0 || !argv0[1])
- die(_("missing branch name; try -b"));
+ die(_("missing branch name; try -%c"),
-+ variant == CMD_CHECKOUT ? 'b' : 'c');
++ cb_option);
opts->new_branch = argv0 + 1;
}
-@@ builtin/checkout.c: 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;
- }
@@ builtin/checkout.c: int cmd_switch(int argc, const char **argv, const char *prefix)
+ options = add_common_options(&opts, options);
options = add_common_switch_branch_options(&opts, options);
++ cb_option = 'c';
++
ret = checkout_main(argc, argv, prefix, &opts,
-- options, switch_branch_usage);
-+ options, switch_branch_usage, CMD_SWITCH);
+ options, switch_branch_usage);
FREE_AND_NULL(options);
- return ret;
- }
-@@ builtin/checkout.c: 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;
- }
builtin/checkout.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8bc94d392b..a45519a2b4 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1544,6 +1544,9 @@ static struct option *add_checkout_path_options(struct checkout_opts *opts,
return newopts;
}
+/* create-branch option (either b or c) */
+static char cb_option = 'b';
+
static int checkout_main(int argc, const char **argv, const char *prefix,
struct checkout_opts *opts, struct option *options,
const char * const usagestr[])
@@ -1586,7 +1589,8 @@ 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(_("-%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 +1618,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 +1626,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 -c/-C/-b/-B/--orphan should DWIM */
if (opts->track != BRANCH_TRACK_UNSPECIFIED && !opts->new_branch) {
const char *argv0 = argv[0];
if (!argc || !strcmp(argv0, "--"))
@@ -1631,7 +1635,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"),
+ cb_option);
opts->new_branch = argv0 + 1;
}
@@ -1822,6 +1827,8 @@ int cmd_switch(int argc, const char **argv, const char *prefix)
options = add_common_options(&opts, options);
options = add_common_switch_branch_options(&opts, options);
+ cb_option = 'c';
+
ret = checkout_main(argc, argv, prefix, &opts,
options, switch_branch_usage);
FREE_AND_NULL(options);
--
2.26.2.548.gbb00c8a0a9
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] switch: fix errors and comments related to -c and -C
2020-04-30 11:54 ` [PATCH v2] " Denton Liu
@ 2020-04-30 16:21 ` Taylor Blau
2020-04-30 20:45 ` Junio C Hamano
0 siblings, 1 reply; 8+ messages in thread
From: Taylor Blau @ 2020-04-30 16:21 UTC (permalink / raw)
To: Denton Liu
Cc: Git Mailing List, Robert Simpson, Taylor Blau, Eric Sunshine,
Junio C Hamano
On Thu, Apr 30, 2020 at 07:54:57AM -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), 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 error messages that refer to `-b` and `-B`, use a format string
> instead so that `-c` and `-C` can be printed when `git switch` is
> invoked.
>
> Reported-by: Robert Simpson
> 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.)
Nicely done. This revision looks great to me, thanks.
Reviewed-by: Taylor Blau <me@ttaylorr.com>
Thanks,
Taylor
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] switch: fix errors and comments related to -c and -C
2020-04-30 16:21 ` Taylor Blau
@ 2020-04-30 20:45 ` Junio C Hamano
0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2020-04-30 20:45 UTC (permalink / raw)
To: Taylor Blau; +Cc: Denton Liu, Git Mailing List, Robert Simpson, Eric Sunshine
Taylor Blau <me@ttaylorr.com> writes:
> Nicely done. This revision looks great to me, thanks.
>
> Reviewed-by: Taylor Blau <me@ttaylorr.com>
I'd squeeze this into a single line, though.
> - die(_("missing branch name; try -b"));
> + die(_("missing branch name; try -%c"),
> + cb_option);
Will queue with a local fix-up for the above.
Thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread