Re: [Outreachy PATCH v3 1/2] parse-options: answer --git-completion-helper before command runs

[Kaartic Sivaraam <kaartic.sivaraam@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 --<TAB>" 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 <l.s.r@web.de>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Tanga Joris <tangajoris@gmail.com>
> ---
> 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