[PATCH v1 0/4] make "too many arguments" a bit more useful

[PATCH v1 0/4] make "too many arguments" a bit more useful

[Junio C Hamano]

Imagine seeing your command failing with "too many arguments" when
you run "git cmd foo bar baz".  Can you tell it will work if you
said "git cmd foo bar"?  Or is that trimming your command line too
much?  Too little?  You cannot quite tell.

If the command reported "unknown argument: 'bar'", you would know
that among the arguments you gave to "git", i.e. "cmd foo bar baz",
up to "cmd foo" were understood and "bar" and "baz" were not.

This was one of the things I noticed while reviewing a topic and
marked as leftoverbits to be cleaned up after the dust settled.  The
dust has settled and it is a good time to clean them up.

Junio C Hamano (4):
  refs: avoid "too many arguments"
  cat-file: avoid "too many arguments"
  notes: avoid "too many arguments"
  miscellaneous: avoid "too many arguments"

 builtin/cat-file.c      |  3 ++-
 builtin/notes.c         | 18 +++++++++---------
 builtin/prune-packed.c  |  6 +++---
 builtin/receive-pack.c  |  3 ++-
 builtin/refs.c          |  4 +++-
 builtin/tag.c           |  2 +-
 t/t1006-cat-file.sh     |  8 +++++---
 t/t1460-refs-migrate.sh |  7 ++++---
 t/t3301-notes.sh        |  2 +-
 9 files changed, 30 insertions(+), 23 deletions(-)

-- 
2.46.0-235-g968ce1ce0e

[PATCH v1 1/4] refs: avoid "too many arguments"

[Junio C Hamano]

Running "git refs migrate master main" would fail and say "too many
arguments".  By reading that message, you cannot tell if you just
should have given a single ref and made it "git refs migrate
master", or the command refuses to take any arguments.

Let's report that 'master' is unknown in such an input, which would
be easier for the user to understand.

In this particular case, "the command takes no arguments" would also
be a good alternative message, but because we plan to reuse the same
pattern for commands that take 1 or more messages, and saying "the
command takes exactly 1 argument" when "git foo --option bar baz"
has to fail (because it does not want to see "baz") can mislead the
reader into thinking that "--option" may count as that single
argument, so let's be a bit more explicit and mention the first
thing we do not want to see on the command line instead.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/refs.c          | 4 +++-
 t/t1460-refs-migrate.sh | 7 ++++---
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/builtin/refs.c b/builtin/refs.c
index 46dcd150d4..a2aac38ceb 100644
--- a/builtin/refs.c
+++ b/builtin/refs.c
@@ -30,7 +30,9 @@ static int cmd_refs_migrate(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, prefix, options, migrate_usage, 0);
 	if (argc)
-		usage(_("too many arguments"));
+		usage_msg_optf(_("unknown argument: '%s'"),
+			       migrate_usage, options,
+			       argv[0]);
 	if (!format_str)
 		usage(_("missing --ref-format=<format>"));
 
diff --git a/t/t1460-refs-migrate.sh b/t/t1460-refs-migrate.sh
index f7c0783d30..b32e740001 100755
--- a/t/t1460-refs-migrate.sh
+++ b/t/t1460-refs-migrate.sh
@@ -31,9 +31,10 @@ test_expect_success "superfluous arguments" '
 	test_when_finished "rm -rf repo" &&
 	git init repo &&
 	test_must_fail git -C repo refs migrate foo 2>err &&
-	cat >expect <<-EOF &&
-	usage: too many arguments
-	EOF
+	{
+		printf "fatal: unknown argument: ${SQ}foo${SQ}\n\n" &&
+		( git -C repo refs migrate -h || : )
+	} >expect &&
 	test_cmp expect err
 '
 
-- 
2.46.0-235-g968ce1ce0e

[PATCH v1 2/4] cat-file: avoid "too many arguments"

[Junio C Hamano]

