| Message ID | 20220330190956.21447-1-garrit@slashdev.space (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] cli: add -v and -h shorthands | expand |
Garrit Franke <garrit@slashdev.space> writes: > Change the behavior of "git -v" to be synonymous with "--version" / > "version", and "git -h" to be synonymous with "--help", but not "help". > > These shorthands both display the "unknown option" message. Following > this change, "-v" displays the version, and "-h" displays the help text > of the "git" command. Sorry, but it is unclear why it is a good thing. > It should be noted that the "-v" shorthand could be misinterpreted by > the user to mean "verbose" instead of "version", since some sub-commands > make use of it in this context. The top-level "git" command does not > have a "verbose" flag, so it's safe to introduce this shorthand > unambiguously. Again, it might be safe right now, but it also closes the door for introducing global "verbose" option. What in exchange are we gaining? Are these short options worth it? I am not convinced.
On Wed, Mar 30 2022, Junio C Hamano wrote: > Sorry, but it is unclear why it is a good thing. My main motivation behind this change was a standardized user experience across tools. Many users use these shorthands out of habit to get an overview of a program. Seeing the command fail and having to retype it in a longer form creates unnecessary friction between the user and the program. > Again, it might be safe right now, but it also closes the door for > introducing global "verbose" option. What in exchange are we > gaining? Are these short options worth it? I definitely see your concerns. Ultimately it's a question of which of the two flags would be more convinient to have as a shorthand. As stated above, users unfamiliar to the software arguably expect to see the version number printed when using this flag. A user who seeks more verbose output is probably more familiar with the options, so they are more likely to know they have to use the longer "--verbose" form. Thanks Garrit
On Thu, Mar 31 2022, Garrit Franke wrote: > On Wed, Mar 30 2022, Junio C Hamano wrote: > >> Sorry, but it is unclear why it is a good thing. > > My main motivation behind this change was a standardized user > experience across tools. Many users use these shorthands out of habit > to get an overview of a program. Seeing the command fail and having to > retype it in a longer form creates unnecessary friction between the > user and the program. I think this is a good trade-off in this case. I.e. -v and -h are commonly understood. >> Again, it might be safe right now, but it also closes the door for >> introducing global "verbose" option. What in exchange are we >> gaining? Are these short options worth it? > > I definitely see your concerns. Ultimately it's a question of which of > the two flags would be more convinient to have as a shorthand. As > stated above, users unfamiliar to the software arguably expect to see > the version number printed when using this flag. A user who seeks more > verbose output is probably more familiar with the options, so they are > more likely to know they have to use the longer "--verbose" form. This was somewhat discussed in/around https://lore.kernel.org/git/87zgs593ja.fsf@evledraar.gmail.com/, i.e. there I was arguing the counter-point, that we should have things like "git --no-progress subcmd <opts>", rather than just "git subcmd --no-progress <other-opts>". But I think the alternative won the day (at least in that commit-graph case), and in retrospect I think it was the right call for our UI in general. I still think it makes sense for git to understand a thing like --object-format at the top-level, and maybe/probably --progress, but --verbose is probably too confusing. I.e. it would imply verbose output for all commands (which they're much less likely to have/provide than the other two).
On 31.03.22 02:07, Ævar Arnfjörð Bjarmason wrote: > > On Thu, Mar 31 2022, Garrit Franke wrote: > >> On Wed, Mar 30 2022, Junio C Hamano wrote: >> >>> Sorry, but it is unclear why it is a good thing. >> >> My main motivation behind this change was a standardized user >> experience across tools. Many users use these shorthands out of habit >> to get an overview of a program. Seeing the command fail and having to >> retype it in a longer form creates unnecessary friction between the >> user and the program. > > I think this is a good trade-off in this case. I.e. -v and -h are > commonly understood. An interesting observation I just made is that curl [0] uses both "--verbose" and "--version" on the top level [1][2] including shorthands. "-v" corresponds to "verbose", "-V" corresponds to "version. Not that I'm a fan of this clutter, but it's a possible path to go down if we actually needed a second shorthand using this letter. [0]: https://github.com/curl/curl [1]: https://github.com/curl/curl/blob/master/docs/cmdline-opts/version.d [2]: https://github.com/curl/curl/blob/master/docs/cmdline-opts/verbose.d
Garrit Franke <garrit@slashdev.space> writes: > On 31.03.22 02:07, Ævar Arnfjörð Bjarmason wrote: > >> I think this is a good trade-off in this case. I.e. -v and -h are >> commonly understood. > > An interesting observation I just made is that curl [0] uses both > "--verbose" and "--version" on the top level [1][2] including > shorthands. "-v" corresponds to "verbose", "-V" corresponds to > "version. > > Not that I'm a fan of this clutter, but it's a possible path to go > down if we actually needed a second shorthand using this letter. Do you mean you want to use "-V" for version, instead of the "-v" used in the patch, so that "-v" can be left for "--verbose"? I am not sure consistency with whom we are aiming for anymore with that mixed to the proposal X-<. > const char git_usage_string[] = > - N_("git [--version] [--help] [-C <path>] [-c <name>=<value>]\n" > + N_("git [-v | --version] [-h | --help] [-C <path>] [-c <name>=<value>]\n" > " [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]\n" > " [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--bare]\n" > " [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]\n" This is an improvement in that the first line that used to be unusually shorter now becomes comparable length to other lines ;-) > @@ -146,7 +146,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) > * commands can be written with "--" prepended > * to make them look like flags. > */ > - if (!strcmp(cmd, "--help") || !strcmp(cmd, "--version")) > + if (!strcmp(cmd, "--help") || !strcmp(cmd, "--version") || > + !strcmp(cmd, "-h") || !strcmp(cmd, "-v")) > break; OK. > @@ -893,7 +894,12 @@ int cmd_main(int argc, const char **argv) > handle_options(&argv, &argc, NULL); > if (argc > 0) { > /* translate --help and --version into commands */ > - skip_prefix(argv[0], "--", &argv[0]); > + if (!strcmp("-v", argv[0])) > + argv[0] = "version"; > + else if (!strcmp("-h", argv[0])) > + argv[0] = "help"; > + else > + skip_prefix(argv[0], "--", &argv[0]); I find this unnecessarily hard to read. (Side note) A tip for reviewers. Be suspicious of any change that adds new things _in front_ of existing sequence. Question if there is a good justification for it. If there isn't, see if it makes it better if you instead append the new stuff to existing sequence. If neither results in satisfying code, perhaps it is good time to totally rewrite it to make both existing and new stuff fit in the flow. The original was in a sense a bit sloppy to strip "--" from anything that begins with "--", when we only care about "--verbose" and "--help" and nothing else (granted, handle_options() would barf when argv[0] begins with "--" and is not one of these two, so the sloppiness does not make a difference in practice). If we accept the same kind of sloppiness, perhaps if (skip_prefix(argv[0], "--", &argv[0])) ; /* removed "--" from "--help" and "--version" */ else if (argv[0][0] == '-') argv[0] = argv[0][1] == 'v' ? "version" : "help"; would make the new side match the existing one better, but I would not necessarily recommend it. We may want to be a bit more explicit and readable, by spelling out the expectation, i.e. if (!strcmp("--version", argv[0]) || !strcmp("-v", argv[0])) argv[0] = "version"; else if (!strcmp("--help", argv[0]) || !strcmp("-h", argv[0])) argv[0] = "help"; This makes it clear that these two pseudo-commands, spelled with a dash in front and stand for other commands, are the only thing we care about and what their accepted spelling are. Hmm?
On Thu, Mar 31 2022, Junio C Hamano wrote: > Garrit Franke <garrit@slashdev.space> writes: > >> On 31.03.22 02:07, Ævar Arnfjörð Bjarmason wrote: >> >>> I think this is a good trade-off in this case. I.e. -v and -h are >>> commonly understood. >> >> An interesting observation I just made is that curl [0] uses both >> "--verbose" and "--version" on the top level [1][2] including >> shorthands. "-v" corresponds to "verbose", "-V" corresponds to >> "version. >> >> Not that I'm a fan of this clutter, but it's a possible path to go >> down if we actually needed a second shorthand using this letter. > > Do you mean you want to use "-V" for version, instead of the "-v" > used in the patch, so that "-v" can be left for "--verbose"? > > I am not sure consistency with whom we are aiming for anymore with > that mixed to the proposal X-<. Maybe you've been convinced to accept -v and -h (I don't care much either way, honestly), but if not I wonder if this alternate approach would make everyone in this thread happy (or at least mostly so): diff --git a/git.c b/git.c index a25940d72e8..929faa12537 100644 --- a/git.c +++ b/git.c @@ -323,7 +323,12 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) exit(list_cmds(cmd)); } } else { - fprintf(stderr, _("unknown option: %s\n"), cmd); + if (!strcmp(cmd, "-h")) + fprintf(stderr, _("unknown option: %s, did you mean --help?\n"), cmd); + else if (!strcmp(cmd, "-v")) + fprintf(stderr, _("unknown option: %s, did you mean --version?\n"), cmd); + else + fprintf(stderr, _("unknown option: %s\n"), cmd); usage(git_usage_string); } (Better to pass those as a parameter to the translation, but it's just a throwaway demo patch for discussion) I.e. we could help the user in these cases by suggesting that they may have meant --help or --version, while still leaving the door open to using these short options for something else. In git.c we don't use parse-options.c, but that might be a useful addition for it in general. I.e. to allow an option to define a list of known alternative (but not understood!) aliases for itself. So for the cases where we're about to emit an error about -v or whatever anyway, we could be a bit more helpful and suggest --version, even if we didn't want to define that as a short option for whatever reason.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> Do you mean you want to use "-V" for version, instead of the "-v" >> used in the patch, so that "-v" can be left for "--verbose"? >> >> I am not sure consistency with whom we are aiming for anymore with >> that mixed to the proposal X-<. > > Maybe you've been convinced to accept -v and -h (I don't care much > either way, honestly), but if not I wonder if this alternate approach > would make everyone in this thread happy (or at least mostly so): I don't care too much about being similar to stuff made by other people, compared to how much I care about internal consistency ;-) I do not think one extra level of "unknown option", even with hint, is worth the trouble. If we want to cater to those who expect "-h" to be more special than random "-<some single letter>", we should go all the way and make "-h" truly supported. If we do not, they can read "git help git" just like those who wonder what "git -X" means.
On 01.04.22 18:02, Junio C Hamano wrote: > I do not think one extra level of "unknown option", even with hint, > is worth the trouble. If we want to cater to those who expect "-h" > to be more special than random "-<some single letter>", we should go > all the way and make "-h" truly supported. If we do not, they can > read "git help git" just like those who wonder what "git -X" means. From what I gathered, we all agree that adding the "-h" shorthand is a good addition to the UI. Given that the "-v" option is understandably controversial, I could cut it from this patch. Thoughts?
Garrit Franke <garrit@slashdev.space> writes: > On 01.04.22 18:02, Junio C Hamano wrote: > >> I do not think one extra level of "unknown option", even with hint, >> is worth the trouble. If we want to cater to those who expect "-h" >> to be more special than random "-<some single letter>", we should go >> all the way and make "-h" truly supported. If we do not, they can >> read "git help git" just like those who wonder what "git -X" means. > > From what I gathered, we all agree that adding the "-h" shorthand is a > good addition to the UI. Given that the "-v" option is understandably > controversial, I could cut it from this patch. > > Thoughts? I do not care too deeply, and it is the most time efficient to just take the last patch as-is. Nobody would care they used "git -h" or "git -v" only once and learned to use "git help" and "git version" anyway ;-)
diff --git a/Documentation/git.txt b/Documentation/git.txt index 13f83a2..302607a 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -9,7 +9,7 @@ git - the stupid content tracker SYNOPSIS -------- [verse] -'git' [--version] [--help] [-C <path>] [-c <name>=<value>] +'git' [-v | --version] [-h | --help] [-C <path>] [-c <name>=<value>] [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path] [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare] [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>] @@ -39,6 +39,7 @@ or https://git-scm.com/docs. OPTIONS ------- +-v:: --version:: Prints the Git suite version that the 'git' program came from. + @@ -46,6 +47,7 @@ This option is internally converted to `git version ...` and accepts the same options as the linkgit:git-version[1] command. If `--help` is also given, it takes precedence over `--version`. +-h:: --help:: Prints the synopsis and a list of the most commonly used commands. If the option `--all` or `-a` is given then all diff --git a/git.c b/git.c index a25940d..024d463 100644 --- a/git.c +++ b/git.c @@ -25,7 +25,7 @@ struct cmd_struct { }; const char git_usage_string[] = - N_("git [--version] [--help] [-C <path>] [-c <name>=<value>]\n" + N_("git [-v | --version] [-h | --help] [-C <path>] [-c <name>=<value>]\n" " [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]\n" " [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--bare]\n" " [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]\n" @@ -146,7 +146,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) * commands can be written with "--" prepended * to make them look like flags. */ - if (!strcmp(cmd, "--help") || !strcmp(cmd, "--version")) + if (!strcmp(cmd, "--help") || !strcmp(cmd, "--version") || + !strcmp(cmd, "-h") || !strcmp(cmd, "-v")) break; /* @@ -893,7 +894,12 @@ int cmd_main(int argc, const char **argv) handle_options(&argv, &argc, NULL); if (argc > 0) { /* translate --help and --version into commands */ - skip_prefix(argv[0], "--", &argv[0]); + if (!strcmp("-v", argv[0])) + argv[0] = "version"; + else if (!strcmp("-h", argv[0])) + argv[0] = "help"; + else + skip_prefix(argv[0], "--", &argv[0]); } else { /* The user didn't specify a command; give them help */ commit_pager_choice(); diff --git a/t/t0012-help.sh b/t/t0012-help.sh index 6c3e1f7..6c33a43 100755 --- a/t/t0012-help.sh +++ b/t/t0012-help.sh @@ -181,7 +181,7 @@ for cmd in git "git help" do test_expect_success "'$cmd' section spacing" ' test_section_spacing_trailer git help <<-\EOF && - usage: git [--version] [--help] [-C <path>] [-c <name>=<value>] + usage: git [-v | --version] [-h | --help] [-C <path>] [-c <name>=<value>] These are common Git commands used in various situations: