Wrong option -h in grep, ls-remote, and show-ref.

Wrong option -h in grep, ls-remote, and show-ref.

[Ignacy Gawedzki]

Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.

What did you do before the bug happened? (Steps to reproduce your issue)

git ls-remote -h

What did you expect to happen? (Expected behavior)

The same as git ls-remote --heads.

What happened instead? (Actual behavior)

Displayed the git ls-remote usage.

What's different between what you expected and what actually happened?

The usage indicates -h is the same as --heads, while -h is handled
upstream and always displays the usage of the command.

Anything else you want to add:

The same problem exists with the following commands:

  grep -h is supposed to not show filenames according to its usage string.
  show-ref -h is defined as some hidden option equivalent to --head.

Please review the rest of the bug report below.
You can delete any lines you don't wish to share.


[System Info]
git version:
git version 2.30.2
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 5.11.0-31-lowlatency #33-Ubuntu SMP PREEMPT Wed Aug 11 14:21:21 UTC 2021 x86_64
compiler info: gnuc: 10.2
libc info: glibc: 2.33
$SHELL (typically, interactive shell): /bin/zsh


[Enabled Hooks]
not run from a git repository - no hooks to show

[PATCH 0/3] show-ref, ls-remote, grep: fix -h handling

[Ævar Arnfjörð Bjarmason]

This fixes the issues reported at
http://lore.kernel.org/git/20210924141920.ezfpyf4uutob2h5z@zenon.in.qult.net,
thanks to Ignacy Gawedzki for the report!

Ævar Arnfjörð Bjarmason (3):
  show-ref: remove unused custom handling of -h
  ls-remote: remove documentation for custom -h option
  grep: pass PARSE_OPT_NO_INTERNAL_HELP, fix odd -h handling

 Documentation/git-ls-remote.txt | 1 -
 builtin/grep.c                  | 3 ++-
 builtin/ls-remote.c             | 2 +-
 builtin/show-ref.c              | 2 --
 t/t0012-help.sh                 | 4 +++-
 t/t7810-grep.sh                 | 4 ++++
 6 files changed, 10 insertions(+), 6 deletions(-)

-- 
2.33.0.1285.g7aff81f6560

[PATCH 1/3] show-ref: remove unused custom handling of -h

[Ævar Arnfjörð Bjarmason]

Since b92891f9783 (parseopt: add PARSE_OPT_NO_INTERNAL_HELP,
2009-03-08) parse_options() has handled "-h" unless told not to, so
when show-ref was migrated to parse_options() in
69932bc6117 (show-ref: migrate to parse-options, 2009-06-20) the
custom "-h" handling that was retained did nothing.

