# [Outreachy PATCH] git.c: drop NO_PARSEOPT from builtins that use parse-options 18 messages from 2026-10-24 to 2026-11-05. Participants: Tanga Joris, René Scharfe, Junio C Hamano, Kaartic Sivaraam, Cynthia Uche. Thread: https://gitlist.dev/t/66649 ## Tanga Joris, 2026-10-24 05:24 Subject: [Outreachy PATCH] git.c: drop NO_PARSEOPT from builtins that use parse-options Message-ID: <20261024052449.95869-1-tangajoris@gmail.com> URL: https://gitlist.dev/e/20261024052449.95869-1-tangajoris%40gmail.com ``` The NO_PARSEOPT flag in the commands[] table marks builtins that parse their options by hand. Builtins marked with it are left out of "git --list-cmds=parseopt", which bash completion uses to decide whether it can ask a command for its options with "--git-completion-helper". Four builtins have since been converted to parse-options, but are still marked NO_PARSEOPT: - patch-id, in 2871f4d447 (builtin: patch-id: add --verbatim as a command mode, 2022-10-24) - diff-pairs, which has used parse-options since it was introduced in 5bd10b2adc (builtin: introduce diff-pairs command, 2025-02-28) - fast-import, in 863937696e (fast-import: use parse_options() for command line options, 2026-08-11) - var, in e19f0779c6 (var: add "-z" output mode, 2026-09-26) This causes "git fast-import --", "git patch-id --" and "git diff-pairs --" to offer no options, even though they are listed by "--git-completion-helper". Remove the flag from these four builtins, like 66fa6e8ed8 (git.c: update NO_PARSEOPT markings, 2022-08-19) did for others. Signed-off-by: Tanga Joris --- Removing the flag from var does not change its completion, since it only has short options: -l and -z. rev-parse also calls parse_options(), but unlike the four above, parse_options() is called only in its --parseopt mode. Its normal options are parsed by hand, so it keeps the flag. git.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/git.c b/git.c index d7b5f88e52..2d27ad4175 100644 --- a/git.c +++ b/git.c @@ -571,11 +571,11 @@ static struct cmd_struct commands[] = { { "diff", cmd_diff, NO_PARSEOPT }, { "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT }, { "diff-index", cmd_diff_index, RUN_SETUP | NO_PARSEOPT }, - { "diff-pairs", cmd_diff_pairs, RUN_SETUP | NO_PARSEOPT }, + { "diff-pairs", cmd_diff_pairs, RUN_SETUP }, { "diff-tree", cmd_diff_tree, RUN_SETUP | NO_PARSEOPT }, { "difftool", cmd_difftool, RUN_SETUP_GENTLY }, { "fast-export", cmd_fast_export, RUN_SETUP }, - { "fast-import", cmd_fast_import, RUN_SETUP | NO_PARSEOPT }, + { "fast-import", cmd_fast_import, RUN_SETUP }, { "fetch", cmd_fetch, RUN_SETUP }, { "fetch-pack", cmd_fetch_pack, RUN_SETUP | NO_PARSEOPT }, { "fmt-merge-msg", cmd_fmt_merge_msg, RUN_SETUP }, @@ -626,7 +626,7 @@ static struct cmd_struct commands[] = { { "pack-redundant", cmd_pack_redundant, RUN_SETUP | NO_PARSEOPT | DEPRECATED }, #endif { "pack-refs", cmd_pack_refs, RUN_SETUP }, - { "patch-id", cmd_patch_id, RUN_SETUP_GENTLY | NO_PARSEOPT }, + { "patch-id", cmd_patch_id, RUN_SETUP_GENTLY }, { "pickaxe", cmd_blame, RUN_SETUP }, { "prune", cmd_prune, RUN_SETUP }, { "prune-packed", cmd_prune_packed, RUN_SETUP }, @@ -676,7 +676,7 @@ static struct cmd_struct commands[] = { { "upload-archive--writer", cmd_upload_archive_writer, NO_PARSEOPT }, { "upload-pack", cmd_upload_pack }, { "url-parse", cmd_url_parse }, - { "var", cmd_var, RUN_SETUP_GENTLY | NO_PARSEOPT }, + { "var", cmd_var, RUN_SETUP_GENTLY }, { "verify-commit", cmd_verify_commit, RUN_SETUP }, { "verify-pack", cmd_verify_pack }, { "verify-tag", cmd_verify_tag, RUN_SETUP }, -- 2.43.0 ``` ## René Scharfe, 2026-10-25 07:59 Subject: Re: [Outreachy PATCH] git.c: drop NO_PARSEOPT from builtins that use parse-options Message-ID: <8b60c7f6-4a1c-495a-a594-aaf322279539@web.de> URL: https://gitlist.dev/e/8b60c7f6-4a1c-495a-a594-aaf322279539%40web.de In-Reply-To: <20261024052449.95869-1-tangajoris@gmail.com> ``` On 10/24/26 7:24 AM, Tanga Joris wrote: > The NO_PARSEOPT flag in the commands[] table marks builtins that > parse their options by hand. Builtins marked with it are left out > of "git --list-cmds=parseopt", which bash completion uses to > decide whether it can ask a command for its options with > "--git-completion-helper". > > Four builtins have since been converted to parse-options, but are > still marked NO_PARSEOPT: > > - patch-id, in 2871f4d447 (builtin: patch-id: add --verbatim as a > command mode, 2022-10-24) > - diff-pairs, which has used parse-options since it was introduced > in 5bd10b2adc (builtin: introduce diff-pairs command, 2025-02-28) > - fast-import, in 863937696e (fast-import: use parse_options() for > command line options, 2026-08-11) > - var, in e19f0779c6 (var: add "-z" output mode, 2026-09-26) > > This causes "git fast-import --", "git patch-id --" and > "git diff-pairs --" to offer no options, even though they are > listed by "--git-completion-helper". > > Remove the flag from these four builtins, like 66fa6e8ed8 (git.c: > update NO_PARSEOPT markings, 2022-08-19) did for others. Good idea! "git fast-import --git-completion-helper" prints its command line options only after it gets fed a line of input. That's odd. Do the completion scripts still work seamlessly, i.e. without requiring the user to press ? René ``` ## Tanga Joris, 2026-10-26 02:59 Subject: Re: [Outreachy PATCH] git.c: drop NO_PARSEOPT from builtins that use parse-options Message-ID: URL: https://gitlist.dev/e/CAKrRMNqtqYiE2YKVky%2BmHyrkqtW2c_L_DcCen1hyaQa8D0%2BfWw%40mail.gmail.com In-Reply-To: <8b60c7f6-4a1c-495a-a594-aaf322279539@web.de> ``` On Sun, 25 Oct 2026 at 10:59, René Scharfe wrote: > "git fast-import --git-completion-helper" prints its command line > options only after it gets fed a line of input. That's odd. Do the > completion scripts still work seamlessly, i.e. without requiring the > user to press ? Thanks for catching this. They don't. __git() in the completion script does not redirect stdin, so the helper reads from the terminal and "git fast-import --" waits until the user presses or ^D. This seems to happen because fast-import only calls parse_options() once it sees the first command in the stream (or EOF), as it reads the "feature" lines first. Since patch-id, diff-pairs and var return immediately, I'll send a v2 that still removes the NO_PARSEOPT flag from them, but keeps it on fast-import, for now, as I make a separate patch fixing the wait behaviour, and ultimately removing the flag. The separate patch would teach fast-import to answer --git-completion-helper before reading its input, like it does for "-h". Christian, I plan to base it on top of your early-scan series, since it touches the same code. Would you be fine with that? Thanks, Tanga ``` ## Junio C Hamano, 2026-10-26 04:58 Subject: Re: [Outreachy PATCH] git.c: drop NO_PARSEOPT from builtins that use parse-options Message-ID: URL: https://gitlist.dev/e/xmqq33ttvz0q.fsf%40gitster.g In-Reply-To: ``` Tanga Joris writes: > Christian, I plan to base it on top of your early-scan series, since > it touches the same code. Would you be fine with that? It is not something Christian can approve, nor do you need his permission. You need to make sure that the topic you plan to depend on is as solid as possible. Those who write topics that depend on other topics usually make sure the topics they depend on are at least in 'next'. You know how you can help make Christian's 'cc/early-scan-options' topic as solid as possible and help it get merged to 'next' sooner, right? In this community, topics do not move forward without positive reviews. As a potential user of the service the topic offers, you are in a better position than random bystanders to carefully review it, find issues, and offer improvements. Thanks. ``` ## Tanga Joris, 2026-10-29 01:26 Subject: [Outreachy PATCH v2 1/2] fast-import: handle --git-completion-helper before reading input Message-ID: <20261029012616.139783-1-tangajoris@gmail.com> URL: https://gitlist.dev/e/20261029012616.139783-1-tangajoris%40gmail.com In-Reply-To: <20261024052449.95869-1-tangajoris@gmail.com> ``` fast-import parses its options only after reading the "feature" lines from its input, and it creates a temporary pack file before that. So "git fast-import --git-completion-helper" waits for input, and each run leaves a tmp_pack_* file behind. bash completion runs the helper with the terminal as input, so "git fast-import --" would freeze once fast-import is no longer marked with the NO_PARSEOPT flag. Handle a lone --git-completion-helper or --git-completion-helper-all right after the -h check, before creating the pack or reading input. Reported-by: René Scharfe Signed-off-by: Tanga Joris --- Changes since v1: - New patch 1/2, addressing René's observation that completion would block on fast-import - 2/2 is unchanged builtin/fast-import.c | 4 ++++ t/t9300-fast-import.sh | 8 ++++++++ 2 files changed, 12 insertions(+) diff --git a/builtin/fast-import.c b/builtin/fast-import.c index 9db3f83eea..f53113d5a5 100644 --- a/builtin/fast-import.c +++ b/builtin/fast-import.c @@ -4204,6 +4204,10 @@ int cmd_fast_import(int argc, }; show_usage_with_options_if_asked(argc, argv, fast_import_usage, fast_import_options); + if (argc == 2 && (!strcmp(argv[1], "--git-completion-helper") || + !strcmp(argv[1], "--git-completion-helper-all"))) + parse_options(argc, argv, prefix, fast_import_options, + fast_import_usage, 0); fast_import_state_init(&state, argc, argv, prefix, fast_import_options); diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh index 73c6e3d448..99d7843217 100755 --- a/t/t9300-fast-import.sh +++ b/t/t9300-fast-import.sh @@ -2914,6 +2914,14 @@ test_expect_success 'R: blob appears only once' ' test 1 = $n ' +test_expect_success 'R: --git-completion-helper does not read from stdin' ' + find .git/objects/pack -name "tmp_pack_*" >before && + echo garbage | git fast-import --git-completion-helper >out && + test_grep -e "--date-format=" out && + find .git/objects/pack -name "tmp_pack_*" >after && + test_cmp before after +' + ### ### series S (mark and path parsing) ### -- 2.56.0.291.g0b48760bf4 ``` ## Tanga Joris, 2026-10-29 01:26 Subject: [Outreachy PATCH v2 2/2] git.c: drop NO_PARSEOPT from builtins that use parse-options Message-ID: <20261029012616.139783-2-tangajoris@gmail.com> URL: https://gitlist.dev/e/20261029012616.139783-2-tangajoris%40gmail.com In-Reply-To: <20261029012616.139783-1-tangajoris@gmail.com> ``` The NO_PARSEOPT flag in the commands[] table marks builtins that parse their options by hand. Builtins marked with it are left out of "git --list-cmds=parseopt", which bash completion uses to decide whether it can ask a command for its options with "--git-completion-helper". Four builtins have since been converted to parse-options, but are still marked NO_PARSEOPT: - patch-id, in 2871f4d447 (builtin: patch-id: add --verbatim as a command mode, 2022-10-24) - diff-pairs, which has used parse-options since it was introduced in 5bd10b2adc (builtin: introduce diff-pairs command, 2025-02-28) - fast-import, in 863937696e (fast-import: use parse_options() for command line options, 2026-08-11) - var, in e19f0779c6 (var: add "-z" output mode, 2026-09-26) This causes "git fast-import --", "git patch-id --" and "git diff-pairs --" to offer no options, even though they are listed by "--git-completion-helper". Remove the flag from these four builtins, like 66fa6e8ed8 (git.c: update NO_PARSEOPT markings, 2022-08-19) did for others. Signed-off-by: Tanga Joris --- git.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/git.c b/git.c index d7b5f88e52..2d27ad4175 100644 --- a/git.c +++ b/git.c @@ -571,11 +571,11 @@ static struct cmd_struct commands[] = { { "diff", cmd_diff, NO_PARSEOPT }, { "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT }, { "diff-index", cmd_diff_index, RUN_SETUP | NO_PARSEOPT }, - { "diff-pairs", cmd_diff_pairs, RUN_SETUP | NO_PARSEOPT }, + { "diff-pairs", cmd_diff_pairs, RUN_SETUP }, { "diff-tree", cmd_diff_tree, RUN_SETUP | NO_PARSEOPT }, { "difftool", cmd_difftool, RUN_SETUP_GENTLY }, { "fast-export", cmd_fast_export, RUN_SETUP }, - { "fast-import", cmd_fast_import, RUN_SETUP | NO_PARSEOPT }, + { "fast-import", cmd_fast_import, RUN_SETUP }, { "fetch", cmd_fetch, RUN_SETUP }, { "fetch-pack", cmd_fetch_pack, RUN_SETUP | NO_PARSEOPT }, { "fmt-merge-msg", cmd_fmt_merge_msg, RUN_SETUP }, @@ -626,7 +626,7 @@ static struct cmd_struct commands[] = { { "pack-redundant", cmd_pack_redundant, RUN_SETUP | NO_PARSEOPT | DEPRECATED }, #endif { "pack-refs", cmd_pack_refs, RUN_SETUP }, - { "patch-id", cmd_patch_id, RUN_SETUP_GENTLY | NO_PARSEOPT }, + { "patch-id", cmd_patch_id, RUN_SETUP_GENTLY }, { "pickaxe", cmd_blame, RUN_SETUP }, { "prune", cmd_prune, RUN_SETUP }, { "prune-packed", cmd_prune_packed, RUN_SETUP }, @@ -676,7 +676,7 @@ static struct cmd_struct commands[] = { { "upload-archive--writer", cmd_upload_archive_writer, NO_PARSEOPT }, { "upload-pack", cmd_upload_pack }, { "url-parse", cmd_url_parse }, - { "var", cmd_var, RUN_SETUP_GENTLY | NO_PARSEOPT }, + { "var", cmd_var, RUN_SETUP_GENTLY }, { "verify-commit", cmd_verify_commit, RUN_SETUP }, { "verify-pack", cmd_verify_pack }, { "verify-tag", cmd_verify_tag, RUN_SETUP }, -- 2.56.0.291.g0b48760bf4 ``` ## Junio C Hamano, 2026-10-29 18:04 Subject: Re: [Outreachy PATCH v2 1/2] fast-import: handle --git-completion-helper before reading input Message-ID: URL: https://gitlist.dev/e/xmqqy0bg2xj5.fsf%40gitster.g In-Reply-To: <20261029012616.139783-1-tangajoris@gmail.com> ``` Tanga Joris writes: > diff --git a/builtin/fast-import.c b/builtin/fast-import.c > index 9db3f83eea..f53113d5a5 100644 > --- a/builtin/fast-import.c > +++ b/builtin/fast-import.c > @@ -4204,6 +4204,10 @@ int cmd_fast_import(int argc, > }; > > show_usage_with_options_if_asked(argc, argv, fast_import_usage, fast_import_options); > + if (argc == 2 && (!strcmp(argv[1], "--git-completion-helper") || > + !strcmp(argv[1], "--git-completion-helper-all"))) > + parse_options(argc, argv, prefix, fast_import_options, > + fast_import_usage, 0); > > fast_import_state_init(&state, argc, argv, prefix, fast_import_options); It is way too ugly to target this so specifically. I wonder if we can teach show_usage_with_options_if_asked() to do this? With this, and the above reverted, the new test added to t9300 in this patch seems to pass. parse-options.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git c/parse-options.c w/parse-options.c index 283a0f1dc2..f2a23b79a4 100644 --- c/parse-options.c +++ w/parse-options.c @@ -1536,6 +1536,12 @@ void show_usage_with_options_if_asked(int ac, const char **av, usage_with_options_internal(NULL, usagestr, opts, USAGE_FULL, USAGE_TO_STDOUT); exit(0); + } else if (!strcmp(av[1], "--git-completion-helper")) { + show_gitcomp(opts, 0); + exit(0); + } else if (!strcmp(av[1], "--git-completion-helper-all")) { + show_gitcomp(opts, 1); + exit(0); } } } ``` ## Tanga Joris, 2026-10-29 23:16 Subject: [Outreachy PATCH v3 1/2] parse-options: answer --git-completion-helper before command runs Message-ID: <20261029231621.48958-1-tangajoris@gmail.com> URL: https://gitlist.dev/e/20261029231621.48958-1-tangajoris%40gmail.com In-Reply-To: <20261029012616.139783-1-tangajoris@gmail.com> ``` fast-import parses its options only after reading the "feature" lines from its input, and it creates a temporary pack file before that. So "git fast-import --git-completion-helper" waits for input, and each run leaves a tmp_pack_* file behind. bash completion runs the helper with the terminal as input, so "git fast-import --" would freeze once fast-import is no longer marked with the NO_PARSEOPT flag. Since show_usage_with_options_if_asked() already answers -h and --help-all before a command does any work, it now also answers the completion helper, so fast-import and other commands that call it reply without reading input. Reported-by: René Scharfe Helped-by: Junio C Hamano Signed-off-by: Tanga Joris --- Changes since v2: - Moved the check from fast-import into show_usage_with_options_if_asked() in parse-options, as Junio suggested. - 2/2 is unchanged parse-options.c | 6 ++++++ t/t9300-fast-import.sh | 8 ++++++++ 2 files changed, 14 insertions(+) diff --git a/parse-options.c b/parse-options.c index 283a0f1dc2..f2a23b79a4 100644 --- a/parse-options.c +++ b/parse-options.c @@ -1536,6 +1536,12 @@ void show_usage_with_options_if_asked(int ac, const char **av, usage_with_options_internal(NULL, usagestr, opts, USAGE_FULL, USAGE_TO_STDOUT); exit(0); + } else if (!strcmp(av[1], "--git-completion-helper")) { + show_gitcomp(opts, 0); + exit(0); + } else if (!strcmp(av[1], "--git-completion-helper-all")) { + show_gitcomp(opts, 1); + exit(0); } } } diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh index 73c6e3d448..99d7843217 100755 --- a/t/t9300-fast-import.sh +++ b/t/t9300-fast-import.sh @@ -2914,6 +2914,14 @@ test_expect_success 'R: blob appears only once' ' test 1 = $n ' +test_expect_success 'R: --git-completion-helper does not read from stdin' ' + find .git/objects/pack -name "tmp_pack_*" >before && + echo garbage | git fast-import --git-completion-helper >out && + test_grep -e "--date-format=" out && + find .git/objects/pack -name "tmp_pack_*" >after && + test_cmp before after +' + ### ### series S (mark and path parsing) ### -- 2.43.0 ``` ## Tanga Joris, 2026-10-29 23:16 Subject: [Outreachy PATCH v3 2/2] git.c: drop NO_PARSEOPT from builtins that use parse-options Message-ID: <20261029231621.48958-2-tangajoris@gmail.com> URL: https://gitlist.dev/e/20261029231621.48958-2-tangajoris%40gmail.com In-Reply-To: <20261029231621.48958-1-tangajoris@gmail.com> ``` The NO_PARSEOPT flag in the commands[] table marks builtins that parse their options by hand. Builtins marked with it are left out of "git --list-cmds=parseopt", which bash completion uses to decide whether it can ask a command for its options with "--git-completion-helper". Four builtins have since been converted to parse-options, but are still marked NO_PARSEOPT: - patch-id, in 2871f4d447 (builtin: patch-id: add --verbatim as a command mode, 2022-10-24) - diff-pairs, which has used parse-options since it was introduced in 5bd10b2adc (builtin: introduce diff-pairs command, 2025-02-28) - fast-import, in 863937696e (fast-import: use parse_options() for command line options, 2026-08-11) - var, in e19f0779c6 (var: add "-z" output mode, 2026-09-26) This causes "git fast-import --", "git patch-id --" and "git diff-pairs --" to offer no options, even though they are listed by "--git-completion-helper". Remove the flag from these four builtins, like 66fa6e8ed8 (git.c: update NO_PARSEOPT markings, 2022-08-19) did for others. Signed-off-by: Tanga Joris --- git.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/git.c b/git.c index d7b5f88e52..2d27ad4175 100644 --- a/git.c +++ b/git.c @@ -571,11 +571,11 @@ static struct cmd_struct commands[] = { { "diff", cmd_diff, NO_PARSEOPT }, { "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT }, { "diff-index", cmd_diff_index, RUN_SETUP | NO_PARSEOPT }, - { "diff-pairs", cmd_diff_pairs, RUN_SETUP | NO_PARSEOPT }, + { "diff-pairs", cmd_diff_pairs, RUN_SETUP }, { "diff-tree", cmd_diff_tree, RUN_SETUP | NO_PARSEOPT }, { "difftool", cmd_difftool, RUN_SETUP_GENTLY }, { "fast-export", cmd_fast_export, RUN_SETUP }, - { "fast-import", cmd_fast_import, RUN_SETUP | NO_PARSEOPT }, + { "fast-import", cmd_fast_import, RUN_SETUP }, { "fetch", cmd_fetch, RUN_SETUP }, { "fetch-pack", cmd_fetch_pack, RUN_SETUP | NO_PARSEOPT }, { "fmt-merge-msg", cmd_fmt_merge_msg, RUN_SETUP }, @@ -626,7 +626,7 @@ static struct cmd_struct commands[] = { { "pack-redundant", cmd_pack_redundant, RUN_SETUP | NO_PARSEOPT | DEPRECATED }, #endif { "pack-refs", cmd_pack_refs, RUN_SETUP }, - { "patch-id", cmd_patch_id, RUN_SETUP_GENTLY | NO_PARSEOPT }, + { "patch-id", cmd_patch_id, RUN_SETUP_GENTLY }, { "pickaxe", cmd_blame, RUN_SETUP }, { "prune", cmd_prune, RUN_SETUP }, { "prune-packed", cmd_prune_packed, RUN_SETUP }, @@ -676,7 +676,7 @@ static struct cmd_struct commands[] = { { "upload-archive--writer", cmd_upload_archive_writer, NO_PARSEOPT }, { "upload-pack", cmd_upload_pack }, { "url-parse", cmd_url_parse }, - { "var", cmd_var, RUN_SETUP_GENTLY | NO_PARSEOPT }, + { "var", cmd_var, RUN_SETUP_GENTLY }, { "verify-commit", cmd_verify_commit, RUN_SETUP }, { "verify-pack", cmd_verify_pack }, { "verify-tag", cmd_verify_tag, RUN_SETUP }, -- 2.43.0 ``` ## Kaartic Sivaraam, 2026-10-30 09:44 Subject: Re: [Outreachy PATCH v3 1/2] parse-options: answer --git-completion-helper before command runs Message-ID: <123de4e4-3387-4eef-8a05-9ed1ad1fce30@gmail.com> URL: https://gitlist.dev/e/123de4e4-3387-4eef-8a05-9ed1ad1fce30%40gmail.com In-Reply-To: <20261029231621.48958-1-tangajoris@gmail.com> ``` Hi Tanga Joris, Just a gentle reminder about this portion of the MyFirstContribution[1]: It’s good manners to reply to each comment in the mailing list discussion instead of letting the next version of your patch be your only response. Tell the reviewer whether you plan to make the suggested change, keep the original, or pursue a different approach. This way reviewers can respond to your reasoning before you spend time preparing a version they may not agree with, and later do not need to inspect your v2 to figure out whether you implemented their comment or not. On 10/30/26 04:46, Tanga Joris wrote: > fast-import parses its options only after reading the "feature" > lines from its input, and it creates a temporary pack file before > that. So "git fast-import --git-completion-helper" waits for input, > and each run leaves a tmp_pack_* file behind. > > bash completion runs the helper with the terminal as input, so > "git fast-import --" would freeze once fast-import is no > longer marked with the NO_PARSEOPT flag. > May be give a hint that you plan to do this in the following commit? That might give someone without context (for example someone reading the commit after a while) a hint about why you are worrying about marking fast-import with the NO_PARSEOPT flag. > Since show_usage_with_options_if_asked() already answers -h and > --help-all before a command does any work, it now also answers > the completion helper, so fast-import and other commands that > call it reply without reading input. > > Reported-by: René Scharfe > Helped-by: Junio C Hamano > Signed-off-by: Tanga Joris > --- > Changes since v2: > - Moved the check from fast-import into > show_usage_with_options_if_asked() in parse-options, > as Junio suggested. > - 2/2 is unchanged > > parse-options.c | 6 ++++++ > t/t9300-fast-import.sh | 8 ++++++++ > 2 files changed, 14 insertions(+) > > diff --git a/parse-options.c b/parse-options.c > index 283a0f1dc2..f2a23b79a4 100644 > --- a/parse-options.c > +++ b/parse-options.c > @@ -1536,6 +1536,12 @@ void show_usage_with_options_if_asked(int ac, const char **av, > usage_with_options_internal(NULL, usagestr, opts, > USAGE_FULL, USAGE_TO_STDOUT); > exit(0); > + } else if (!strcmp(av[1], "--git-completion-helper")) { > + show_gitcomp(opts, 0); > + exit(0); > + } else if (!strcmp(av[1], "--git-completion-helper-all")) { > + show_gitcomp(opts, 1); > + exit(0); > } > } > } The change seems to be fine but I wonder if it is worth a note in parse-options.h about why show_usage_with_options_if_asked() is also handling the '--git-completion-helper' and '--git-completion-helper-all' arguments? [ References ] [1]: https://git-scm.com/docs/MyFirstContribution#reviewing -- Sivaraam ``` ## Kaartic Sivaraam, 2026-10-30 10:21 Subject: Re: [Outreachy PATCH v3 2/2] git.c: drop NO_PARSEOPT from builtins that use parse-options Message-ID: URL: https://gitlist.dev/e/d026d6a0-5b1b-4bbf-bd1f-5f41c9c2a4e4%40gmail.com In-Reply-To: <20261029231621.48958-2-tangajoris@gmail.com> ``` On 10/30/26 04:46, Tanga Joris wrote: > diff --git a/git.c b/git.c > index d7b5f88e52..2d27ad4175 100644 > --- a/git.c > +++ b/git.c > @@ -571,11 +571,11 @@ static struct cmd_struct commands[] = { > { "diff", cmd_diff, NO_PARSEOPT }, > { "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT }, > { "diff-index", cmd_diff_index, RUN_SETUP | NO_PARSEOPT }, > - { "diff-pairs", cmd_diff_pairs, RUN_SETUP | NO_PARSEOPT }, > + { "diff-pairs", cmd_diff_pairs, RUN_SETUP }, > { "diff-tree", cmd_diff_tree, RUN_SETUP | NO_PARSEOPT }, > { "difftool", cmd_difftool, RUN_SETUP_GENTLY }, > { "fast-export", cmd_fast_export, RUN_SETUP }, > - { "fast-import", cmd_fast_import, RUN_SETUP | NO_PARSEOPT }, > + { "fast-import", cmd_fast_import, RUN_SETUP }, > { "fetch", cmd_fetch, RUN_SETUP }, > { "fetch-pack", cmd_fetch_pack, RUN_SETUP | NO_PARSEOPT }, > { "fmt-merge-msg", cmd_fmt_merge_msg, RUN_SETUP }, > @@ -626,7 +626,7 @@ static struct cmd_struct commands[] = { > { "pack-redundant", cmd_pack_redundant, RUN_SETUP | NO_PARSEOPT | DEPRECATED }, > #endif > { "pack-refs", cmd_pack_refs, RUN_SETUP }, > - { "patch-id", cmd_patch_id, RUN_SETUP_GENTLY | NO_PARSEOPT }, > + { "patch-id", cmd_patch_id, RUN_SETUP_GENTLY }, > { "pickaxe", cmd_blame, RUN_SETUP }, > { "prune", cmd_prune, RUN_SETUP }, > { "prune-packed", cmd_prune_packed, RUN_SETUP }, > @@ -676,7 +676,7 @@ static struct cmd_struct commands[] = { > { "upload-archive--writer", cmd_upload_archive_writer, NO_PARSEOPT }, > { "upload-pack", cmd_upload_pack }, > { "url-parse", cmd_url_parse }, > - { "var", cmd_var, RUN_SETUP_GENTLY | NO_PARSEOPT }, > + { "var", cmd_var, RUN_SETUP_GENTLY }, > { "verify-commit", cmd_verify_commit, RUN_SETUP }, > { "verify-pack", cmd_verify_pack }, > { "verify-tag", cmd_verify_tag, RUN_SETUP }, This looks good to me. I was wondering if it would be valuable to also add a test so that any future commands that start using parse_options() could be caught if the corresponding change does not also remove NO_PARSEOPT from the command? For instance, something like so: ' diff --git a/t/t0012-help.sh b/t/t0012-help.sh index b0a3fdbbbc..cb147f30ef 100755 --- a/t/t0012-help.sh +++ b/t/t0012-help.sh @@ -267,4 +267,29 @@ do ' done parseopt && + sort builtins >sorted-builtins && + sort parseopt >sorted-parseopt && + false && + comm -23 sorted-builtins sorted-parseopt >no-parseopt +' + +while read builtin +do + case "$builtin" in + # these parse their own options by hand, but pass the diff + # options through parse-options, which answers the helper + diff|diff-files|diff-index|diff-tree|rev-list) + continue ;; + esac + + test_expect_success "$builtin is not using parse-options" ' + test_might_fail git $builtin --git-completion-helper \ + output 2>/dev/null && + # rev-parse echoes unknown arguments back + ! grep -v "^--git-completion-helper\$" output | grep "^--" + ' +done URL: https://gitlist.dev/e/CAKrRMNoAALZ0V5fdKjZPQnCXuMP8y49vfUmxYnRt7UEeUTE7kA%40mail.gmail.com In-Reply-To: <123de4e4-3387-4eef-8a05-9ed1ad1fce30@gmail.com> ``` Hello Kaartic, Thank you so much for taking the time to review this. On Fri, 30 Oct 2026 at 12:44, Kaartic Sivaraam wrote: > Just a gentle reminder about this portion of the MyFirstContribution[1]: > > It’s good manners to reply to each comment in the mailing list > discussion instead of letting the next version of your patch be your > only response. Tell the reviewer whether you plan to make the > suggested change, keep the original, or pursue a different approach. Thanks for the reminder about replying to comments. I'll do that from now on. > May be give a hint that you plan to do this in the following commit? > That might give someone without context (for example someone reading the > commit after a while) a hint about why you are worrying about marking > fast-import with the NO_PARSEOPT flag. Alright, I'll add a sentence to the commit message saying that the next commit removes the NO_PARSEOPT flag from fast-import, which would make "git fast-import --" freeze without this patch. > The change seems to be fine but I wonder if it is worth a note in > parse-options.h about why show_usage_with_options_if_asked() is also > handling the '--git-completion-helper' and '--git-completion-helper-all' > arguments? Yes, I think that would help, so I will add a short comment above its declaration in parse-options.h explaining that it answers these before the command does any work. Thanks, Tanga ``` ## Tanga Joris, 2026-11-02 02:26 Subject: Re: [Outreachy PATCH v3 2/2] git.c: drop NO_PARSEOPT from builtins that use parse-options Message-ID: URL: https://gitlist.dev/e/CAKrRMNogw6rUsZzDsEPLmLAmjB6fvcH4njga%2B_qBz554x8oEwg%40mail.gmail.com In-Reply-To: ``` Hello Kaartic, Thanks for reviewing my patch 2/2, and for writing the test sketch. On Fri, 30 Oct 2026 at 13:21, Kaartic Sivaraam wrote: > This looks good to me. I was wondering if it would be valuable to also > add a test so that any future commands that start using parse_options() > could be caught if the corresponding change does not also remove > NO_PARSEOPT from the command? I also think the test is a good idea. It would definitely catch future commands that switch to parse-options but forget to remove NO_PARSEOPT. I tried your sketch on top of this series. Without 2/2, it fails for diff-pairs, fast-import and patch-id, so it catches the bug this patch fixes. With patch 2/2, it passes. I had to remove the 'false &&' line to make it run, which I think was left over from testing. It does not catch var, though, because var only has short options (like -l), and the test looks for lines starting with '--'. Currently, I am not sure how to make the test catch commands with only short options, because the helper only lists those with long options. Do you have an idea on how we could do this? Would you like the test inside 2/2, or as a separate patch 3/3? Thanks, Tanga ``` ## Cynthia Uche, 2026-11-03 09:25 Subject: Re: [Outreachy PATCH v3 1/2] parse-options: answer --git-completion-helper before command runs Message-ID: <20261103092503.14872-1-herroyalpriestess@gmail.com> URL: https://gitlist.dev/e/20261103092503.14872-1-herroyalpriestess%40gmail.com In-Reply-To: <20261029231621.48958-1-tangajoris@gmail.com> ``` Hi Tanga, I applied v3 1/2 and 2/2 on top of master (ebdc087dc9) and built with NO_RUST=1. t9300, t0012 and t9902 pass. On master, "git --list-cmds=parseopt" lists none of diff-pairs, fast-import, patch-id and var; with the series it lists all four, and the first three print their options with --git-completion-helper. On Fri, Oct 30, 2026 at 12:17 AM Tanga Joris wrote: > + } else if (!strcmp(av[1], "--git-completion-helper-all")) { > + show_gitcomp(opts, 1); > + exit(0); Since this affects every caller of show_usage_with_options_if_asked(), I ran "git --git-completion-helper" and the "-all" form with all && test_grep -e "--allow-unsafe-features" all ' Cynthia ``` ## Tanga Joris, 2026-11-04 22:34 Subject: Re: [Outreachy PATCH v3 1/2] parse-options: answer --git-completion-helper before command runs Message-ID: URL: https://gitlist.dev/e/CAKrRMNpaF%3DNQOi0mmr_1jE0TQynbQRzhEtdSj2RLfon-UHMLgw%40mail.gmail.com In-Reply-To: <20261103092503.14872-1-herroyalpriestess@gmail.com> ``` Hey Cynthia, Thank you for testing this. On Tue, 3 Nov 2026 at 12:25, Cynthia Uche wrote: > The new t9300 test fails without the parse-options.c change, so it > guards the fix, but it only covers the plain form: with the "-all" > branch changed to show_gitcomp(opts, 0) it still passes. Could you > add a test for that branch? Yes, I will add the test in my v4. I have tried it locally myself and it passes with my fix and fails when the -all branch is broken. Thanks again, I will credit you with Helped-by:. Tanga ``` ## Tanga Joris, 2026-11-05 00:39 Subject: [Outreachy PATCH v4 1/3] parse-options: answer --git-completion-helper before command runs Message-ID: <20261105003914.708836-1-tangajoris@gmail.com> URL: https://gitlist.dev/e/20261105003914.708836-1-tangajoris%40gmail.com In-Reply-To: <20261029231621.48958-1-tangajoris@gmail.com> ``` fast-import parses its options only after reading the "feature" lines from its input, and it creates a temporary pack file before that. So "git fast-import --git-completion-helper" waits for input, and each run leaves a tmp_pack_* file behind. bash completion runs the helper with the terminal as input. The next commit removes the NO_PARSEOPT flag from fast-import, which would make "git fast-import --" freeze without this patch. show_usage_with_options_if_asked() already answers -h and --help-all before a command does any work. Teach it to answer the completion helper too. Then fast-import, and the other commands that call it, reply without reading input. Reported-by: René Scharfe Helped-by: Junio C Hamano Helped-by: Kaartic Sivaraam Helped-by: Cynthia Uche Signed-off-by: Tanga Joris --- Changes since v3: - Added a sentence to the log message saying that the next commit removes NO_PARSEOPT from fast-import, as Kaartic suggested. - Added a comment above show_usage_with_options_if_asked() in parse-options.h, as Kaartic suggested. - Added Cynthia's test for --git-completion-helper-all. I also added the tmp_pack_* check from the first test to it, because without it the test still passes when the fix is reverted. - 2/3 is unchanged. - 3/3 is new, it adds the test Kaartic suggested. parse-options.c | 6 ++++++ parse-options.h | 6 ++++++ t/t9300-fast-import.sh | 16 ++++++++++++++++ 3 files changed, 28 insertions(+) diff --git a/parse-options.c b/parse-options.c index 283a0f1dc2..f2a23b79a4 100644 --- a/parse-options.c +++ b/parse-options.c @@ -1536,6 +1536,12 @@ void show_usage_with_options_if_asked(int ac, const char **av, usage_with_options_internal(NULL, usagestr, opts, USAGE_FULL, USAGE_TO_STDOUT); exit(0); + } else if (!strcmp(av[1], "--git-completion-helper")) { + show_gitcomp(opts, 0); + exit(0); + } else if (!strcmp(av[1], "--git-completion-helper-all")) { + show_gitcomp(opts, 1); + exit(0); } } } diff --git a/parse-options.h b/parse-options.h index f68b51ceca..7ad33801fe 100644 --- a/parse-options.h +++ b/parse-options.h @@ -427,6 +427,12 @@ int parse_options(int argc, const char **argv, const char *prefix, NORETURN void usage_with_options(const char * const *usagestr, const struct option *options); +/* + * If the only argument is -h, --help-all, --git-completion-helper or + * --git-completion-helper-all, print the answer and exit. Call this + * before the command does any work. Then asking for help or for the + * completion list does not wait for input or leave files behind. + */ void show_usage_with_options_if_asked(int ac, const char **av, const char * const *usage, const struct option *options); diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh index 73c6e3d448..1305614381 100755 --- a/t/t9300-fast-import.sh +++ b/t/t9300-fast-import.sh @@ -2914,6 +2914,22 @@ test_expect_success 'R: blob appears only once' ' test 1 = $n ' +test_expect_success 'R: --git-completion-helper does not read from stdin' ' + find .git/objects/pack -name "tmp_pack_*" >before && + echo garbage | git fast-import --git-completion-helper >out && + test_grep -e "--date-format=" out && + find .git/objects/pack -name "tmp_pack_*" >after && + test_cmp before after +' + +test_expect_success 'R: --git-completion-helper-all does not read from stdin' ' + find .git/objects/pack -name "tmp_pack_*" >before && + echo garbage | git fast-import --git-completion-helper-all >all && + test_grep -e "--allow-unsafe-features" all && + find .git/objects/pack -name "tmp_pack_*" >after && + test_cmp before after +' + ### ### series S (mark and path parsing) ### -- 2.43.0 ``` ## Tanga Joris, 2026-11-05 00:39 Subject: [Outreachy PATCH v4 2/3] git.c: drop NO_PARSEOPT from builtins that use parse-options Message-ID: <20261105003914.708836-2-tangajoris@gmail.com> URL: https://gitlist.dev/e/20261105003914.708836-2-tangajoris%40gmail.com In-Reply-To: <20261105003914.708836-1-tangajoris@gmail.com> ``` The NO_PARSEOPT flag in the commands[] table marks builtins that parse their options by hand. Builtins marked with it are left out of "git --list-cmds=parseopt", which bash completion uses to decide whether it can ask a command for its options with "--git-completion-helper". Four builtins have since been converted to parse-options, but are still marked NO_PARSEOPT: - patch-id, in 2871f4d447 (builtin: patch-id: add --verbatim as a command mode, 2022-10-24) - diff-pairs, which has used parse-options since it was introduced in 5bd10b2adc (builtin: introduce diff-pairs command, 2025-02-28) - fast-import, in 863937696e (fast-import: use parse_options() for command line options, 2026-08-11) - var, in e19f0779c6 (var: add "-z" output mode, 2026-09-26) This causes "git fast-import --", "git patch-id --" and "git diff-pairs --" to offer no options, even though they are listed by "--git-completion-helper". Remove the flag from these four builtins, like 66fa6e8ed8 (git.c: update NO_PARSEOPT markings, 2022-08-19) did for others. Signed-off-by: Tanga Joris --- git.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/git.c b/git.c index d7b5f88e52..2d27ad4175 100644 --- a/git.c +++ b/git.c @@ -571,11 +571,11 @@ static struct cmd_struct commands[] = { { "diff", cmd_diff, NO_PARSEOPT }, { "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT }, { "diff-index", cmd_diff_index, RUN_SETUP | NO_PARSEOPT }, - { "diff-pairs", cmd_diff_pairs, RUN_SETUP | NO_PARSEOPT }, + { "diff-pairs", cmd_diff_pairs, RUN_SETUP }, { "diff-tree", cmd_diff_tree, RUN_SETUP | NO_PARSEOPT }, { "difftool", cmd_difftool, RUN_SETUP_GENTLY }, { "fast-export", cmd_fast_export, RUN_SETUP }, - { "fast-import", cmd_fast_import, RUN_SETUP | NO_PARSEOPT }, + { "fast-import", cmd_fast_import, RUN_SETUP }, { "fetch", cmd_fetch, RUN_SETUP }, { "fetch-pack", cmd_fetch_pack, RUN_SETUP | NO_PARSEOPT }, { "fmt-merge-msg", cmd_fmt_merge_msg, RUN_SETUP }, @@ -626,7 +626,7 @@ static struct cmd_struct commands[] = { { "pack-redundant", cmd_pack_redundant, RUN_SETUP | NO_PARSEOPT | DEPRECATED }, #endif { "pack-refs", cmd_pack_refs, RUN_SETUP }, - { "patch-id", cmd_patch_id, RUN_SETUP_GENTLY | NO_PARSEOPT }, + { "patch-id", cmd_patch_id, RUN_SETUP_GENTLY }, { "pickaxe", cmd_blame, RUN_SETUP }, { "prune", cmd_prune, RUN_SETUP }, { "prune-packed", cmd_prune_packed, RUN_SETUP }, @@ -676,7 +676,7 @@ static struct cmd_struct commands[] = { { "upload-archive--writer", cmd_upload_archive_writer, NO_PARSEOPT }, { "upload-pack", cmd_upload_pack }, { "url-parse", cmd_url_parse }, - { "var", cmd_var, RUN_SETUP_GENTLY | NO_PARSEOPT }, + { "var", cmd_var, RUN_SETUP_GENTLY }, { "verify-commit", cmd_verify_commit, RUN_SETUP }, { "verify-pack", cmd_verify_pack }, { "verify-tag", cmd_verify_tag, RUN_SETUP }, -- 2.43.0 ``` ## Tanga Joris, 2026-11-05 00:39 Subject: [Outreachy PATCH v4 3/3] t0012: catch builtins that keep NO_PARSEOPT after conversion Message-ID: <20261105003914.708836-3-tangajoris@gmail.com> URL: https://gitlist.dev/e/20261105003914.708836-3-tangajoris%40gmail.com In-Reply-To: <20261105003914.708836-1-tangajoris@gmail.com> ``` When a builtin is converted to parse-options, the NO_PARSEOPT flag in git.c has to be removed by hand. No test checks this. The previous commit removes the flag from four builtins where it was left behind. Add a test that runs "git --git-completion-helper" for every builtin left out of "git --list-cmds=parseopt". The test fails if the command prints an option starting with "--". Without the previous commit, it fails for diff-pairs, fast-import and patch-id. The test skips diff, diff-files, diff-index, diff-tree and rev-list, because they print the diff options. It also skips rev-parse, because it prints the argument back. It does not catch var, because var only has short options (like -l) and the helper only lists long options. Suggested-by: Kaartic Sivaraam Signed-off-by: Tanga Joris --- Kaartic, I put your test in a separate patch for now, so it is easy to find. I'm happy to squash it into 2/3 if you prefer it there. t/t0012-help.sh | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/t/t0012-help.sh b/t/t0012-help.sh index b0a3fdbbbc..41197c67e2 100755 --- a/t/t0012-help.sh +++ b/t/t0012-help.sh @@ -267,4 +267,29 @@ do ' done parseopt && + tr " " "\n" sorted-parseopt && + sort builtins >sorted-builtins && + comm -23 sorted-builtins sorted-parseopt >no-parseopt +' + +while read builtin +do + case "$builtin" in + # these answer with the diff options + diff|diff-files|diff-index|diff-tree|rev-list) + continue ;; + # rev-parse prints unknown arguments back + rev-parse) + continue ;; + esac + + test_expect_success "$builtin does not answer --git-completion-helper" ' + test_might_fail git $builtin --git-completion-helper \ + output 2>/dev/null && + test_grep ! "^--" output + ' +done