{"thread":{"id":"61908","subject":"[PATCH v1 0/4] make \"too many arguments\" a bit more useful","startedAt":"2024-08-06T00:35:43Z","lastAt":"2024-08-07T05:02:01Z","messageCount":16,"participants":["Junio C Hamano","Eric Sunshine","Patrick Steinhardt","Justin Tobler","Martin Ågren"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"500137","messageId":"20240806003539.3292562-1-gitster@pobox.com","threadId":"61908","inReplyTo":null,"subject":"[PATCH v1 0/4] make \"too many arguments\" a bit more useful","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-06T00:35:35Z","receivedAt":"2024-08-06T00:35:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Imagine seeing your command failing with \"too many arguments\" when\nyou run \"git cmd foo bar baz\".  Can you tell it will work if you\nsaid \"git cmd foo bar\"?  Or is that trimming your command line too\nmuch?  Too little?  You cannot quite tell.\n\nIf the command reported \"unknown argument: 'bar'\", you would know\nthat among the arguments you gave to \"git\", i.e. \"cmd foo bar baz\",\nup to \"cmd foo\" were understood and \"bar\" and \"baz\" were not.\n\nThis was one of the things I noticed while reviewing a topic and\nmarked as leftoverbits to be cleaned up after the dust settled.  The\ndust has settled and it is a good time to clean them up.\n\nJunio C Hamano (4):\n  refs: avoid \"too many arguments\"\n  cat-file: avoid \"too many arguments\"\n  notes: avoid \"too many arguments\"\n  miscellaneous: avoid \"too many arguments\"\n\n builtin/cat-file.c      |  3 ++-\n builtin/notes.c         | 18 +++++++++---------\n builtin/prune-packed.c  |  6 +++---\n builtin/receive-pack.c  |  3 ++-\n builtin/refs.c          |  4 +++-\n builtin/tag.c           |  2 +-\n t/t1006-cat-file.sh     |  8 +++++---\n t/t1460-refs-migrate.sh |  7 ++++---\n t/t3301-notes.sh        |  2 +-\n 9 files changed, 30 insertions(+), 23 deletions(-)\n\n-- \n2.46.0-235-g968ce1ce0e\n\n"},{"id":"500138","messageId":"20240806003539.3292562-2-gitster@pobox.com","threadId":"61908","inReplyTo":"20240806003539.3292562-1-gitster@pobox.com","subject":"[PATCH v1 1/4] refs: avoid \"too many arguments\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-06T00:35:36Z","receivedAt":"2024-08-06T00:35:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Running \"git refs migrate master main\" would fail and say \"too many\narguments\".  By reading that message, you cannot tell if you just\nshould have given a single ref and made it \"git refs migrate\nmaster\", or the command refuses to take any arguments.\n\nLet's report that 'master' is unknown in such an input, which would\nbe easier for the user to understand.\n\nIn this particular case, \"the command takes no arguments\" would also\nbe a good alternative message, but because we plan to reuse the same\npattern for commands that take 1 or more messages, and saying \"the\ncommand takes exactly 1 argument\" when \"git foo --option bar baz\"\nhas to fail (because it does not want to see \"baz\") can mislead the\nreader into thinking that \"--option\" may count as that single\nargument, so let's be a bit more explicit and mention the first\nthing we do not want to see on the command line instead.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/refs.c          | 4 +++-\n t/t1460-refs-migrate.sh | 7 ++++---\n 2 files changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/refs.c b/builtin/refs.c\nindex 46dcd150d4..a2aac38ceb 100644\n--- a/builtin/refs.c\n+++ b/builtin/refs.c\n@@ -30,7 +30,9 @@ static int cmd_refs_migrate(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, prefix, options, migrate_usage, 0);\n \tif (argc)\n-\t\tusage(_(\"too many arguments\"));\n+\t\tusage_msg_optf(_(\"unknown argument: '%s'\"),\n+\t\t\t       migrate_usage, options,\n+\t\t\t       argv[0]);\n \tif (!format_str)\n \t\tusage(_(\"missing --ref-format=<format>\"));\n \ndiff --git a/t/t1460-refs-migrate.sh b/t/t1460-refs-migrate.sh\nindex f7c0783d30..b32e740001 100755\n--- a/t/t1460-refs-migrate.sh\n+++ b/t/t1460-refs-migrate.sh\n@@ -31,9 +31,10 @@ test_expect_success \"superfluous arguments\" '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n \ttest_must_fail git -C repo refs migrate foo 2>err &&\n-\tcat >expect <<-EOF &&\n-\tusage: too many arguments\n-\tEOF\n+\t{\n+\t\tprintf \"fatal: unknown argument: ${SQ}foo${SQ}\\n\\n\" &&\n+\t\t( git -C repo refs migrate -h || : )\n+\t} >expect &&\n \ttest_cmp expect err\n '\n \n-- \n2.46.0-235-g968ce1ce0e\n\n"},{"id":"500139","messageId":"20240806003539.3292562-3-gitster@pobox.com","threadId":"61908","inReplyTo":"20240806003539.3292562-1-gitster@pobox.com","subject":"[PATCH v1 2/4] cat-file: avoid \"too many arguments\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-06T00:35:37Z","receivedAt":"2024-08-06T00:35:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Running \"git cat-file -e a b c d e f g\" would fail and say \"too many\narguments\".  By reading that message, you cannot tell if the command\ncould have worked if you limited the list of objects to 5 items\ninstead of 7, or the command is prepared to take only a single item.\n\nLet's report that \"b\" is an unexpected argument instead.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/cat-file.c  | 3 ++-\n t/t1006-cat-file.sh | 8 +++++---\n 2 files changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 18fe58d6b8..ba85825c55 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -1074,7 +1074,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \t\telse if (argc == 1)\n \t\t\tobj_name = argv[0];\n \t\telse\n-\t\t\tusage_msg_opt(_(\"too many arguments\"), usage, options);\n+\t\t\tusage_msg_optf(_(\"unknown argument: '%s'\"),\n+\t\t\t\t       usage, options, argv[1]);\n \t} else if (!argc) {\n \t\tusage_with_options(usage, options);\n \t} else if (argc != 2) {\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex ff9bf213aa..8ad440bbcc 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -66,16 +66,18 @@ do\n \tdone\n done\n \n-test_too_many_arguments () {\n+test_unknown_arg () {\n+\tunk=$1\n+\tshift\n \ttest_expect_code 129 \"$@\" 2>err &&\n-\tgrep -E \"^fatal: too many arguments$\" err\n+\tgrep -E \"^fatal: unknown argument: '$unk'\" err\n }\n \n for opt in $short_modes $cw_modes\n do\n \targs=\"one two three\"\n \ttest_expect_success \"usage: too many arguments: $opt $args\" '\n-\t\ttest_too_many_arguments git cat-file $opt $args\n+\t\ttest_unknown_arg two git cat-file $opt $args\n \t'\n \n \tfor opt2 in --buffer --follow-symlinks\n-- \n2.46.0-235-g968ce1ce0e\n\n"},{"id":"500140","messageId":"20240806003539.3292562-4-gitster@pobox.com","threadId":"61908","inReplyTo":"20240806003539.3292562-1-gitster@pobox.com","subject":"[PATCH v1 3/4] notes: avoid \"too many arguments\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-06T00:35:38Z","receivedAt":"2024-08-06T00:35:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Imagine seeing your command failing with \"too many arguments\" when\nyou run \"git cmd foo bar baz\".  Can you tell it will work if you\nsaid \"git cmd foo bar\"?  Or is that trimming your command line too\nmuch?  Too little?\n\nInstead, if the command reports \"unknown argument: 'bar'\", you'd know\nthat \"bar\" and everything after it is unwanted.\n\nLet's make it so for \"git notes\".\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/notes.c  | 18 +++++++++---------\n t/t3301-notes.sh |  2 +-\n 2 files changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex d9c356e354..235baeb118 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -447,7 +447,7 @@ static int list(int argc, const char **argv, const char *prefix)\n \t\t\t\t     git_notes_list_usage, 0);\n \n \tif (1 < argc) {\n-\t\terror(_(\"too many arguments\"));\n+\t\terror(_(\"unknown argument: '%s'\"), argv[1]);\n \t\tusage_with_options(git_notes_list_usage, options);\n \t}\n \n@@ -509,7 +509,7 @@ static int add(int argc, const char **argv, const char *prefix)\n \t\t\t     PARSE_OPT_KEEP_ARGV0);\n \n \tif (2 < argc) {\n-\t\terror(_(\"too many arguments\"));\n+\t\terror(_(\"unknown argument: '%s'\"), argv[2]);\n \t\tusage_with_options(git_notes_add_usage, options);\n \t}\n \n@@ -591,7 +591,7 @@ static int copy(int argc, const char **argv, const char *prefix)\n \n \tif (from_stdin || rewrite_cmd) {\n \t\tif (argc) {\n-\t\t\terror(_(\"too many arguments\"));\n+\t\t\terror(_(\"unknown argument: '%s'\"), argv[0]);\n \t\t\tusage_with_options(git_notes_copy_usage, options);\n \t\t} else {\n \t\t\treturn notes_copy_from_stdin(force, rewrite_cmd);\n@@ -603,7 +603,7 @@ static int copy(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_notes_copy_usage, options);\n \t}\n \tif (2 < argc) {\n-\t\terror(_(\"too many arguments\"));\n+\t\terror(_(\"unknown argument: '%s'\"), argv[2]);\n \t\tusage_with_options(git_notes_copy_usage, options);\n \t}\n \n@@ -686,7 +686,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\t\t     PARSE_OPT_KEEP_ARGV0);\n \n \tif (2 < argc) {\n-\t\terror(_(\"too many arguments\"));\n+\t\terror(_(\"unknown argument: '%s'\"), argv[2]);\n \t\tusage_with_options(usage, options);\n \t}\n \n@@ -762,7 +762,7 @@ static int show(int argc, const char **argv, const char *prefix)\n \t\t\t     0);\n \n \tif (1 < argc) {\n-\t\terror(_(\"too many arguments\"));\n+\t\terror(_(\"unknown argument: '%s'\"), argv[1]);\n \t\tusage_with_options(git_notes_show_usage, options);\n \t}\n \n@@ -915,7 +915,7 @@ static int merge(int argc, const char **argv, const char *prefix)\n \t\terror(_(\"must specify a notes ref to merge\"));\n \t\tusage_with_options(git_notes_merge_usage, options);\n \t} else if (!do_merge && argc) {\n-\t\terror(_(\"too many arguments\"));\n+\t\terror(_(\"unknown argument: '%s'\"), argv[0]);\n \t\tusage_with_options(git_notes_merge_usage, options);\n \t}\n \n@@ -1069,7 +1069,7 @@ static int prune(int argc, const char **argv, const char *prefix)\n \t\t\t     0);\n \n \tif (argc) {\n-\t\terror(_(\"too many arguments\"));\n+\t\terror(_(\"unknown argument: '%s'\"), argv[0]);\n \t\tusage_with_options(git_notes_prune_usage, options);\n \t}\n \n@@ -1091,7 +1091,7 @@ static int get_ref(int argc, const char **argv, const char *prefix)\n \t\t\t     git_notes_get_ref_usage, 0);\n \n \tif (argc) {\n-\t\terror(_(\"too many arguments\"));\n+\t\terror(_(\"unknown argument: '%s'\"), argv[0]);\n \t\tusage_with_options(git_notes_get_ref_usage, options);\n \t}\n \ndiff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\nindex 536bd11ff4..9db6a2f5c4 100755\n--- a/t/t3301-notes.sh\n+++ b/t/t3301-notes.sh\n@@ -1472,7 +1472,7 @@ test_expect_success 'git notes copy diagnoses too many or too few arguments' '\n \ttest_must_fail git notes copy 2>error &&\n \ttest_grep \"too few arguments\" error &&\n \ttest_must_fail git notes copy one two three 2>error &&\n-\ttest_grep \"too many arguments\" error\n+\ttest_grep \"unknown argument: ${SQ}three${SQ}\" error\n '\n \n test_expect_success 'git notes get-ref expands refs/heads/main to refs/notes/refs/heads/main' '\n-- \n2.46.0-235-g968ce1ce0e\n\n"},{"id":"500141","messageId":"20240806003539.3292562-5-gitster@pobox.com","threadId":"61908","inReplyTo":"20240806003539.3292562-1-gitster@pobox.com","subject":"[PATCH v1 4/4] miscellaneous: avoid \"too many arguments\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-06T00:35:39Z","receivedAt":"2024-08-06T00:36:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Imagine seeing your command failing with \"too many arguments\" when\nyou run \"git cmd foo bar baz\".  Can you tell it will work if you\nsaid \"git cmd foo bar\"?  Or is that trimming your command line too\nmuch?  Too little?\n\nInstead, if the command reports \"unknown argument: 'bar'\", you'd know\nthat \"bar\" and everything after it is unwanted.\n\nLet's make it so for a few remaining commands.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/prune-packed.c | 6 +++---\n builtin/receive-pack.c | 3 ++-\n builtin/tag.c          | 2 +-\n 3 files changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/prune-packed.c b/builtin/prune-packed.c\nindex ca3578e158..46989e12f9 100644\n--- a/builtin/prune-packed.c\n+++ b/builtin/prune-packed.c\n@@ -23,9 +23,9 @@ int cmd_prune_packed(int argc, const char **argv, const char *prefix)\n \t\t\t     prune_packed_usage, 0);\n \n \tif (argc > 0)\n-\t\tusage_msg_opt(_(\"too many arguments\"),\n-\t\t\t      prune_packed_usage,\n-\t\t\t      prune_packed_options);\n+\t\tusage_msg_optf(_(\"unknown argument: '%s'\"),\n+\t\t\t       prune_packed_usage, prune_packed_options,\n+\t\t\t       argv[0]);\n \n \tprune_packed_objects(opts);\n \treturn 0;\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 339524ae2a..e49f4ea4dd 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -2503,7 +2503,8 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options, receive_pack_usage, 0);\n \n \tif (argc > 1)\n-\t\tusage_msg_opt(_(\"too many arguments\"), receive_pack_usage, options);\n+\t\tusage_msg_optf(_(\"unknown argument: '%s'\"),\n+\t\t\t       receive_pack_usage, options, argv[0]);\n \tif (argc == 0)\n \t\tusage_msg_opt(_(\"you must specify a directory\"), receive_pack_usage, options);\n \ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex a1fb218512..274bd0e6ce 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -641,7 +641,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \n \tobject_ref = argc == 2 ? argv[1] : \"HEAD\";\n \tif (argc > 2)\n-\t\tdie(_(\"too many arguments\"));\n+\t\tdie(_(\"unknown argument: '%s'\"), argv[2]);\n \n \tif (repo_get_oid(the_repository, object_ref, &object))\n \t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), object_ref);\n-- \n2.46.0-235-g968ce1ce0e\n\n"},{"id":"500146","messageId":"CAPig+cQ563EVtWEYxZDaQ4awrOcemc4a9a6OSmGN2wVFeMidwA@mail.gmail.com","threadId":"61908","inReplyTo":"20240806003539.3292562-5-gitster@pobox.com","subject":"Re: [PATCH v1 4/4] miscellaneous: avoid \"too many arguments\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-08-06T02:31:37Z","receivedAt":"2024-08-06T02:31:49Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 5, 2024 at 8:36 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Imagine seeing your command failing with \"too many arguments\" when\n> you run \"git cmd foo bar baz\".  Can you tell it will work if you\n> said \"git cmd foo bar\"?  Or is that trimming your command line too\n> much?  Too little?\n>\n> Instead, if the command reports \"unknown argument: 'bar'\", you'd know\n> that \"bar\" and everything after it is unwanted.\n>\n> Let's make it so for a few remaining commands.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> @@ -2503,7 +2503,8 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n>         if (argc > 1)\n> -               usage_msg_opt(_(\"too many arguments\"), receive_pack_usage, options);\n> +               usage_msg_optf(_(\"unknown argument: '%s'\"),\n> +                              receive_pack_usage, options, argv[0]);\n\nIs this supposed to be referencing `argv[1]` rather than `argv[0]`...\n\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> @@ -641,7 +641,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>         if (argc > 2)\n> -               die(_(\"too many arguments\"));\n> +               die(_(\"unknown argument: '%s'\"), argv[2]);\n\n...similar to how this references `argv[2]` when the condition is `argc > 2`?\n"},{"id":"500153","messageId":"ZrG_A3UQk56aYJ_m@tanuki","threadId":"61908","inReplyTo":"20240806003539.3292562-2-gitster@pobox.com","subject":"Re: [PATCH v1 1/4] refs: avoid \"too many arguments\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-06T06:13:23Z","receivedAt":"2024-08-06T06:13:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Aug 05, 2024 at 05:35:36PM -0700, Junio C Hamano wrote:\n> diff --git a/t/t1460-refs-migrate.sh b/t/t1460-refs-migrate.sh\n> index f7c0783d30..b32e740001 100755\n> --- a/t/t1460-refs-migrate.sh\n> +++ b/t/t1460-refs-migrate.sh\n> @@ -31,9 +31,10 @@ test_expect_success \"superfluous arguments\" '\n>  \ttest_when_finished \"rm -rf repo\" &&\n>  \tgit init repo &&\n>  \ttest_must_fail git -C repo refs migrate foo 2>err &&\n> -\tcat >expect <<-EOF &&\n> -\tusage: too many arguments\n> -\tEOF\n> +\t{\n> +\t\tprintf \"fatal: unknown argument: ${SQ}foo${SQ}\\n\\n\" &&\n> +\t\t( git -C repo refs migrate -h || : )\n> +\t} >expect &&\n\nI always have to wonder how helpful it really is to print the usage\ninformation in such a context. I feel that it is too distracting because\nin many cases, we end up printing dozens of lines of options that drown\nout the single line of information that the user actually cares for,\nnamely why the command has failed.\n\nIn this case here it is somewhat manageable, because only 4/5th of the\noutput are unnecessary noise. But the picture changes as commands grow\nmore options over time, making the output less and less usable.\n\nSo while I think that it is a big improvement to explicitly point out\nthe unknown argument, I think it is step backwards to also print the\nusage info.\n\nPatrick\n"},{"id":"500234","messageId":"xmqqed71vbps.fsf@gitster.g","threadId":"61908","inReplyTo":"ZrG_A3UQk56aYJ_m@tanuki","subject":"Re: [PATCH v1 1/4] refs: avoid \"too many arguments\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-06T16:48:47Z","receivedAt":"2024-08-06T16:48:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>> -\tcat >expect <<-EOF &&\n>> -\tusage: too many arguments\n>> -\tEOF\n>> +\t{\n>> +\t\tprintf \"fatal: unknown argument: ${SQ}foo${SQ}\\n\\n\" &&\n>> +\t\t( git -C repo refs migrate -h || : )\n>> +\t} >expect &&\n>\n> I always have to wonder how helpful it really is to print the usage\n> information in such a context. I feel that it is too distracting because\n> in many cases, we end up printing dozens of lines of options that drown\n> out the single line of information that the user actually cares for,\n> namely why the command has failed.\n>\n> In this case here it is somewhat manageable, because only 4/5th of the\n> output are unnecessary noise. But the picture changes as commands grow\n> more options over time, making the output less and less usable.\n>\n> So while I think that it is a big improvement to explicitly point out\n> the unknown argument, I think it is step backwards to also print the\n> usage info.\n\nYeah, I somehow was fooled by the original that called a usage()\nfunction ;-).\n\n\"usage:\" should signal that the message given is a command line to\nshow the usage, i.e.\n\n$ git grep -E -e '[^_]usage\\(\"' builtin/\\*.c\nbuiltin/merge-index.c:\t\tusage(\"git merge-index [-o] [-q] <merge-program> (-a | [--] [<filename>...])\");\nbuiltin/unpack-file.c:\t\tusage(\"git unpack-file <blob>\");\n\nand is not a signal that the message explains what it found\nproblematic in this particular usage of the command.  builtin/refs.c\nbeing relatively young do not honor the tradition, it seems.\n\n$ git grep -E -e 'usage\\(' builtin/refs.c\nbuiltin/refs.c:\t\tusage(_(\"too many arguments\"));\nbuiltin/refs.c:\t\tusage(_(\"missing --ref-format=<format>\"));\n\nI think die(_(\"...\")) would be a lot more appropriate in these two\nplaces, including the one I touched.\n\nThanks.\n\n\n"},{"id":"500235","messageId":"xmqqa5hpvbno.fsf@gitster.g","threadId":"61908","inReplyTo":"CAPig+cQ563EVtWEYxZDaQ4awrOcemc4a9a6OSmGN2wVFeMidwA@mail.gmail.com","subject":"Re: [PATCH v1 4/4] miscellaneous: avoid \"too many arguments\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-06T16:50:03Z","receivedAt":"2024-08-06T16:50:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Mon, Aug 5, 2024 at 8:36 PM Junio C Hamano <gitster@pobox.com> wrote:\n>> Imagine seeing your command failing with \"too many arguments\" when\n>> you run \"git cmd foo bar baz\".  Can you tell it will work if you\n>> said \"git cmd foo bar\"?  Or is that trimming your command line too\n>> much?  Too little?\n>>\n>> Instead, if the command reports \"unknown argument: 'bar'\", you'd know\n>> that \"bar\" and everything after it is unwanted.\n>>\n>> Let's make it so for a few remaining commands.\n>>\n>> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n>> ---\n>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n>> @@ -2503,7 +2503,8 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n>>         if (argc > 1)\n>> -               usage_msg_opt(_(\"too many arguments\"), receive_pack_usage, options);\n>> +               usage_msg_optf(_(\"unknown argument: '%s'\"),\n>> +                              receive_pack_usage, options, argv[0]);\n>\n> Is this supposed to be referencing `argv[1]` rather than `argv[0]`...\n\nYou are right.  To the command, the first argument is acceptable,\nand the unexpected ones are the second and later ones, so argv[1] is\nwhat it should have said.\n\n>\n>> diff --git a/builtin/tag.c b/builtin/tag.c\n>> @@ -641,7 +641,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>>         if (argc > 2)\n>> -               die(_(\"too many arguments\"));\n>> +               die(_(\"unknown argument: '%s'\"), argv[2]);\n>\n> ...similar to how this references `argv[2]` when the condition is `argc > 2`?\n\nYes.\n"},{"id":"500238","messageId":"xmqqmslptw3u.fsf_-_@gitster.g","threadId":"61908","inReplyTo":"xmqqed71vbps.fsf@gitster.g","subject":"[RFC] usage_msg_opt() and _optf() must die","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-06T17:11:17Z","receivedAt":"2024-08-06T17:11:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Patrick Steinhardt <ps@pks.im> writes:\n>\n>> I always have to wonder how helpful it really is to print the usage\n>> information in such a context. I feel that it is too distracting because\n>> in many cases, we end up printing dozens of lines of options that drown\n>> out the single line of information that the user actually cares for,\n>> namely why the command has failed.\n\nYes.  I do not think I found it useful to give the single-line\nmessage, blank line, followed by the full usage text even a single\ntime myself.\n\nI am very much tempted to suggest us do this.\n\n parse-options.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git c/parse-options.c w/parse-options.c\nindex 30b9e68f8a..f27c425557 100644\n--- c/parse-options.c\n+++ w/parse-options.c\n@@ -1277,11 +1277,10 @@ void NORETURN usage_with_options(const char * const *usagestr,\n }\n \n void NORETURN usage_msg_opt(const char *msg,\n-\t\t   const char * const *usagestr,\n-\t\t   const struct option *options)\n+\t\t   const char * const *usagestr UNUSED,\n+\t\t   const struct option *options UNUSED)\n {\n-\tdie_message(\"%s\\n\", msg); /* The extra \\n is intentional */\n-\tusage_with_options(usagestr, options);\n+\tdie(\"%s\", msg);\n }\n \n void NORETURN usage_msg_optf(const char * const fmt,\n"},{"id":"500241","messageId":"CAPig+cSobEaKf3G4OQ7wHYMXTB23mHdwk2sJi5t9+X3ZX_xcLA@mail.gmail.com","threadId":"61908","inReplyTo":"xmqqmslptw3u.fsf_-_@gitster.g","subject":"Re: [RFC] usage_msg_opt() and _optf() must die","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-08-06T17:38:06Z","receivedAt":"2024-08-06T17:38:18Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 6, 2024 at 1:11 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> > Patrick Steinhardt <ps@pks.im> writes:\n> >> I always have to wonder how helpful it really is to print the usage\n> >> information in such a context. I feel that it is too distracting because\n> >> in many cases, we end up printing dozens of lines of options that drown\n> >> out the single line of information that the user actually cares for,\n> >> namely why the command has failed.\n\nThank you, Patrick, for voicing this concern; I nearly did so myself\nupon reading this series. I always find it very user-hostile when the\nprogram dumps the entire usage text, thus forcing the user to sift\nthrough a bunch of noise, when it should instead just be printing a\nsimple explanation of the problem to help the user correct the\ninvocation. (It's even more frustrating when a program dumps the usage\ntext but doesn't even bother explaining the problem[*], as if the user\nwill somehow be able to intuit what went wrong.)\n\n[*]: https://lore.kernel.org/git/CAPig+cSK+wLPUDuGf1d41J_F5jQS+J=a=7kHQLV07-ZKZW9GsA@mail.gmail.com/\n\n> Yes.  I do not think I found it useful to give the single-line\n> message, blank line, followed by the full usage text even a single\n> time myself.\n>\n> I am very much tempted to suggest us do this.\n>\n>  void NORETURN usage_msg_opt(const char *msg,\n> -                  const char * const *usagestr,\n> -                  const struct option *options)\n> +                  const char * const *usagestr UNUSED,\n> +                  const struct option *options UNUSED)\n>  {\n> -       die_message(\"%s\\n\", msg); /* The extra \\n is intentional */\n> -       usage_with_options(usagestr, options);\n> +       die(\"%s\", msg);\n>  }\n\nAs a minimal \"fix\" to eliminate the user-hostile behavior, I would be\nvery much in favor of this change.\n\n(Retiring `usage_msg_opt` altogether would be even better, but is much\nmore invasive.)\n"},{"id":"500242","messageId":"xmqqed71tufv.fsf_-_@gitster.g","threadId":"61908","inReplyTo":"20240806003539.3292562-2-gitster@pobox.com","subject":"[PATCH v2] refs: avoid \"too many arguments\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-06T17:47:16Z","receivedAt":"2024-08-06T17:47:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Running \"git refs migrate master main\" would fail and say \"too many\narguments\".  By reading that message, you cannot tell if you just\nshould have given a single ref and made it \"git refs migrate\nmaster\", or the command refuses to take any arguments.\n\nInstead, report that \"git ref migrate\" takes no arguments, which is\nfar easier for the user to understand.\n\n    $ git refs migrate master main\n    fatal: 'git refs migrate' takes no arguments\n\nThe other side of the coin this change is covering is to remove\ndoubts in new users' minds when we say \"git refs migrate\", if it is\n\"git\" command running with two \"refs migrate\" arguments, \"git refs\"\ncommand running with one \"migrate\" argument, or \"git refs migrate\"\ncommand running with no arguments.\n\nIn the same spirit, reword the existing \"missing --ref-format=<format>\"\nmessage and say\n\n    $ git refs migrate\n    fatal: 'git refs migrate' needs '--ref-format=<format>'\n\nNote that we are turning two usage() calls to die() calls.  The\nformer should signal that the message given is a command line that\nshows the usage help of the command.  If we are giving a fatal error\nmessage, we should not hesitate to use die().\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n * This is no longer a series.  I may resurrect [2,3,4/4] later, but\n   they do not share any new helper routines at this moment, so they\n   can stay independent patches.  Or perhaps while looking at this\n   use case deeper during the review of this patch, we might come up\n   with a good way to address this issue for many other scenarios\n   and make a set of helper routines, in which case those other\n   steps may have to become a part of series again to take advantage\n   of them.\n\n builtin/refs.c          |  4 ++--\n t/t1460-refs-migrate.sh | 10 ++--------\n 2 files changed, 4 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/refs.c b/builtin/refs.c\nindex 46dcd150d4..a51602f84b 100644\n--- a/builtin/refs.c\n+++ b/builtin/refs.c\n@@ -30,9 +30,9 @@ static int cmd_refs_migrate(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, prefix, options, migrate_usage, 0);\n \tif (argc)\n-\t\tusage(_(\"too many arguments\"));\n+\t\tdie(_(\"'git refs migrate' takes no arguments\"));\n \tif (!format_str)\n-\t\tusage(_(\"missing --ref-format=<format>\"));\n+\t\tdie(_(\"'git refs migrate' needs '--ref-format=<format>'\"));\n \n \tformat = ref_storage_format_by_name(format_str);\n \tif (format == REF_STORAGE_FORMAT_UNKNOWN) {\ndiff --git a/t/t1460-refs-migrate.sh b/t/t1460-refs-migrate.sh\nindex f7c0783d30..e063a98b11 100755\n--- a/t/t1460-refs-migrate.sh\n+++ b/t/t1460-refs-migrate.sh\n@@ -31,20 +31,14 @@ test_expect_success \"superfluous arguments\" '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n \ttest_must_fail git -C repo refs migrate foo 2>err &&\n-\tcat >expect <<-EOF &&\n-\tusage: too many arguments\n-\tEOF\n-\ttest_cmp expect err\n+\ttest_grep \"takes no arguments\" err\n '\n \n test_expect_success \"missing ref storage format\" '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n \ttest_must_fail git -C repo refs migrate 2>err &&\n-\tcat >expect <<-EOF &&\n-\tusage: missing --ref-format=<format>\n-\tEOF\n-\ttest_cmp expect err\n+\ttest_grep \"needs ${SQ}--ref-format=<format>${SQ}\" err\n '\n \n test_expect_success \"unknown ref storage format\" '\n\nInterdiff against v1:\n  diff --git a/builtin/refs.c b/builtin/refs.c\n  index a2aac38ceb..a51602f84b 100644\n  --- a/builtin/refs.c\n  +++ b/builtin/refs.c\n  @@ -30,11 +30,9 @@ static int cmd_refs_migrate(int argc, const char **argv, const char *prefix)\n   \n   \targc = parse_options(argc, argv, prefix, options, migrate_usage, 0);\n   \tif (argc)\n  -\t\tusage_msg_optf(_(\"unknown argument: '%s'\"),\n  -\t\t\t       migrate_usage, options,\n  -\t\t\t       argv[0]);\n  +\t\tdie(_(\"'git refs migrate' takes no arguments\"));\n   \tif (!format_str)\n  -\t\tusage(_(\"missing --ref-format=<format>\"));\n  +\t\tdie(_(\"'git refs migrate' needs '--ref-format=<format>'\"));\n   \n   \tformat = ref_storage_format_by_name(format_str);\n   \tif (format == REF_STORAGE_FORMAT_UNKNOWN) {\n  diff --git a/t/t1460-refs-migrate.sh b/t/t1460-refs-migrate.sh\n  index b32e740001..e063a98b11 100755\n  --- a/t/t1460-refs-migrate.sh\n  +++ b/t/t1460-refs-migrate.sh\n  @@ -31,21 +31,14 @@ test_expect_success \"superfluous arguments\" '\n   \ttest_when_finished \"rm -rf repo\" &&\n   \tgit init repo &&\n   \ttest_must_fail git -C repo refs migrate foo 2>err &&\n  -\t{\n  -\t\tprintf \"fatal: unknown argument: ${SQ}foo${SQ}\\n\\n\" &&\n  -\t\t( git -C repo refs migrate -h || : )\n  -\t} >expect &&\n  -\ttest_cmp expect err\n  +\ttest_grep \"takes no arguments\" err\n   '\n   \n   test_expect_success \"missing ref storage format\" '\n   \ttest_when_finished \"rm -rf repo\" &&\n   \tgit init repo &&\n   \ttest_must_fail git -C repo refs migrate 2>err &&\n  -\tcat >expect <<-EOF &&\n  -\tusage: missing --ref-format=<format>\n  -\tEOF\n  -\ttest_cmp expect err\n  +\ttest_grep \"needs ${SQ}--ref-format=<format>${SQ}\" err\n   '\n   \n   test_expect_success \"unknown ref storage format\" '\n-- \n2.46.0-235-g968ce1ce0e\n\n"},{"id":"500247","messageId":"ttxuw2agwpbl3fc3wwo52mmetaa23ihxxfajnffcz3rudsjzai@3hxqx4okstpj","threadId":"61908","inReplyTo":"xmqqmslptw3u.fsf_-_@gitster.g","subject":"Re: [RFC] usage_msg_opt() and _optf() must die","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-08-06T19:09:56Z","receivedAt":"2024-08-06T19:10:45Z","isPatch":false,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/08/06 10:11AM, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Patrick Steinhardt <ps@pks.im> writes:\n> >\n> >> I always have to wonder how helpful it really is to print the usage\n> >> information in such a context. I feel that it is too distracting because\n> >> in many cases, we end up printing dozens of lines of options that drown\n> >> out the single line of information that the user actually cares for,\n> >> namely why the command has failed.\n> \n> Yes.  I do not think I found it useful to give the single-line\n> message, blank line, followed by the full usage text even a single\n> time myself.\n\nI tend to also agree. The printed usage information is rather noisy and\nmakes it more challenging to find the information that is actually\nrelevant. I would be in favor of supressing the usage information\naltogether.\n\nIf we did want to provide some sort of breadcrumb for users, maybe we\ncould print a one-liner explaining how to fetch detailed usage\ninformation for a command if desired? Overall it would still be a lot\nless noisy.\n\n-Justin\n"},{"id":"500248","messageId":"CAN0heSpAVinS8tuUr-o4Js3SymP_aFRpQgTVZYVVtJHVoL-uxw@mail.gmail.com","threadId":"61908","inReplyTo":"xmqqmslptw3u.fsf_-_@gitster.g","subject":"Re: [RFC] usage_msg_opt() and _optf() must die","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2024-08-06T19:24:17Z","receivedAt":"2024-08-06T19:24:30Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Tue, 6 Aug 2024 at 19:11, Junio C Hamano <gitster@pobox.com> wrote:\n> I am very much tempted to suggest us do this.\n>\n>  void NORETURN usage_msg_opt(const char *msg,\n> -                  const char * const *usagestr,\n> -                  const struct option *options)\n> +                  const char * const *usagestr UNUSED,\n> +                  const struct option *options UNUSED)\n>  {\n> -       die_message(\"%s\\n\", msg); /* The extra \\n is intentional */\n> -       usage_with_options(usagestr, options);\n> +       die(\"%s\", msg);\n>  }\n\nYes, I'm very much in favor of this. I know I've been greeted by a wall\nof usage text more than once and it does feel kind of intimidating.\n\nI just tested this using a silly\n\n  git clone a b c d e f\n\nand the user experience is so much better after this patch. It's simply\n\"fatal: Too many arguments.\", which is kind of neat, all things\nconsidered. While one can always polish each error message individually,\nhiding it behind a big usage dump feels wrong.\n\nMartin\n"},{"id":"500251","messageId":"xmqqikwds8pz.fsf@gitster.g","threadId":"61908","inReplyTo":"CAPig+cSobEaKf3G4OQ7wHYMXTB23mHdwk2sJi5t9+X3ZX_xcLA@mail.gmail.com","subject":"Re: [RFC] usage_msg_opt() and _optf() must die","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-06T20:21:44Z","receivedAt":"2024-08-06T20:21:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n>> I am very much tempted to suggest us do this.\n>>\n>>  void NORETURN usage_msg_opt(const char *msg,\n>> -                  const char * const *usagestr,\n>> -                  const struct option *options)\n>> +                  const char * const *usagestr UNUSED,\n>> +                  const struct option *options UNUSED)\n>>  {\n>> -       die_message(\"%s\\n\", msg); /* The extra \\n is intentional */\n>> -       usage_with_options(usagestr, options);\n>> +       die(\"%s\", msg);\n>>  }\n>\n> As a minimal \"fix\" to eliminate the user-hostile behavior, I would be\n> very much in favor of this change.\n>\n> (Retiring `usage_msg_opt` altogether would be even better, but is much\n> more invasive.)\n\nThe above is following the usual \"we make changes but be nice to in\nflight topics by keeping the API function still available, but the\nfunction now behaves better\" pattern.  In other words, elimination\nof the API function is a breaking change and can go slower.  What\nneeds more urgent to get to that goal would be to adjust the tests\nand documentation pages to the fallout from the above single liner.\n"},{"id":"500260","messageId":"ZrL_xI0bNga-RqCv@tanuki","threadId":"61908","inReplyTo":"xmqqikwds8pz.fsf@gitster.g","subject":"Re: [RFC] usage_msg_opt() and _optf() must die","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-07T05:01:56Z","receivedAt":"2024-08-07T05:02:01Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Aug 06, 2024 at 01:21:44PM -0700, Junio C Hamano wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> \n> >> I am very much tempted to suggest us do this.\n> >>\n> >>  void NORETURN usage_msg_opt(const char *msg,\n> >> -                  const char * const *usagestr,\n> >> -                  const struct option *options)\n> >> +                  const char * const *usagestr UNUSED,\n> >> +                  const struct option *options UNUSED)\n> >>  {\n> >> -       die_message(\"%s\\n\", msg); /* The extra \\n is intentional */\n> >> -       usage_with_options(usagestr, options);\n> >> +       die(\"%s\", msg);\n> >>  }\n> >\n> > As a minimal \"fix\" to eliminate the user-hostile behavior, I would be\n> > very much in favor of this change.\n> >\n> > (Retiring `usage_msg_opt` altogether would be even better, but is much\n> > more invasive.)\n> \n> The above is following the usual \"we make changes but be nice to in\n> flight topics by keeping the API function still available, but the\n> function now behaves better\" pattern.  In other words, elimination\n> of the API function is a breaking change and can go slower.  What\n> needs more urgent to get to that goal would be to adjust the tests\n> and documentation pages to the fallout from the above single liner.\n\nI think it is fine to do the above as an intermediate step towards\ndropping `usage_msg_opt()` altogether during this cycle. But what I'd\nlike to see is that we already convert all existing callers to stop\ncalling the function such that we can rest assured that we really can\ndrop the function once the Git v2.48 release cycle starts. Somewhat like\nwe have handled the deprecation of `struct ref_store`-less functions in\n\"refs.h\".\n\nOtherwise I fear that it's going to stay around indefinitely in a\nmisleading way.\n\nI'm less sold on swapping the \"usage:\" prefix out for \"error:\". I think\nthat the \"usage:\" prefix actually gives a helpful signal to the user,\nnamely that it was the user that passed unexpected arguments. This is in\ncontrast to \"error:\", where the command went with what they gave it but\nultimately ended up running into an error condition.\n\nPatrick\n"}]}