Running "git cat-file -e a b c d e f g" would fail and say "too many
arguments".  By reading that message, you cannot tell if the command
could have worked if you limited the list of objects to 5 items
instead of 7, or the command is prepared to take only a single item.

Let's report that "b" is an unexpected argument instead.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/cat-file.c  | 3 ++-
 t/t1006-cat-file.sh | 8 +++++---
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 18fe58d6b8..ba85825c55 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -1074,7 +1074,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 		else if (argc == 1)
 			obj_name = argv[0];
 		else
-			usage_msg_opt(_("too many arguments"), usage, options);
+			usage_msg_optf(_("unknown argument: '%s'"),
+				       usage, options, argv[1]);
 	} else if (!argc) {
 		usage_with_options(usage, options);
 	} else if (argc != 2) {
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index ff9bf213aa..8ad440bbcc 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -66,16 +66,18 @@ do
 	done
 done
 
-test_too_many_arguments () {
+test_unknown_arg () {
+	unk=$1
+	shift
 	test_expect_code 129 "$@" 2>err &&
-	grep -E "^fatal: too many arguments$" err
+	grep -E "^fatal: unknown argument: '$unk'" err
 }
 
 for opt in $short_modes $cw_modes
 do
 	args="one two three"
 	test_expect_success "usage: too many arguments: $opt $args" '
-		test_too_many_arguments git cat-file $opt $args
+		test_unknown_arg two git cat-file $opt $args
 	'
 
 	for opt2 in --buffer --follow-symlinks
-- 
2.46.0-235-g968ce1ce0e

[PATCH v1 3/4] notes: avoid "too many arguments"

[Junio C Hamano]

Imagine seeing your command failing with "too many arguments" when
you run "git cmd foo bar baz".  Can you tell it will work if you
said "git cmd foo bar"?  Or is that trimming your command line too
much?  Too little?

Instead, if the command reports "unknown argument: 'bar'", you'd know
that "bar" and everything after it is unwanted.

Let's make it so for "git notes".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/notes.c  | 18 +++++++++---------
 t/t3301-notes.sh |  2 +-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index d9c356e354..235baeb118 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -447,7 +447,7 @@ static int list(int argc, const char **argv, const char *prefix)
 				     git_notes_list_usage, 0);
 
 	if (1 < argc) {
-		error(_("too many arguments"));
+		error(_("unknown argument: '%s'"), argv[1]);
 		usage_with_options(git_notes_list_usage, options);
 	}
 
@@ -509,7 +509,7 @@ static int add(int argc, const char **argv, const char *prefix)
 			     PARSE_OPT_KEEP_ARGV0);
 
 	if (2 < argc) {
-		error(_("too many arguments"));
+		error(_("unknown argument: '%s'"), argv[2]);
 		usage_with_options(git_notes_add_usage, options);
 	}
 
@@ -591,7 +591,7 @@ static int copy(int argc, const char **argv, const char *prefix)
 
 	if (from_stdin || rewrite_cmd) {
 		if (argc) {
-			error(_("too many arguments"));
+			error(_("unknown argument: '%s'"), argv[0]);
 			usage_with_options(git_notes_copy_usage, options);
 		} else {
 			return notes_copy_from_stdin(force, rewrite_cmd);
@@ -603,7 +603,7 @@ static int copy(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_notes_copy_usage, options);
 	}
 	if (2 < argc) {
-		error(_("too many arguments"));
+		error(_("unknown argument: '%s'"), argv[2]);
 		usage_with_options(git_notes_copy_usage, options);
 	}
 
@@ -686,7 +686,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)
 			     PARSE_OPT_KEEP_ARGV0);
 
 	if (2 < argc) {
-		error(_("too many arguments"));
+		error(_("unknown argument: '%s'"), argv[2]);
 		usage_with_options(usage, options);
 	}
 
