* [PATCH 1/2] git: add NO_INTERNAL_HELP flag for builtins
2017-10-15 13:26 ` René Scharfe
@ 2017-10-17 15:39 ` René Scharfe
2017-10-17 17:39 ` Jonathan Nieder
2017-10-17 15:39 ` [PATCH 2/2] ls-remote: use PARSE_OPT_NO_INTERNAL_HELP René Scharfe
2017-10-17 15:39 ` [Alt. PATCH] ls-remote: deprecate -h as short for --heads René Scharfe
2 siblings, 1 reply; 23+ messages in thread
From: René Scharfe @ 2017-10-17 15:39 UTC (permalink / raw)
To: Git Mailing List
Cc: Martin Ågren, Thomas Rikl, Thomas Gummerer, Jonathan Nieder,
Junio C Hamano, Jeff King
Builtin commands have skipped repo setup when called with just a single
option -h and thus shown their short help text even outside of
repositories since 99caeed05d3 (Let 'git <command> -h' show usage
without a git dir).
Add the flag NO_INTERNAL_HELP for builtins to opt out of this special
treatment and provide a list of commands that are exempt from the
corresponding test in t0012. That allows builtins to handle -h
themselves.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
git.c | 6 +++++-
t/t0012-help.sh | 9 ++++++++-
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/git.c b/git.c
index 9e96dd4090..9d1b8c4716 100644
--- a/git.c
+++ b/git.c
@@ -298,6 +298,7 @@ static int handle_alias(int *argcp, const char ***argv)
#define NEED_WORK_TREE (1<<3)
#define SUPPORT_SUPER_PREFIX (1<<4)
#define DELAY_PAGER_CONFIG (1<<5)
+#define NO_INTERNAL_HELP (1<<6)
struct cmd_struct {
const char *cmd;
@@ -312,7 +313,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
const char *prefix;
prefix = NULL;
- help = argc == 2 && !strcmp(argv[1], "-h");
+ if (p->option & NO_INTERNAL_HELP)
+ help = 0;
+ else
+ help = argc == 2 && !strcmp(argv[1], "-h");
if (!help) {
if (p->option & RUN_SETUP)
prefix = setup_git_directory();
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 487b92a5de..74eeead168 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -53,12 +53,19 @@ test_expect_success 'generate builtin list' '
git --list-builtins >builtins
'
+cat >no_internal_help <<EOF
+EOF
+
+test_expect_success 'generate list of builtins with internal help' '
+ grep -w -v -f no_internal_help <builtins >builtins_internal_help
+'
+
while read builtin
do
test_expect_success "$builtin can handle -h" '
test_expect_code 129 git $builtin -h >output 2>&1 &&
test_i18ngrep usage output
'
-done <builtins
+done <builtins_internal_help
test_done
--
2.14.2
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 1/2] git: add NO_INTERNAL_HELP flag for builtins
2017-10-17 15:39 ` [PATCH 1/2] git: add NO_INTERNAL_HELP flag for builtins René Scharfe
@ 2017-10-17 17:39 ` Jonathan Nieder
0 siblings, 0 replies; 23+ messages in thread
From: Jonathan Nieder @ 2017-10-17 17:39 UTC (permalink / raw)
To: René Scharfe
Cc: Git Mailing List, Martin Ågren, Thomas Rikl, Thomas Gummerer,
Junio C Hamano, Jeff King
Hi,
René Scharfe wrote:
> Builtin commands have skipped repo setup when called with just a single
> option -h and thus shown their short help text even outside of
> repositories since 99caeed05d3 (Let 'git <command> -h' show usage
> without a git dir).
>
> Add the flag NO_INTERNAL_HELP for builtins to opt out of this special
> treatment and provide a list of commands that are exempt from the
> corresponding test in t0012. That allows builtins to handle -h
> themselves.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> git.c | 6 +++++-
> t/t0012-help.sh | 9 ++++++++-
> 2 files changed, 13 insertions(+), 2 deletions(-)
Makes sense.
[...]
> +++ b/git.c
[...]
> @@ -312,7 +313,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
> const char *prefix;
>
> prefix = NULL;
> - help = argc == 2 && !strcmp(argv[1], "-h");
> + if (p->option & NO_INTERNAL_HELP)
> + help = 0;
> + else
> + help = argc == 2 && !strcmp(argv[1], "-h");
optional: this could be part of the same expression:
help = !(p->option & NO_INTERNAL_HELP) && argc == 2 && ...;
[...]
> --- a/t/t0012-help.sh
> +++ b/t/t0012-help.sh
> @@ -53,12 +53,19 @@ test_expect_success 'generate builtin list' '
> git --list-builtins >builtins
> '
>
> +cat >no_internal_help <<EOF
> +EOF
> +
> +test_expect_success 'generate list of builtins with internal help' '
> + grep -w -v -f no_internal_help <builtins >builtins_internal_help
> +'
Hm, I don't see any instances of "grep -f" in the testsuite. Are
there portability pitfalls in it I don't know about? It's in POSIX,
so it looks pretty safe.
I would have been tempted to use comm, which is already used in
t6500-gc.sh:
comm -1 -3 no_internal_help builtins >builtins_internal_help
Other nits:
- preparatory 'cat' commands like the above tend to go inside
test_expect_success these days
- test that set up for later tests get marked as 'setup' or 'set up'
With whatever subset of the changes below looks good squashed in,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks.
diff --git i/git.c w/git.c
index 9d1b8c4716..e4b340df7d 100644
--- i/git.c
+++ w/git.c
@@ -313,10 +313,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
const char *prefix;
prefix = NULL;
- if (p->option & NO_INTERNAL_HELP)
- help = 0;
- else
- help = argc == 2 && !strcmp(argv[1], "-h");
+ help = !(p->option & NO_INTERNAL_HELP) &&
+ argc == 2 && !strcmp(argv[1], "-h");
if (!help) {
if (p->option & RUN_SETUP)
prefix = setup_git_directory();
diff --git i/t/t0012-help.sh w/t/t0012-help.sh
index c5a748837c..73fdfd99ab 100755
--- i/t/t0012-help.sh
+++ w/t/t0012-help.sh
@@ -49,15 +49,11 @@ test_expect_success "--help does not work for guides" "
test_i18ncmp expect actual
"
-test_expect_success 'generate builtin list' '
- git --list-builtins >builtins
-'
-
-cat >no_internal_help <<EOF
-EOF
-
-test_expect_success 'generate list of builtins with internal help' '
- grep -w -v -f no_internal_help <builtins >builtins_internal_help
+test_expect_success 'set up list of builtins with internal help' '
+ cat >no_internal_help <<-\EOF &&
+ EOF
+ git --list-builtins >builtins &&
+ comm -1 -3 no_internal_help builtins >builtins_internal_help
'
while read builtin
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 2/2] ls-remote: use PARSE_OPT_NO_INTERNAL_HELP
2017-10-15 13:26 ` René Scharfe
2017-10-17 15:39 ` [PATCH 1/2] git: add NO_INTERNAL_HELP flag for builtins René Scharfe
@ 2017-10-17 15:39 ` René Scharfe
2017-10-17 17:42 ` Jonathan Nieder
2017-10-17 17:42 ` Martin Ågren
2017-10-17 15:39 ` [Alt. PATCH] ls-remote: deprecate -h as short for --heads René Scharfe
2 siblings, 2 replies; 23+ messages in thread
From: René Scharfe @ 2017-10-17 15:39 UTC (permalink / raw)
To: Git Mailing List
Cc: Martin Ågren, Thomas Rikl, Thomas Gummerer, Jonathan Nieder,
Junio C Hamano, Jeff King
Since ba5f28bf79 (ls-remote: use parse-options api) git ls-remote -h
without any other options has shown the short help text of the command
instead of acting as the short equivalent of --heads. Fix this
regression by turning off internal handling of -h for repository setup,
and option parsing, as well as the corresponding test in t0012.
Reported-by: Thomas Rikl <trikl@online.de>
Analyzed-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
builtin/ls-remote.c | 1 +
git.c | 2 +-
t/t0012-help.sh | 1 +
t/t5512-ls-remote.sh | 6 ++++++
4 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index c4be98ab9e..5f27d252b4 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -68,6 +68,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
};
argc = parse_options(argc, argv, prefix, options, ls_remote_usage,
+ PARSE_OPT_NO_INTERNAL_HELP |
PARSE_OPT_STOP_AT_NON_OPTION);
dest = argv[0];
diff --git a/git.c b/git.c
index 9d1b8c4716..056a123506 100644
--- a/git.c
+++ b/git.c
@@ -421,7 +421,7 @@ static struct cmd_struct commands[] = {
{ "interpret-trailers", cmd_interpret_trailers, RUN_SETUP_GENTLY },
{ "log", cmd_log, RUN_SETUP },
{ "ls-files", cmd_ls_files, RUN_SETUP },
- { "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
+ { "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY | NO_INTERNAL_HELP },
{ "ls-tree", cmd_ls_tree, RUN_SETUP },
{ "mailinfo", cmd_mailinfo, RUN_SETUP_GENTLY },
{ "mailsplit", cmd_mailsplit },
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 74eeead168..05d8cf9b49 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -54,6 +54,7 @@ test_expect_success 'generate builtin list' '
'
cat >no_internal_help <<EOF
+ls-remote
EOF
test_expect_success 'generate builtin list with internal help' '
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 02106c9226..637e9a87f3 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -264,4 +264,10 @@ test_expect_success 'ls-remote works outside repository' '
nongit git ls-remote dst.git
'
+test_expect_success 'ls-remote -h == ls-remote --heads' '
+ git ls-remote -h >short &&
+ git ls-remote --heads >long &&
+ test_cmp long short
+'
+
test_done
--
2.14.2
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 2/2] ls-remote: use PARSE_OPT_NO_INTERNAL_HELP
2017-10-17 15:39 ` [PATCH 2/2] ls-remote: use PARSE_OPT_NO_INTERNAL_HELP René Scharfe
@ 2017-10-17 17:42 ` Jonathan Nieder
2017-10-17 17:42 ` Martin Ågren
1 sibling, 0 replies; 23+ messages in thread
From: Jonathan Nieder @ 2017-10-17 17:42 UTC (permalink / raw)
To: René Scharfe
Cc: Git Mailing List, Martin Ågren, Thomas Rikl, Thomas Gummerer,
Junio C Hamano, Jeff King
René Scharfe wrote:
> Since ba5f28bf79 (ls-remote: use parse-options api) git ls-remote -h
> without any other options has shown the short help text of the command
> instead of acting as the short equivalent of --heads. Fix this
> regression by turning off internal handling of -h for repository setup,
> and option parsing, as well as the corresponding test in t0012.
>
> Reported-by: Thomas Rikl <trikl@online.de>
> Analyzed-by: Martin Ågren <martin.agren@gmail.com>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> builtin/ls-remote.c | 1 +
> git.c | 2 +-
> t/t0012-help.sh | 1 +
> t/t5512-ls-remote.sh | 6 ++++++
> 4 files changed, 9 insertions(+), 1 deletion(-)
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
[...]
> --- a/git.c
> +++ b/git.c
> @@ -421,7 +421,7 @@ static struct cmd_struct commands[] = {
> { "interpret-trailers", cmd_interpret_trailers, RUN_SETUP_GENTLY },
> { "log", cmd_log, RUN_SETUP },
> { "ls-files", cmd_ls_files, RUN_SETUP },
> - { "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
> + { "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY | NO_INTERNAL_HELP },
There's only one other command that uses PARSE_OPT_NO_INTERNAL_HELP, and
that's "git archive". Does it need the same treatment?
More generally, is there a straightforward way for parse-options or
some compile-time analysis to catch if we forget to add
NO_INTERNAL_HELP to a command in the commands[] table?
Thanks,
Jonathan
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 2/2] ls-remote: use PARSE_OPT_NO_INTERNAL_HELP
2017-10-17 15:39 ` [PATCH 2/2] ls-remote: use PARSE_OPT_NO_INTERNAL_HELP René Scharfe
2017-10-17 17:42 ` Jonathan Nieder
@ 2017-10-17 17:42 ` Martin Ågren
1 sibling, 0 replies; 23+ messages in thread
From: Martin Ågren @ 2017-10-17 17:42 UTC (permalink / raw)
To: René Scharfe
Cc: Git Mailing List, Thomas Rikl, Thomas Gummerer, Jonathan Nieder,
Junio C Hamano, Jeff King
On 17 October 2017 at 17:39, René Scharfe <l.s.r@web.de> wrote:
> Since ba5f28bf79 (ls-remote: use parse-options api) git ls-remote -h
> without any other options has shown the short help text of the command
> instead of acting as the short equivalent of --heads. Fix this
> regression by turning off internal handling of -h for repository setup,
> and option parsing, as well as the corresponding test in t0012.
>
> Reported-by: Thomas Rikl <trikl@online.de>
> Analyzed-by: Martin Ågren <martin.agren@gmail.com>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> builtin/ls-remote.c | 1 +
> git.c | 2 +-
> t/t0012-help.sh | 1 +
> t/t5512-ls-remote.sh | 6 ++++++
> 4 files changed, 9 insertions(+), 1 deletion(-)
Documentation/gitcli.txt might need updating. It says that "[c]ommands
which have the enhanced option parser activated all understand ... -h".
Of course, it already was in an incorrect state, since it pretends like
no-one uses PARSE_OPT_NO_INTERNAL_HELP. Probably better to leave that
until it's clear in which direction "git ls-remote -h" should go.
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Alt. PATCH] ls-remote: deprecate -h as short for --heads
2017-10-15 13:26 ` René Scharfe
2017-10-17 15:39 ` [PATCH 1/2] git: add NO_INTERNAL_HELP flag for builtins René Scharfe
2017-10-17 15:39 ` [PATCH 2/2] ls-remote: use PARSE_OPT_NO_INTERNAL_HELP René Scharfe
@ 2017-10-17 15:39 ` René Scharfe
2017-10-17 17:40 ` Martin Ågren
` (2 more replies)
2 siblings, 3 replies; 23+ messages in thread
From: René Scharfe @ 2017-10-17 15:39 UTC (permalink / raw)
To: Git Mailing List
Cc: Martin Ågren, Thomas Rikl, Thomas Gummerer, Junio C Hamano,
Jonathan Nieder, Jeff King
Stop advertising -h as the short equivalent of --heads, because it's
used for showing a short help text for almost all other git commands.
Since the ba5f28bf79 (ls-remote: use parse-options api) it has only
been working when used together with other parameters anyway.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
That would be step on the way towards more consistent command line
switches, in the same vein as d69155119 (t0012: test "-h" with
builtins).
Documentation/git-ls-remote.txt | 1 -
builtin/ls-remote.c | 4 +++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 5f2628c8f8..898836a111 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -21,7 +21,6 @@ commit IDs.
OPTIONS
-------
--h::
--heads::
-t::
--tags::
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index c4be98ab9e..840deedbef 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -56,7 +56,9 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
N_("path of git-upload-pack on the remote host"),
PARSE_OPT_HIDDEN },
OPT_BIT('t', "tags", &flags, N_("limit to tags"), REF_TAGS),
- OPT_BIT('h', "heads", &flags, N_("limit to heads"), REF_HEADS),
+ OPT_BIT(0, "heads", &flags, N_("limit to heads"), REF_HEADS),
+ { OPTION_BIT, 'h', NULL, &flags, NULL, N_("limit to heads"),
+ PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, REF_HEADS },
OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL),
OPT_BOOL(0, "get-url", &get_url,
N_("take url.<base>.insteadOf into account")),
--
2.14.2
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [Alt. PATCH] ls-remote: deprecate -h as short for --heads
2017-10-17 15:39 ` [Alt. PATCH] ls-remote: deprecate -h as short for --heads René Scharfe
@ 2017-10-17 17:40 ` Martin Ågren
2017-10-17 17:43 ` Jonathan Nieder
2017-10-17 23:22 ` Junio C Hamano
2 siblings, 0 replies; 23+ messages in thread
From: Martin Ågren @ 2017-10-17 17:40 UTC (permalink / raw)
To: René Scharfe
Cc: Git Mailing List, Thomas Rikl, Thomas Gummerer, Junio C Hamano,
Jonathan Nieder, Jeff King
On 17 October 2017 at 17:39, René Scharfe <l.s.r@web.de> wrote:
> Stop advertising -h as the short equivalent of --heads, because it's
> used for showing a short help text for almost all other git commands.
> Since the ba5f28bf79 (ls-remote: use parse-options api) it has only
> been working when used together with other parameters anyway.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> That would be step on the way towards more consistent command line
> switches, in the same vein as d69155119 (t0012: test "-h" with
> builtins).
FWIW, my first inclination would be to go with a patch like this instead
of the other two (where you introduce an exception so that git ls-remote
-h does not display the usage). ba5f28bf79 was in 2.8.0. That's 18
months ago. Not an eternity, but still some time ago. Not fixing this
regression has an obvious downside, but there's also a downside to
adding the exception.
As long as a lone -h may give the usage or do something else entirely,
then -- put bluntly -- to know whether you can request the usage with
git foo -h without risk of messing up your repo, you'll need to look up
the usage some other way. At which point you've solved your original
problem, without -h.
Of course, we could promise that -h will give the usage or, in case of
historical wart(s), do something harmless. But it seems a bit awkward,
and might limit the perceived usefulness/safeness or -h.
> diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
> index 5f2628c8f8..898836a111 100644
> --- a/Documentation/git-ls-remote.txt
> +++ b/Documentation/git-ls-remote.txt
> @@ -21,7 +21,6 @@ commit IDs.
>
> OPTIONS
> -------
> --h::
> --heads::
> -t::
> --tags::
Do we want to document -h as a deprecated alias, so that people have a
slightly larger chance of noticing and adapting?
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Alt. PATCH] ls-remote: deprecate -h as short for --heads
2017-10-17 15:39 ` [Alt. PATCH] ls-remote: deprecate -h as short for --heads René Scharfe
2017-10-17 17:40 ` Martin Ågren
@ 2017-10-17 17:43 ` Jonathan Nieder
2017-10-17 23:22 ` Junio C Hamano
2 siblings, 0 replies; 23+ messages in thread
From: Jonathan Nieder @ 2017-10-17 17:43 UTC (permalink / raw)
To: René Scharfe
Cc: Git Mailing List, Martin Ågren, Thomas Rikl, Thomas Gummerer,
Junio C Hamano, Jeff King
René Scharfe wrote:
> Stop advertising -h as the short equivalent of --heads, because it's
> used for showing a short help text for almost all other git commands.
> Since the ba5f28bf79 (ls-remote: use parse-options api) it has only
> been working when used together with other parameters anyway.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> Documentation/git-ls-remote.txt | 1 -
> builtin/ls-remote.c | 4 +++-
> 2 files changed, 3 insertions(+), 2 deletions(-)
Yes, I think this is a good step.
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
I would be tempted to go even further and make "git ls-remote -h"
print a warning to stderr to encourage people to update their scripts
to use --heads instead.
Thanks.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Alt. PATCH] ls-remote: deprecate -h as short for --heads
2017-10-17 15:39 ` [Alt. PATCH] ls-remote: deprecate -h as short for --heads René Scharfe
2017-10-17 17:40 ` Martin Ågren
2017-10-17 17:43 ` Jonathan Nieder
@ 2017-10-17 23:22 ` Junio C Hamano
2017-10-19 18:26 ` René Scharfe
2 siblings, 1 reply; 23+ messages in thread
From: Junio C Hamano @ 2017-10-17 23:22 UTC (permalink / raw)
To: René Scharfe
Cc: Git Mailing List, Martin Ågren, Thomas Rikl, Thomas Gummerer,
Jonathan Nieder, Jeff King
René Scharfe <l.s.r@web.de> writes:
> Stop advertising -h as the short equivalent of --heads, because it's
> used for showing a short help text for almost all other git commands.
> Since the ba5f28bf79 (ls-remote: use parse-options api) it has only
> been working when used together with other parameters anyway.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> That would be step on the way towards more consistent command line
> switches, in the same vein as d69155119 (t0012: test "-h" with
> builtins).
Sorry, but I am not sure whom this and the other approach are trying
to help.
The rule we have currently seems to be that "git cmd -h" (no other
arguments) consistently gives a short help, and if a subcommand
supports an option "-h" specific to it that is not about giving a
short help, the caller needs to be aware of it to invoke the option,
by making sure that there is some other arguments on the command
line if "-h" form of that subcommand specific option is used, by
doing e.g. "git ls-remote -h origin" or "git ls-remote --head".
I can see that this "alternative" approach makes it less convenent
for folks who have followed that rule by hiding "-h" (with the
intention of deprecating and possibly removing it in the future) and
encouraging (and later foring) "--head" to be used instead.
The other approach burdens new users by changing the rule to "some
subcommands that have their own '-h' option cannot be asked for a
brief usage with 'git cmd -h'". But the thing is, these new users
who do not know which subcommands do have their own '-h' and which
ones do not are the ones that benefit most from the consistent "'git
cmd -h' with no other argument gets short help" rule.
When making a backward incompatible change, it is asking us to go to
those who are inconvenienced and say "I am sorry that we are making
things less convenient for you, but by doing so we can gain this
greater good which is ...". I can explain how this and the other
approach make things less convenient for existing or new users, but
I am having a hard time formulating what the greater good is.
In short, I cannot sell this change to our users. Please help me do
so if you want this (or the other) change made to our system.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Alt. PATCH] ls-remote: deprecate -h as short for --heads
2017-10-17 23:22 ` Junio C Hamano
@ 2017-10-19 18:26 ` René Scharfe
2017-10-19 20:32 ` Jeff King
0 siblings, 1 reply; 23+ messages in thread
From: René Scharfe @ 2017-10-19 18:26 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git Mailing List, Martin Ågren, Thomas Rikl, Thomas Gummerer,
Jonathan Nieder, Jeff King
Am 18.10.2017 um 01:22 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> Stop advertising -h as the short equivalent of --heads, because it's
>> used for showing a short help text for almost all other git commands.
>> Since the ba5f28bf79 (ls-remote: use parse-options api) it has only
>> been working when used together with other parameters anyway.
>>
>> Signed-off-by: Rene Scharfe <l.s.r@web.de>
>> ---
>> That would be step on the way towards more consistent command line
>> switches, in the same vein as d69155119 (t0012: test "-h" with
>> builtins).
>
> Sorry, but I am not sure whom this and the other approach are trying
> to help.
>
> The rule we have currently seems to be that "git cmd -h" (no other
> arguments) consistently gives a short help, and if a subcommand
> supports an option "-h" specific to it that is not about giving a
> short help, the caller needs to be aware of it to invoke the option,
> by making sure that there is some other arguments on the command
> line if "-h" form of that subcommand specific option is used, by
> doing e.g. "git ls-remote -h origin" or "git ls-remote --head".
>
> I can see that this "alternative" approach makes it less convenent
> for folks who have followed that rule by hiding "-h" (with the
> intention of deprecating and possibly removing it in the future) and
> encouraging (and later foring) "--head" to be used instead.
>
> The other approach burdens new users by changing the rule to "some
> subcommands that have their own '-h' option cannot be asked for a
> brief usage with 'git cmd -h'". But the thing is, these new users
> who do not know which subcommands do have their own '-h' and which
> ones do not are the ones that benefit most from the consistent "'git
> cmd -h' with no other argument gets short help" rule.
They would help by aligning documentation and code, at a price. But
of course there is another way to do that -- I just didn't see it the
other day. We don't have to take anything away:
-- >8 --
Subject: [PATCH] ls-remote: document behavior of lone parameter -h
Reported-by: Thomas Rikl <trikl@online.de>
Analyzed-by: Martin Ågren <martin.agren@gmail.com>
Analyzed-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
Documentation/git-ls-remote.txt | 10 ++++++++++
builtin/ls-remote.c | 5 ++++-
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 5f2628c8f8..82622e7fbc 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -29,6 +29,9 @@ OPTIONS
These options are _not_ mutually exclusive; when given
both, references stored in refs/heads and refs/tags are
displayed.
++
+*NOTE*: -h without any other parameters shows a short help text instead
+of any refs.
--refs::
Do not show peeled tags or pseudorefs like HEAD in the output.
@@ -89,6 +92,13 @@ EXAMPLES
f25a265a342aed6041ab0cc484224d9ca54b6f41 refs/tags/v0.99.1
c5db5456ae3b0873fc659c19fafdde22313cc441 refs/tags/v0.99.2
7ceca275d047c90c0c7d5afb13ab97efdf51bd6e refs/tags/v0.99.3
+ $ git ls-remote -hh
+ From https://git.kernel.org/pub/scm/git/git.git/
+ 4c2224e83951a685185bb8c1f83b28e22fee0e27 refs/heads/maint
+ 5fe978a5381f1fbad26a80e682ddd2a401966740 refs/heads/master
+ 76aedb4517c834be2dc89efb5f9d15908e324422 refs/heads/next
+ c781a84b5204fb294c9ccc79f8b3baceeb32c061 refs/heads/pu
+ 5d38b589ccc7a8355c62f1577865df5b8216c00d refs/heads/todo
GIT
---
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index c4be98ab9e..0438dfec05 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -56,7 +56,10 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
N_("path of git-upload-pack on the remote host"),
PARSE_OPT_HIDDEN },
OPT_BIT('t', "tags", &flags, N_("limit to tags"), REF_TAGS),
- OPT_BIT('h', "heads", &flags, N_("limit to heads"), REF_HEADS),
+ OPT_BIT(0, "heads", &flags, N_("limit to heads"), REF_HEADS),
+ OPT_BIT('h', NULL, &flags,
+ N_("if only parameter: show this help text, otherwise limit to heads"),
+ REF_HEADS),
OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL),
OPT_BOOL(0, "get-url", &get_url,
N_("take url.<base>.insteadOf into account")),
--
2.14.2
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [Alt. PATCH] ls-remote: deprecate -h as short for --heads
2017-10-19 18:26 ` René Scharfe
@ 2017-10-19 20:32 ` Jeff King
2017-10-20 1:04 ` Junio C Hamano
0 siblings, 1 reply; 23+ messages in thread
From: Jeff King @ 2017-10-19 20:32 UTC (permalink / raw)
To: René Scharfe
Cc: Junio C Hamano, Git Mailing List, Martin Ågren, Thomas Rikl,
Thomas Gummerer, Jonathan Nieder
On Thu, Oct 19, 2017 at 08:26:47PM +0200, René Scharfe wrote:
> diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
> index 5f2628c8f8..82622e7fbc 100644
> --- a/Documentation/git-ls-remote.txt
> +++ b/Documentation/git-ls-remote.txt
> @@ -29,6 +29,9 @@ OPTIONS
> These options are _not_ mutually exclusive; when given
> both, references stored in refs/heads and refs/tags are
> displayed.
> ++
> +*NOTE*: -h without any other parameters shows a short help text instead
> +of any refs.
Yuck. This "we only treat -h as special in certain cases" rule is
sufficiently magical that I don't think we want to advertise and lock
ourselves into it.
-Peff
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Alt. PATCH] ls-remote: deprecate -h as short for --heads
2017-10-19 20:32 ` Jeff King
@ 2017-10-20 1:04 ` Junio C Hamano
2017-10-20 3:05 ` Jeff King
0 siblings, 1 reply; 23+ messages in thread
From: Junio C Hamano @ 2017-10-20 1:04 UTC (permalink / raw)
To: Jeff King
Cc: René Scharfe, Git Mailing List, Martin Ågren,
Thomas Rikl, Thomas Gummerer, Jonathan Nieder
Jeff King <peff@peff.net> writes:
> On Thu, Oct 19, 2017 at 08:26:47PM +0200, René Scharfe wrote:
>
>> diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
>> index 5f2628c8f8..82622e7fbc 100644
>> --- a/Documentation/git-ls-remote.txt
>> +++ b/Documentation/git-ls-remote.txt
>> @@ -29,6 +29,9 @@ OPTIONS
>> These options are _not_ mutually exclusive; when given
>> both, references stored in refs/heads and refs/tags are
>> displayed.
>> ++
>> +*NOTE*: -h without any other parameters shows a short help text instead
>> +of any refs.
>
> Yuck. This "we only treat -h as special in certain cases" rule is
> sufficiently magical that I don't think we want to advertise and lock
> ourselves into it.
Hmph. I think it is way too late to be worried about "locked into"
the convention---hasn't the "git cmd -h" been with us forever?
Besides, I personally feel that there is nothing magical in the rule
that is "we always treat 'git $cmd -h' as asking for short help, but
individual subcommand may choose to use -h for, perhaps to be
compatible with other tools and conventions, in other situations".
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Alt. PATCH] ls-remote: deprecate -h as short for --heads
2017-10-20 1:04 ` Junio C Hamano
@ 2017-10-20 3:05 ` Jeff King
2017-10-20 5:35 ` Junio C Hamano
0 siblings, 1 reply; 23+ messages in thread
From: Jeff King @ 2017-10-20 3:05 UTC (permalink / raw)
To: Junio C Hamano
Cc: René Scharfe, Git Mailing List, Martin Ågren,
Thomas Rikl, Thomas Gummerer, Jonathan Nieder
On Fri, Oct 20, 2017 at 10:04:23AM +0900, Junio C Hamano wrote:
> > Yuck. This "we only treat -h as special in certain cases" rule is
> > sufficiently magical that I don't think we want to advertise and lock
> > ourselves into it.
>
> Hmph. I think it is way too late to be worried about "locked into"
> the convention---hasn't the "git cmd -h" been with us forever?
I guess. I still find it pretty nasty (not that "-h" works for help, but
that it can override the normal usage).
> Besides, I personally feel that there is nothing magical in the rule
> that is "we always treat 'git $cmd -h' as asking for short help, but
> individual subcommand may choose to use -h for, perhaps to be
> compatible with other tools and conventions, in other situations".
<shrug> It seems weird and inconsistent to me that the meaning of "-h"
depends on the position and presence of other unrelated options. Maybe
it's just me. I know _why_ it's that way, but this seems like one of
those weird corners of the interface that end up confusing people and
giving Git's interface the reputation of being full of mysterious
inconsistencies. I suspect one of the reasons nobody has complained
about it is that "ls-remote" is not commonly used, and "ls-remote -h"
less so (I didn't even know it was there until this conversation).
-Peff
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Alt. PATCH] ls-remote: deprecate -h as short for --heads
2017-10-20 3:05 ` Jeff King
@ 2017-10-20 5:35 ` Junio C Hamano
2017-10-21 7:16 ` Jeff King
2017-10-21 10:14 ` René Scharfe
0 siblings, 2 replies; 23+ messages in thread
From: Junio C Hamano @ 2017-10-20 5:35 UTC (permalink / raw)
To: Jeff King
Cc: René Scharfe, Git Mailing List, Martin Ågren,
Thomas Rikl, Thomas Gummerer, Jonathan Nieder
Jeff King <peff@peff.net> writes:
> <shrug> It seems weird and inconsistent to me that the meaning of "-h"
> depends on the position and presence of other unrelated options. Maybe
> it's just me. I know _why_ it's that way, but this seems like one of
> those weird corners of the interface that end up confusing people and
> giving Git's interface the reputation of being full of mysterious
> inconsistencies. I suspect one of the reasons nobody has complained
> about it is that "ls-remote" is not commonly used, and "ls-remote -h"
> less so (I didn't even know it was there until this conversation).
Technically, yes, it may be weird.
I may be biased as every time I think about this one, the first one
that comes to my mind is how "grep -h" (not "git grep", but GNU
grep) behaves. Yes, "-h" means something else, but by itself, the
command does not make sense, and "ls-remote -h" is an exception to
the rule: most sane commands do not have a sensible semantics when
they take only "-h" and nothing else. And even for ls-remote it is
true only when you try to be extra lazy and do not say which remote
you are asking.
And that is why I think "'cmd -h" and nothing else consistently
gives help" may overall be positive in usability, than a rule that
says "'cmd -h' is for short-help unless -h means something else for
the command".
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Alt. PATCH] ls-remote: deprecate -h as short for --heads
2017-10-20 5:35 ` Junio C Hamano
@ 2017-10-21 7:16 ` Jeff King
2017-10-21 10:14 ` René Scharfe
1 sibling, 0 replies; 23+ messages in thread
From: Jeff King @ 2017-10-21 7:16 UTC (permalink / raw)
To: Junio C Hamano
Cc: René Scharfe, Git Mailing List, Martin Ågren,
Thomas Rikl, Thomas Gummerer, Jonathan Nieder
On Fri, Oct 20, 2017 at 02:35:36PM +0900, Junio C Hamano wrote:
> I may be biased as every time I think about this one, the first one
> that comes to my mind is how "grep -h" (not "git grep", but GNU
> grep) behaves. Yes, "-h" means something else, but by itself, the
> command does not make sense, and "ls-remote -h" is an exception to
> the rule: most sane commands do not have a sensible semantics when
> they take only "-h" and nothing else. And even for ls-remote it is
> true only when you try to be extra lazy and do not say which remote
> you are asking.
Yeah, I have to admit "grep -h" is a lot more compelling, since it
matches normal grep. And I've used "grep -h" many times without thinking
about it, simply because the rule just falls out naturally there
(there's nothing possible to do without a regex argument, so we'd show
the usage either way).
> And that is why I think "'cmd -h" and nothing else consistently
> gives help" may overall be positive in usability, than a rule that
> says "'cmd -h' is for short-help unless -h means something else for
> the command".
Yeah, I was shooting more for "let's avoid assigning -h to things". But
that's not really possible if we want to be consistent with upstream
grep, which is probably more important.
I think you've convinced me.
-Peff
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Alt. PATCH] ls-remote: deprecate -h as short for --heads
2017-10-20 5:35 ` Junio C Hamano
2017-10-21 7:16 ` Jeff King
@ 2017-10-21 10:14 ` René Scharfe
2017-10-21 12:18 ` Junio C Hamano
1 sibling, 1 reply; 23+ messages in thread
From: René Scharfe @ 2017-10-21 10:14 UTC (permalink / raw)
To: Junio C Hamano, Jeff King
Cc: Git Mailing List, Martin Ågren, Thomas Rikl, Thomas Gummerer,
Jonathan Nieder
Am 20.10.2017 um 07:35 schrieb Junio C Hamano:
> Jeff King <peff@peff.net> writes:
>
>> <shrug> It seems weird and inconsistent to me that the meaning of "-h"
>> depends on the position and presence of other unrelated options.
The position is relevant with parse-options for *each* flag for a
different reason as well: You can't put a flag after a non-flag. I
find that more annoying, as it slightly but noticeable slows down
adding a flag to a previous command by requiring to navigate to the
middle of the line.
(I guess I should train myself to use Meta-b for going back one word
more often..)
> I may be biased as every time I think about this one, the first one
> that comes to my mind is how "grep -h" (not "git grep", but GNU
> grep) behaves. Yes, "-h" means something else, but by itself, the
> command does not make sense, and "ls-remote -h" is an exception to
> the rule: most sane commands do not have a sensible semantics when
> they take only "-h" and nothing else. And even for ls-remote it is
> true only when you try to be extra lazy and do not say which remote
> you are asking.
>
> And that is why I think "'cmd -h" and nothing else consistently
> gives help" may overall be positive in usability, than a rule that
> says "'cmd -h' is for short-help unless -h means something else for
> the command".
FWIW, I use "-?" for that everywhere. I have yet to find a command or
environment where it does something dangerous.
René
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Alt. PATCH] ls-remote: deprecate -h as short for --heads
2017-10-21 10:14 ` René Scharfe
@ 2017-10-21 12:18 ` Junio C Hamano
2017-10-23 15:05 ` René Scharfe
0 siblings, 1 reply; 23+ messages in thread
From: Junio C Hamano @ 2017-10-21 12:18 UTC (permalink / raw)
To: René Scharfe
Cc: Jeff King, Git Mailing List, Martin Ågren, Thomas Rikl,
Thomas Gummerer, Jonathan Nieder
René Scharfe <l.s.r@web.de> writes:
> FWIW, I use "-?" for that everywhere. I have yet to find a command or
> environment where it does something dangerous.
Yeah, it would have made the world a better place if we made that
choice back in 2008. If we start a transition to make it so right
now, we might be able to make the world a better place by 2022,
perhaps. I am not sure if the pain during the transition is worth
it, though.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Alt. PATCH] ls-remote: deprecate -h as short for --heads
2017-10-21 12:18 ` Junio C Hamano
@ 2017-10-23 15:05 ` René Scharfe
2017-10-24 0:52 ` Junio C Hamano
0 siblings, 1 reply; 23+ messages in thread
From: René Scharfe @ 2017-10-23 15:05 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Git Mailing List, Martin Ågren, Thomas Rikl,
Thomas Gummerer, Jonathan Nieder
Am 21.10.2017 um 14:18 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> FWIW, I use "-?" for that everywhere. I have yet to find a command or
>> environment where it does something dangerous.
>
> Yeah, it would have made the world a better place if we made that
> choice back in 2008. If we start a transition to make it so right
> now, we might be able to make the world a better place by 2022,
> perhaps. I am not sure if the pain during the transition is worth
> it, though.
"-?" works fine with builtins already -- they complain that the option
is unknown and then show the short help text.
If we removed the special handling for "-h" from parse-options it would
do the same for most commands, i.e. we'd get an additional error
message, followed by the short help text that we're used to see. We
could check for "git grep -h" without any other arguments and show the
usage text in that case. We could do the same for "git ls-remote -h"
as well, but I'm not sure we want to.
So the cost would be an extra error message in most cases, and possibly
the inability to show help with "git ls-remote -h". That doesn't sound
very painful.
René
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Alt. PATCH] ls-remote: deprecate -h as short for --heads
2017-10-23 15:05 ` René Scharfe
@ 2017-10-24 0:52 ` Junio C Hamano
2017-10-24 18:26 ` René Scharfe
0 siblings, 1 reply; 23+ messages in thread
From: Junio C Hamano @ 2017-10-24 0:52 UTC (permalink / raw)
To: René Scharfe
Cc: Jeff King, Git Mailing List, Martin Ågren, Thomas Rikl,
Thomas Gummerer, Jonathan Nieder
René Scharfe <l.s.r@web.de> writes:
> Am 21.10.2017 um 14:18 schrieb Junio C Hamano:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>> FWIW, I use "-?" for that everywhere. I have yet to find a command or
>>> environment where it does something dangerous.
>>
>> Yeah, it would have made the world a better place if we made that
>> choice back in 2008. If we start a transition to make it so right
>> now, we might be able to make the world a better place by 2022,
>> perhaps. I am not sure if the pain during the transition is worth
>> it, though.
>
> "-?" works fine with builtins already -- they complain that the option
> is unknown and then show the short help text.
Ah, I misunderstood what you meant, then. I thought you were
advocating to move the built-in short-help support to know about and
explicitly react to "-?", and somehow forgot that it "works" more or
less already.
The fact that "-?" already works for most things is good, but the
transition pain still remains, as what's costly is to transition
people's expectation (i.e. "'-?' and not '-h' is the way to get
short help from any subcommand"), not the implementation to fill the
gaps for those that do not yet support '-?', I am afraid.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Alt. PATCH] ls-remote: deprecate -h as short for --heads
2017-10-24 0:52 ` Junio C Hamano
@ 2017-10-24 18:26 ` René Scharfe
0 siblings, 0 replies; 23+ messages in thread
From: René Scharfe @ 2017-10-24 18:26 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Git Mailing List, Martin Ågren, Thomas Rikl,
Thomas Gummerer, Jonathan Nieder
Am 24.10.2017 um 02:52 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> Am 21.10.2017 um 14:18 schrieb Junio C Hamano:
>>> René Scharfe <l.s.r@web.de> writes:
>>>
>>>> FWIW, I use "-?" for that everywhere. I have yet to find a command or
>>>> environment where it does something dangerous.
>>>
>>> Yeah, it would have made the world a better place if we made that
>>> choice back in 2008. If we start a transition to make it so right
>>> now, we might be able to make the world a better place by 2022,
>>> perhaps. I am not sure if the pain during the transition is worth
>>> it, though.
>>
>> "-?" works fine with builtins already -- they complain that the option
>> is unknown and then show the short help text.
>
> Ah, I misunderstood what you meant, then. I thought you were
> advocating to move the built-in short-help support to know about and
> explicitly react to "-?", and somehow forgot that it "works" more or
> less already.
I don't mean to advocate here -- it's more like trying to get the
accounting right. A little bit of OCD perhaps?
> The fact that "-?" already works for most things is good, but the
> transition pain still remains, as what's costly is to transition
> people's expectation (i.e. "'-?' and not '-h' is the way to get
> short help from any subcommand"), not the implementation to fill the
> gaps for those that do not yet support '-?', I am afraid.
A minor cost for help-seeking users would be the extra error message at
the top of the short help text.
The main change would cause "git ls-remote -h" to show remote heads and
"git show-ref -h" to show HEAD. Confused users would have to resort to
e.g. man, -help, --help, their search engine of choice, or -?. I feel
this could be justified. It would be different if e.g. reset started
to take -h as shorthand for --hard, as this would cause data loss.
The hard part would probably be allowing the execution of commands with
unknown arguments outside of repositories to show the help text, even
if they'd normally (with correct arguments) require one. Converting
all commands from RUN_SETUP to RUN_SETUP_GENTLY seems painful. Showing
help when a required repo is not found might be possible somehow.
With that I'm going to shut up, as I don't even use the affected
commands, nor can I imagine being in the place of someone impacted by
"git <something> -h" not showing a help text.
René
^ permalink raw reply [flat|nested] 23+ messages in thread