From: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Date: Fri, 30 Oct 2026 10:21:57 GMT
Subject: Re: [Outreachy PATCH v3 2/2] git.c: drop NO_PARSEOPT from builtins that use parse-options
Message-ID: <d026d6a0-5b1b-4bbf-bd1f-5f41c9c2a4e4@gmail.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 <builtins

+test_expect_success 'generate list of builtins marked NO_PARSEOPT' '
+	git --list-cmds=parseopt | tr " " "\n" >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 \
+			</dev/null >output 2>/dev/null &&
+		# rev-parse echoes unknown arguments back
+		! grep -v "^--git-completion-helper\$" output | grep "^--"
+	'
+done <no-parseopt
+
 test_done

-- 
Sivaraam