@@ -762,7 +762,7 @@ static int show(int argc, const char **argv, const char *prefix)
 			     0);
 
 	if (1 < argc) {
-		error(_("too many arguments"));
+		error(_("unknown argument: '%s'"), argv[1]);
 		usage_with_options(git_notes_show_usage, options);
 	}
 
@@ -915,7 +915,7 @@ static int merge(int argc, const char **argv, const char *prefix)
 		error(_("must specify a notes ref to merge"));
 		usage_with_options(git_notes_merge_usage, options);
 	} else if (!do_merge && argc) {
-		error(_("too many arguments"));
+		error(_("unknown argument: '%s'"), argv[0]);
 		usage_with_options(git_notes_merge_usage, options);
 	}
 
@@ -1069,7 +1069,7 @@ static int prune(int argc, const char **argv, const char *prefix)
 			     0);
 
 	if (argc) {
-		error(_("too many arguments"));
+		error(_("unknown argument: '%s'"), argv[0]);
 		usage_with_options(git_notes_prune_usage, options);
 	}
 
@@ -1091,7 +1091,7 @@ static int get_ref(int argc, const char **argv, const char *prefix)
 			     git_notes_get_ref_usage, 0);
 
 	if (argc) {
-		error(_("too many arguments"));
+		error(_("unknown argument: '%s'"), argv[0]);
 		usage_with_options(git_notes_get_ref_usage, options);
 	}
 
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 536bd11ff4..9db6a2f5c4 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -1472,7 +1472,7 @@ test_expect_success 'git notes copy diagnoses too many or too few arguments' '
 	test_must_fail git notes copy 2>error &&
 	test_grep "too few arguments" error &&
 	test_must_fail git notes copy one two three 2>error &&
-	test_grep "too many arguments" error
+	test_grep "unknown argument: ${SQ}three${SQ}" error
 '
 
 test_expect_success 'git notes get-ref expands refs/heads/main to refs/notes/refs/heads/main' '
-- 
2.46.0-235-g968ce1ce0e

[PATCH v1 4/4] miscellaneous: avoid "too many arguments"

[Junio C Hamano]

Imagine seeing your command failing with "too many arguments" when
you run "git cmd foo bar baz".  Can you tell it will work if you
said "git cmd foo bar"?  Or is that trimming your command line too
much?  Too little?

Instead, if the command reports "unknown argument: 'bar'", you'd know
that "bar" and everything after it is unwanted.

Let's make it so for a few remaining commands.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/prune-packed.c | 6 +++---
 builtin/receive-pack.c | 3 ++-
 builtin/tag.c          | 2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/builtin/prune-packed.c b/builtin/prune-packed.c
index ca3578e158..46989e12f9 100644
--- a/builtin/prune-packed.c
+++ b/builtin/prune-packed.c
@@ -23,9 +23,9 @@ int cmd_prune_packed(int argc, const char **argv, const char *prefix)
 			     prune_packed_usage, 0);
 
 	if (argc > 0)
-		usage_msg_opt(_("too many arguments"),
-			      prune_packed_usage,
-			      prune_packed_options);
+		usage_msg_optf(_("unknown argument: '%s'"),
+			       prune_packed_usage, prune_packed_options,
+			       argv[0]);
 
 	prune_packed_objects(opts);
 	return 0;
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 339524ae2a..e49f4ea4dd 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2503,7 +2503,8 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options, receive_pack_usage, 0);
 
 	if (argc > 1)
-		usage_msg_opt(_("too many arguments"), receive_pack_usage, options);
+		usage_msg_optf(_("unknown argument: '%s'"),
+			       receive_pack_usage, options, argv[0]);
 	if (argc == 0)
 		usage_msg_opt(_("you must specify a directory"), receive_pack_usage, options);
 
diff --git a/builtin/tag.c b/builtin/tag.c
index a1fb218512..274bd0e6ce 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -641,7 +641,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 
 	object_ref = argc == 2 ? argv[1] : "HEAD";
 	if (argc > 2)