The option was then hidden in e62b3935056 (Show usage string for 'git
show-ref -h', 2009-11-09), but that OPT_BOOLEAN didn't do
anything. Let's just remove this dead code.

Reported-by: Ignacy Gawedzki <ignacy.gawedzki@green-communications.fr>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/show-ref.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 7f8a5332f83..8cefb663282 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -163,8 +163,6 @@ static const struct option show_ref_options[] = {
 	OPT_BOOL(0, "heads", &heads_only, N_("only show heads (can be combined with tags)")),
 	OPT_BOOL(0, "verify", &verify, N_("stricter reference checking, "
 		    "requires exact ref path")),
-	OPT_HIDDEN_BOOL('h', NULL, &show_head,
-			N_("show the HEAD reference, even if it would be filtered out")),
 	OPT_BOOL(0, "head", &show_head,
 	  N_("show the HEAD reference, even if it would be filtered out")),
 	OPT_BOOL('d', "dereference", &deref_tags,
-- 
2.33.0.1285.g7aff81f6560

Re: [PATCH 1/3] show-ref: remove unused custom handling of -h

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Since b92891f9783 (parseopt: add PARSE_OPT_NO_INTERNAL_HELP,
> 2009-03-08) parse_options() has handled "-h" unless told not to, so
> when show-ref was migrated to parse_options() in
> 69932bc6117 (show-ref: migrate to parse-options, 2009-06-20) the
> custom "-h" handling that was retained did nothing.
>
> The option was then hidden in e62b3935056 (Show usage string for 'git
> show-ref -h', 2009-11-09), but that OPT_BOOLEAN didn't do
> anything. Let's just remove this dead code.
>
> Reported-by: Ignacy Gawedzki <ignacy.gawedzki@green-communications.fr>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/show-ref.c | 2 --
>  1 file changed, 2 deletions(-)

While this is a fine no-op, I am not sure this "fixes" complaint
in Ignacy's report.  "git show-ref -h" would (and should) show the
short-help, no?

Re: [PATCH 1/3] show-ref: remove unused custom handling of -h

[René Scharfe]

Am 24.09.21 um 21:24 schrieb Junio C Hamano:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Since b92891f9783 (parseopt: add PARSE_OPT_NO_INTERNAL_HELP,
>> 2009-03-08) parse_options() has handled "-h" unless told not to, so
>> when show-ref was migrated to parse_options() in
>> 69932bc6117 (show-ref: migrate to parse-options, 2009-06-20) the
>> custom "-h" handling that was retained did nothing.
>>
>> The option was then hidden in e62b3935056 (Show usage string for 'git
>> show-ref -h', 2009-11-09), but that OPT_BOOLEAN didn't do
>> anything. Let's just remove this dead code.
>>
>> Reported-by: Ignacy Gawedzki <ignacy.gawedzki@green-communications.fr>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  builtin/show-ref.c | 2 --
>>  1 file changed, 2 deletions(-)
>
> While this is a fine no-op, I am not sure this "fixes" complaint
> in Ignacy's report.  "git show-ref -h" would (and should) show the
> short-help, no?
>

It would, but -h is not a no-op without this patch.  The option is
equivalent to --heads as long as it's not the only argument.  E.g. it
has an effect in "git show-ref -h HEA." or "git show-ref -hh".

René

Re: [PATCH 1/3] show-ref: remove unused custom handling of -h

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Am 24.09.21 um 21:24 schrieb Junio C Hamano:
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>
>>> Since b92891f9783 (parseopt: add PARSE_OPT_NO_INTERNAL_HELP,
>>> 2009-03-08) parse_options() has handled "-h" unless told not to, so
>>> when show-ref was migrated to parse_options() in
>>> 69932bc6117 (show-ref: migrate to parse-options, 2009-06-20) the
>>> custom "-h" handling that was retained did nothing.
>>>
>>> The option was then hidden in e62b3935056 (Show usage string for 'git
>>> show-ref -h', 2009-11-09), but that OPT_BOOLEAN didn't do
>>> anything. Let's just remove this dead code.
>>>
>>> Reported-by: Ignacy Gawedzki <ignacy.gawedzki@green-communications.fr>
>>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>>> ---
>>>  builtin/show-ref.c | 2 --
>>>  1 file changed, 2 deletions(-)
>>
>> While this is a fine no-op, I am not sure this "fixes" complaint
>> in Ignacy's report.  "git show-ref -h" would (and should) show the
>> short-help, no?
>>
>
> It would, but -h is not a no-op without this patch.  The option is
> equivalent to --heads as long as it's not the only argument.  E.g. it
> has an effect in "git show-ref -h HEA." or "git show-ref -hh".

Ah, so this actively breaks the command?  Yeah, thanks---I smelled
something fishy in the change.

[PATCH 2/3] ls-remote: remove documentation for custom -h option

[Ævar Arnfjörð Bjarmason]

The custom handling of the "-h" option was broken in
ba5f28bf79e (ls-remote: use parse-options api, 2016-01-19), first
released with Git v2.8.0. We've been promising that it's a synonym of
--head, but it's not.

We could make this work again by supplying the
PARSE_OPT_NO_INTERNAL_HELP flag to parse_options(), but if we were
writing this command today we wouldn't make this an exception. Since
it's been such a long time let's just remove this rather than
restoring the exception to "-h" handling.

Reported-by: Ignacy Gawedzki <ignacy.gawedzki@green-communications.fr>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-ls-remote.txt | 1 -
 builtin/ls-remote.c             | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 492e573856f..6e241640bd9 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 f4fd823af83..c5e68918b78 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -64,7 +64,7 @@ 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(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.33.0.1285.g7aff81f6560

Re: [PATCH 2/3] ls-remote: remove documentation for custom -h option

[SZEDER Gábor]

On Fri, Sep 24, 2021 at 06:51:45PM +0200, Ævar Arnfjörð Bjarmason wrote:
> The custom handling of the "-h" option was broken in
> ba5f28bf79e (ls-remote: use parse-options api, 2016-01-19), first
> released with Git v2.8.0. We've been promising that it's a synonym of
> --head, but it's not.
> 
> We could make this work again by supplying the
> PARSE_OPT_NO_INTERNAL_HELP flag to parse_options(), but if we were
> writing this command today we wouldn't make this an exception. Since
> it's been such a long time let's just remove this rather than
> restoring the exception to "-h" handling.

This breaks the case when '-h' is used in combination with a remote:

  $ git ls-remote -h origin
  225bc32a989d7a22fa6addafd4ce7dcd04675dbf	refs/heads/maint
  ddb1055343948e0d0bc81f8d20245f1ada6430a0	refs/heads/master
  4c38ced6901a8523cea197b31b2616240ec9fb6e	refs/heads/next
  ee03ddbf0ea6a78ad9a229bd029408bbff85232e	refs/heads/seen
  687d33056ee28fd03567f3725150e3fcd0582979	refs/heads/todo

> Reported-by: Ignacy Gawedzki <ignacy.gawedzki@green-communications.fr>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  Documentation/git-ls-remote.txt | 1 -
>  builtin/ls-remote.c             | 2 +-
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
> index 492e573856f..6e241640bd9 100644
> --- a/Documentation/git-ls-remote.txt
> +++ b/Documentation/git-ls-remote.txt
> @@ -21,7 +21,6 @@ commit IDs.
>  
>  OPTIONS
>  -------
> --h::

The description of this option contains the following:

  Note that git ls-remote -h used without anything else on the command
  line gives help, consistent with other git subcommands.

>  --heads::
>  -t::
>  --tags::
> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
> index f4fd823af83..c5e68918b78 100644
> --- a/builtin/ls-remote.c
> +++ b/builtin/ls-remote.c
> @@ -64,7 +64,7 @@ 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(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.33.0.1285.g7aff81f6560
> 

Re: [PATCH 2/3] ls-remote: remove documentation for custom -h option

[Jeff King]

On Fri, Sep 24, 2021 at 07:11:13PM +0200, SZEDER Gábor wrote:

> > diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
> > index 492e573856f..6e241640bd9 100644
> > --- a/Documentation/git-ls-remote.txt
> > +++ b/Documentation/git-ls-remote.txt
> > @@ -21,7 +21,6 @@ commit IDs.
> >  
> >  OPTIONS
> >  -------
> > --h::
> 
> The description of this option contains the following:
> 
>   Note that git ls-remote -h used without anything else on the command
>   line gives help, consistent with other git subcommands.

Yup. That comes from this thread discussing an identical patch:

  https://lore.kernel.org/git/b0397b3285eab3448a3fd5dd2c50abb9@pascalroeleven.nl/

which in turn references more discussion of this behavior:

  https://lore.kernel.org/git/xmqqk1ztmkbn.fsf@gitster.mtv.corp.google.com/

It's gross and weird, but kept for backwards compatibility.

-Peff

Re: [PATCH 2/3] ls-remote: remove documentation for custom -h option

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Fri, Sep 24, 2021 at 06:51:45PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> The custom handling of the "-h" option was broken in
>> ba5f28bf79e (ls-remote: use parse-options api, 2016-01-19), first
>> released with Git v2.8.0. We've been promising that it's a synonym of
>> --head, but it's not.
>> 
>> We could make this work again by supplying the
>> PARSE_OPT_NO_INTERNAL_HELP flag to parse_options(), but if we were
>> writing this command today we wouldn't make this an exception. Since
>> it's been such a long time let's just remove this rather than
>> restoring the exception to "-h" handling.
>
> This breaks the case when '-h' is used in combination with a remote:

Yes, I think I covered this in my response to Ignacy.  Perhaps Ævar
can read it before rerolling this series.

Thanks.

[PATCH 3/3] grep: pass PARSE_OPT_NO_INTERNAL_HELP, fix odd -h handling

[Ævar Arnfjörð Bjarmason]

The "grep" command supports both "-h" and "-H" options, along with a
mandatory pattern, but this has been partially usurped by the "-h"
handling in parse_options().

The reason it's just been "odd" instead of a bug is that we'll only
print out "-h" usage with parse_options() if there's no further
non-option arguments, so instead of printing this brief blurb on a
stand-alone -h we'd print out the full usage:

    $ git grep -H
    fatal: no pattern given

But for the aforementioned reason a "git grep -h <pattern>" would
work, we wouldn't take the !PARSE_OPT_NO_INTERNAL_HELP branch in
parse_options_step(), would handle our own custom 'h' option, and
builtin/grep.c itself would know what to do at that point.

Reported-by: Ignacy Gawedzki <ignacy.gawedzki@green-communications.fr>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c  | 3 ++-
 t/t0012-help.sh | 4 +++-
 t/t7810-grep.sh | 4 ++++
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 51278b01fa2..a2d6704aa41 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -981,7 +981,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	 */
 	argc = parse_options(argc, argv, prefix, options, grep_usage,
 			     PARSE_OPT_KEEP_DASHDASH |
-			     PARSE_OPT_STOP_AT_NON_OPTION);
+			     PARSE_OPT_STOP_AT_NON_OPTION |
+			     PARSE_OPT_NO_INTERNAL_HELP);
 	grep_commit_pattern_type(pattern_type_arg, &opt);
 
 	if (use_index && !startup_info->have_repository) {
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 913f34c8e9d..0b623fe794a 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -90,7 +90,9 @@ test_expect_success 'git help succeeds without git.html' '
 '
 
 test_expect_success 'generate builtin list' '
-	git --list-cmds=builtins >builtins
+	git --list-cmds=builtins >builtins &&
+	grep -v "^grep$" builtins >builtins+ &&
+	mv builtins+ builtins
 '
 
 while read builtin
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 6b6423a07c3..4c2777120b0 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -11,6 +11,10 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
 . ./test-lib.sh
 
+test_expect_success 'usage' '
+	test_expect_code 128 git grep -h
+'
+
 test_invalid_grep_expression() {
 	params="$@" &&
 	test_expect_success "invalid expression: grep $params" '
-- 
2.33.0.1285.g7aff81f6560

Re: Wrong option -h in grep, ls-remote, and show-ref.

[Junio C Hamano]

Ignacy Gawedzki <ignacy.gawedzki@green-communications.fr> writes:

> What did you do before the bug happened? (Steps to reproduce your issue)
>
> git ls-remote -h
>
> What did you expect to happen? (Expected behavior)
>
> The same as git ls-remote --heads.
>
> What happened instead? (Actual behavior)
>
> Displayed the git ls-remote usage.

Thanks for a report, but this is very much working as intended.

There may be some subcommands that assign their own meaning to "-h"
for historical reasons (like "ls-remote -h origin"), or for external
reasons (like "grep -h -e pattern"), but most newbies expect a short
help out of "-h" uniformly across subcommands.

Fortunately, "-h" alone would not make any sense for "grep" (you
need a pattern) and you do not need to use "-h" for "ls-remote" [*].
We prioritized to help newbies by consistently giving a short help
across subcommands, over letting "git grep -h" to complain "you need
to give me a pattern", like so:

    $ git ls-remote --heads
    fatal: No remote configured to list refs from.

    $ git grep
    fatal: no pattern given

    $ git grep -h
    usage: git grep [<options>] [-e] <pattern> [<rev>...] [[--] <path>...]

    --cached              search in index instead of in the work tree
    ...

    $ git ls-remote -h
    usage: git ls-remote [--heads] [--tags] [--refs] [--upload-pack=<exec>]
                         [-q | --quiet] [--exit-code] [--get-url]
    ...

HTH.


[Footnote]

* It is not end-user facing Porcelain, but is meant for scripting,
  and you can afford to write "--heads".  Besides, "-h" acts as
  "--heads" in "git ls-remote -h origin" or "git ls-remote -h -q"
  just fine.  Only the "-h and nothing else is given" case is
  sacrificed to help newbies in the case of this subcommand.