-		die(_("too many arguments"));
+		die(_("unknown argument: '%s'"), argv[2]);
 
 	if (repo_get_oid(the_repository, object_ref, &object))
 		die(_("Failed to resolve '%s' as a valid ref."), object_ref);
-- 
2.46.0-235-g968ce1ce0e

Re: [PATCH v1 4/4] miscellaneous: avoid "too many arguments"

[Eric Sunshine]

On Mon, Aug 5, 2024 at 8:36 PM Junio C Hamano <gitster@pobox.com> wrote:
> Imagine seeing your command failing with "too many arguments" when
> you run "git cmd foo bar baz".  Can you tell it will work if you
> said "git cmd foo bar"?  Or is that trimming your command line too
> much?  Too little?
>
> Instead, if the command reports "unknown argument: 'bar'", you'd know
> that "bar" and everything after it is unwanted.
>
> Let's make it so for a few remaining commands.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> @@ -2503,7 +2503,8 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
>         if (argc > 1)
> -               usage_msg_opt(_("too many arguments"), receive_pack_usage, options);
> +               usage_msg_optf(_("unknown argument: '%s'"),
> +                              receive_pack_usage, options, argv[0]);

Is this supposed to be referencing `argv[1]` rather than `argv[0]`...

> diff --git a/builtin/tag.c b/builtin/tag.c
> @@ -641,7 +641,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>         if (argc > 2)
> -               die(_("too many arguments"));
> +               die(_("unknown argument: '%s'"), argv[2]);

...similar to how this references `argv[2]` when the condition is `argc > 2`?

Re: [PATCH v1 1/4] refs: avoid "too many arguments"

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1291 bytes --]

On Mon, Aug 05, 2024 at 05:35:36PM -0700, Junio C Hamano wrote:
> diff --git a/t/t1460-refs-migrate.sh b/t/t1460-refs-migrate.sh
> index f7c0783d30..b32e740001 100755
> --- a/t/t1460-refs-migrate.sh
> +++ b/t/t1460-refs-migrate.sh
> @@ -31,9 +31,10 @@ test_expect_success "superfluous arguments" '
>  	test_when_finished "rm -rf repo" &&
>  	git init repo &&
>  	test_must_fail git -C repo refs migrate foo 2>err &&
> -	cat >expect <<-EOF &&
> -	usage: too many arguments
> -	EOF
> +	{
> +		printf "fatal: unknown argument: ${SQ}foo${SQ}\n\n" &&
> +		( git -C repo refs migrate -h || : )
> +	} >expect &&

I always have to wonder how helpful it really is to print the usage
information in such a context. I feel that it is too distracting because
in many cases, we end up printing dozens of lines of options that drown
out the single line of information that the user actually cares for,
namely why the command has failed.

In this case here it is somewhat manageable, because only 4/5th of the
output are unnecessary noise. But the picture changes as commands grow
more options over time, making the output less and less usable.

So while I think that it is a big improvement to explicitly point out
the unknown argument, I think it is step backwards to also print the
usage info.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 1/4] refs: avoid "too many arguments"

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> -	cat >expect <<-EOF &&
>> -	usage: too many arguments
>> -	EOF
>> +	{
>> +		printf "fatal: unknown argument: ${SQ}foo${SQ}\n\n" &&
>> +		( git -C repo refs migrate -h || : )
>> +	} >expect &&
>
> I always have to wonder how helpful it really is to print the usage
> information in such a context. I feel that it is too distracting because
> in many cases, we end up printing dozens of lines of options that drown
> out the single line of information that the user actually cares for,
> namely why the command has failed.
>
> In this case here it is somewhat manageable, because only 4/5th of the
> output are unnecessary noise. But the picture changes as commands grow
> more options over time, making the output less and less usable.
>
> So while I think that it is a big improvement to explicitly point out
> the unknown argument, I think it is step backwards to also print the
> usage info.

Yeah, I somehow was fooled by the original that called a usage()
function ;-).

"usage:" should signal that the message given is a command line to
show the usage, i.e.

$ git grep -E -e '[^_]usage\("' builtin/\*.c
builtin/merge-index.c:		usage("git merge-index [-o] [-q] <merge-program> (-a | [--] [<filename>...])");
builtin/unpack-file.c:		usage("git unpack-file <blob>");

and is not a signal that the message explains what it found
problematic in this particular usage of the command.  builtin/refs.c
being relatively young do not honor the tradition, it seems.

$ git grep -E -e 'usage\(' builtin/refs.c
builtin/refs.c:		usage(_("too many arguments"));
builtin/refs.c:		usage(_("missing --ref-format=<format>"));

I think die(_("...")) would be a lot more appropriate in these two
places, including the one I touched.

Thanks.

Re: [PATCH v1 4/4] miscellaneous: avoid "too many arguments"

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Aug 5, 2024 at 8:36 PM Junio C Hamano <gitster@pobox.com> wrote:
>> Imagine seeing your command failing with "too many arguments" when
>> you run "git cmd foo bar baz".  Can you tell it will work if you
>> said "git cmd foo bar"?  Or is that trimming your command line too
>> much?  Too little?
>>
>> Instead, if the command reports "unknown argument: 'bar'", you'd know
>> that "bar" and everything after it is unwanted.
>>
>> Let's make it so for a few remaining commands.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
>> @@ -2503,7 +2503,8 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
>>         if (argc > 1)
>> -               usage_msg_opt(_("too many arguments"), receive_pack_usage, options);
>> +               usage_msg_optf(_("unknown argument: '%s'"),
>> +                              receive_pack_usage, options, argv[0]);
>
> Is this supposed to be referencing `argv[1]` rather than `argv[0]`...

You are right.  To the command, the first argument is acceptable,
and the unexpected ones are the second and later ones, so argv[1] is
what it should have said.

>
>> diff --git a/builtin/tag.c b/builtin/tag.c
>> @@ -641,7 +641,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>>         if (argc > 2)
>> -               die(_("too many arguments"));
>> +               die(_("unknown argument: '%s'"), argv[2]);
>
> ...similar to how this references `argv[2]` when the condition is `argc > 2`?

Yes.

[RFC] usage_msg_opt() and _optf() must die

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Patrick Steinhardt <ps@pks.im> writes:
>
>> I always have to wonder how helpful it really is to print the usage
>> information in such a context. I feel that it is too distracting because
>> in many cases, we end up printing dozens of lines of options that drown
>> out the single line of information that the user actually cares for,
>> namely why the command has failed.

Yes.  I do not think I found it useful to give the single-line
message, blank line, followed by the full usage text even a single
time myself.

I am very much tempted to suggest us do this.

 parse-options.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git c/parse-options.c w/parse-options.c
index 30b9e68f8a..f27c425557 100644
--- c/parse-options.c
+++ w/parse-options.c
@@ -1277,11 +1277,10 @@ void NORETURN usage_with_options(const char * const *usagestr,
 }
 
 void NORETURN usage_msg_opt(const char *msg,
-		   const char * const *usagestr,
-		   const struct option *options)
+		   const char * const *usagestr UNUSED,
+		   const struct option *options UNUSED)
 {
-	die_message("%s\n", msg); /* The extra \n is intentional */
-	usage_with_options(usagestr, options);
+	die("%s", msg);
 }
 
 void NORETURN usage_msg_optf(const char * const fmt,

Re: [RFC] usage_msg_opt() and _optf() must die

[Eric Sunshine]

On Tue, Aug 6, 2024 at 1:11 PM Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> > Patrick Steinhardt <ps@pks.im> writes:
> >> I always have to wonder how helpful it really is to print the usage
> >> information in such a context. I feel that it is too distracting because
> >> in many cases, we end up printing dozens of lines of options that drown
> >> out the single line of information that the user actually cares for,
> >> namely why the command has failed.

Thank you, Patrick, for voicing this concern; I nearly did so myself
upon reading this series. I always find it very user-hostile when the
program dumps the entire usage text, thus forcing the user to sift
through a bunch of noise, when it should instead just be printing a
simple explanation of the problem to help the user correct the
invocation. (It's even more frustrating when a program dumps the usage
text but doesn't even bother explaining the problem[*], as if the user
will somehow be able to intuit what went wrong.)

[*]: https://lore.kernel.org/git/CAPig+cSK+wLPUDuGf1d41J_F5jQS+J=a=7kHQLV07-ZKZW9GsA@mail.gmail.com/

> Yes.  I do not think I found it useful to give the single-line
> message, blank line, followed by the full usage text even a single
> time myself.
>
> I am very much tempted to suggest us do this.
>
>  void NORETURN usage_msg_opt(const char *msg,
> -                  const char * const *usagestr,
> -                  const struct option *options)
> +                  const char * const *usagestr UNUSED,
> +                  const struct option *options UNUSED)
>  {
> -       die_message("%s\n", msg); /* The extra \n is intentional */
> -       usage_with_options(usagestr, options);
> +       die("%s", msg);
>  }

As a minimal "fix" to eliminate the user-hostile behavior, I would be
very much in favor of this change.

(Retiring `usage_msg_opt` altogether would be even better, but is much
more invasive.)

[PATCH v2] refs: avoid "too many arguments"

[Junio C Hamano]

Running "git refs migrate master main" would fail and say "too many
arguments".  By reading that message, you cannot tell if you just
should have given a single ref and made it "git refs migrate
master", or the command refuses to take any arguments.

Instead, report that "git ref migrate" takes no arguments, which is
far easier for the user to understand.

    $ git refs migrate master main
    fatal: 'git refs migrate' takes no arguments

The other side of the coin this change is covering is to remove
doubts in new users' minds when we say "git refs migrate", if it is
"git" command running with two "refs migrate" arguments, "git refs"
command running with one "migrate" argument, or "git refs migrate"
command running with no arguments.

In the same spirit, reword the existing "missing --ref-format=<format>"
message and say

    $ git refs migrate
    fatal: 'git refs migrate' needs '--ref-format=<format>'

Note that we are turning two usage() calls to die() calls.  The
former should signal that the message given is a command line that
shows the usage help of the command.  If we are giving a fatal error
message, we should not hesitate to use die().

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * This is no longer a series.  I may resurrect [2,3,4/4] later, but
   they do not share any new helper routines at this moment, so they
   can stay independent patches.  Or perhaps while looking at this
   use case deeper during the review of this patch, we might come up
   with a good way to address this issue for many other scenarios
   and make a set of helper routines, in which case those other
   steps may have to become a part of series again to take advantage
   of them.

 builtin/refs.c          |  4 ++--
 t/t1460-refs-migrate.sh | 10 ++--------
 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/builtin/refs.c b/builtin/refs.c
index 46dcd150d4..a51602f84b 100644
--- a/builtin/refs.c
+++ b/builtin/refs.c
@@ -30,9 +30,9 @@ static int cmd_refs_migrate(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, prefix, options, migrate_usage, 0);
 	if (argc)
-		usage(_("too many arguments"));
+		die(_("'git refs migrate' takes no arguments"));
 	if (!format_str)
-		usage(_("missing --ref-format=<format>"));
+		die(_("'git refs migrate' needs '--ref-format=<format>'"));
 
 	format = ref_storage_format_by_name(format_str);
 	if (format == REF_STORAGE_FORMAT_UNKNOWN) {
diff --git a/t/t1460-refs-migrate.sh b/t/t1460-refs-migrate.sh
index f7c0783d30..e063a98b11 100755
--- a/t/t1460-refs-migrate.sh
+++ b/t/t1460-refs-migrate.sh
@@ -31,20 +31,14 @@ test_expect_success "superfluous arguments" '
 	test_when_finished "rm -rf repo" &&
 	git init repo &&
 	test_must_fail git -C repo refs migrate foo 2>err &&
-	cat >expect <<-EOF &&
-	usage: too many arguments
-	EOF
-	test_cmp expect err
+	test_grep "takes no arguments" err
 '
 
 test_expect_success "missing ref storage format" '
 	test_when_finished "rm -rf repo" &&
 	git init repo &&
 	test_must_fail git -C repo refs migrate 2>err &&
-	cat >expect <<-EOF &&
-	usage: missing --ref-format=<format>
-	EOF
-	test_cmp expect err
+	test_grep "needs ${SQ}--ref-format=<format>${SQ}" err
 '
 
 test_expect_success "unknown ref storage format" '

Interdiff against v1:
  diff --git a/builtin/refs.c b/builtin/refs.c
  index a2aac38ceb..a51602f84b 100644
  --- a/builtin/refs.c
  +++ b/builtin/refs.c
  @@ -30,11 +30,9 @@ static int cmd_refs_migrate(int argc, const char **argv, const char *prefix)
   
   	argc = parse_options(argc, argv, prefix, options, migrate_usage, 0);
   	if (argc)
  -		usage_msg_optf(_("unknown argument: '%s'"),
  -			       migrate_usage, options,
  -			       argv[0]);
  +		die(_("'git refs migrate' takes no arguments"));
   	if (!format_str)
  -		usage(_("missing --ref-format=<format>"));
  +		die(_("'git refs migrate' needs '--ref-format=<format>'"));
   
   	format = ref_storage_format_by_name(format_str);
   	if (format == REF_STORAGE_FORMAT_UNKNOWN) {
  diff --git a/t/t1460-refs-migrate.sh b/t/t1460-refs-migrate.sh
  index b32e740001..e063a98b11 100755
  --- a/t/t1460-refs-migrate.sh
  +++ b/t/t1460-refs-migrate.sh
  @@ -31,21 +31,14 @@ test_expect_success "superfluous arguments" '
   	test_when_finished "rm -rf repo" &&
   	git init repo &&
   	test_must_fail git -C repo refs migrate foo 2>err &&
  -	{
  -		printf "fatal: unknown argument: ${SQ}foo${SQ}\n\n" &&
  -		( git -C repo refs migrate -h || : )
  -	} >expect &&
  -	test_cmp expect err
  +	test_grep "takes no arguments" err
   '
   
   test_expect_success "missing ref storage format" '
   	test_when_finished "rm -rf repo" &&
   	git init repo &&
   	test_must_fail git -C repo refs migrate 2>err &&
  -	cat >expect <<-EOF &&
  -	usage: missing --ref-format=<format>
  -	EOF
  -	test_cmp expect err
  +	test_grep "needs ${SQ}--ref-format=<format>${SQ}" err
   '
   
   test_expect_success "unknown ref storage format" '
-- 
2.46.0-235-g968ce1ce0e

Re: [RFC] usage_msg_opt() and _optf() must die

[Justin Tobler]

On 24/08/06 10:11AM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Patrick Steinhardt <ps@pks.im> writes:
> >
> >> I always have to wonder how helpful it really is to print the usage
> >> information in such a context. I feel that it is too distracting because
> >> in many cases, we end up printing dozens of lines of options that drown
> >> out the single line of information that the user actually cares for,
> >> namely why the command has failed.
> 
> Yes.  I do not think I found it useful to give the single-line
> message, blank line, followed by the full usage text even a single
> time myself.

I tend to also agree. The printed usage information is rather noisy and
makes it more challenging to find the information that is actually
relevant. I would be in favor of supressing the usage information
altogether.

If we did want to provide some sort of breadcrumb for users, maybe we
could print a one-liner explaining how to fetch detailed usage
information for a command if desired? Overall it would still be a lot
less noisy.

-Justin

Re: [RFC] usage_msg_opt() and _optf() must die

[Martin Ågren]

On Tue, 6 Aug 2024 at 19:11, Junio C Hamano <gitster@pobox.com> wrote:
> I am very much tempted to suggest us do this.
>
>  void NORETURN usage_msg_opt(const char *msg,
> -                  const char * const *usagestr,
> -                  const struct option *options)
> +                  const char * const *usagestr UNUSED,
> +                  const struct option *options UNUSED)
>  {
> -       die_message("%s\n", msg); /* The extra \n is intentional */
> -       usage_with_options(usagestr, options);
> +       die("%s", msg);
>  }

Yes, I'm very much in favor of this. I know I've been greeted by a wall
of usage text more than once and it does feel kind of intimidating.

I just tested this using a silly

  git clone a b c d e f

and the user experience is so much better after this patch. It's simply
"fatal: Too many arguments.", which is kind of neat, all things
considered. While one can always polish each error message individually,
hiding it behind a big usage dump feels wrong.

Martin

Re: [RFC] usage_msg_opt() and _optf() must die

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> I am very much tempted to suggest us do this.
>>
>>  void NORETURN usage_msg_opt(const char *msg,
>> -                  const char * const *usagestr,
>> -                  const struct option *options)
>> +                  const char * const *usagestr UNUSED,
>> +                  const struct option *options UNUSED)
>>  {
>> -       die_message("%s\n", msg); /* The extra \n is intentional */
>> -       usage_with_options(usagestr, options);
>> +       die("%s", msg);
>>  }
>
> As a minimal "fix" to eliminate the user-hostile behavior, I would be
> very much in favor of this change.
>
> (Retiring `usage_msg_opt` altogether would be even better, but is much
> more invasive.)

The above is following the usual "we make changes but be nice to in
flight topics by keeping the API function still available, but the
function now behaves better" pattern.  In other words, elimination
of the API function is a breaking change and can go slower.  What
needs more urgent to get to that goal would be to adjust the tests
and documentation pages to the fallout from the above single liner.

Re: [RFC] usage_msg_opt() and _optf() must die

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2169 bytes --]

On Tue, Aug 06, 2024 at 01:21:44PM -0700, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> >> I am very much tempted to suggest us do this.
> >>
> >>  void NORETURN usage_msg_opt(const char *msg,
> >> -                  const char * const *usagestr,
> >> -                  const struct option *options)
> >> +                  const char * const *usagestr UNUSED,
> >> +                  const struct option *options UNUSED)
> >>  {
> >> -       die_message("%s\n", msg); /* The extra \n is intentional */
> >> -       usage_with_options(usagestr, options);
> >> +       die("%s", msg);
> >>  }
> >
> > As a minimal "fix" to eliminate the user-hostile behavior, I would be
> > very much in favor of this change.
> >
> > (Retiring `usage_msg_opt` altogether would be even better, but is much
> > more invasive.)
> 
> The above is following the usual "we make changes but be nice to in
> flight topics by keeping the API function still available, but the
> function now behaves better" pattern.  In other words, elimination
> of the API function is a breaking change and can go slower.  What
> needs more urgent to get to that goal would be to adjust the tests
> and documentation pages to the fallout from the above single liner.

I think it is fine to do the above as an intermediate step towards
dropping `usage_msg_opt()` altogether during this cycle. But what I'd
like to see is that we already convert all existing callers to stop
calling the function such that we can rest assured that we really can
drop the function once the Git v2.48 release cycle starts. Somewhat like
we have handled the deprecation of `struct ref_store`-less functions in
"refs.h".

Otherwise I fear that it's going to stay around indefinitely in a
misleading way.

I'm less sold on swapping the "usage:" prefix out for "error:". I think
that the "usage:" prefix actually gives a helpful signal to the user,
namely that it was the user that passed unexpected arguments. This is in
contrast to "error:", where the command went with what they gave it but
ultimately ended up running into an error condition.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]