From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 08 Jun 2026 17:00:09 GMT
Subject: Re: [PATCH 2/2] builtin/add: use die_for_required_opt() helper
Message-ID: <xmqqecihvug6.fsf@gitster.g>
In-Reply-To: <20260603111044.39116-3-r.siddharth.shrimali@gmail.com>

Siddharth Shrimali <r.siddharth.shrimali@gmail.com> writes:

> -	if (!show_only && ignore_missing)
> -		die(_("the option '%s' requires '%s'"), "--ignore-missing", "--dry-run");
> +	die_for_required_opt(ignore_missing, "--ignore-missing", show_only, "--dry-run");

As builtin_add_options[] knows that ignore_missing (variable) comes
from the use of "--ignore-missing" (option), and similarly the value
of show_only (variable) is tightly linked to "--dry-run" (option),
it feels quite wasteful having to pass both.

I wonder if we can do this more declaratively, perhaps by
introducing extra types of elements in struct option[] that tells
"--ignore-missing" requires "--dry-run", so that the client code
does not have to do anything more than calling parse_options() to
implement this?

A possible counter-argument may be that the value of, say,
ignore_missing may be different at this point in the code from what
was set by parse_options() when the command line was processed, but
then it means that the message (with or without your patch) is
misleading, so I am not sure if that counter-argument is valid.

>  	if (chmod_arg && ((chmod_arg[0] != '-' && chmod_arg[0] != '+') ||
>  			  chmod_arg[1] != 'x' || chmod_arg[2]))
> @@ -462,6 +461,8 @@ int cmd_add(int argc,
>  		       PATHSPEC_SYMLINK_LEADING_PATH,
>  		       prefix, argv);
>  
> +	die_for_required_opt(pathspec_file_nul, "--pathspec-file-nul",
> +				!!pathspec_from_file, "--pathspec-from-file");
>  	if (pathspec_from_file) {
>  		if (pathspec.nr)
>  			die(_("'%s' and pathspec arguments cannot be used together"), "--pathspec-from-file");
> @@ -470,8 +471,6 @@ int cmd_add(int argc,
>  				    PATHSPEC_PREFER_FULL |
>  				    PATHSPEC_SYMLINK_LEADING_PATH,
>  				    prefix, pathspec_from_file, pathspec_file_nul);
> -	} else if (pathspec_file_nul) {
> -		die(_("the option '%s' requires '%s'"), "--pathspec-file-nul", "--pathspec-from-file");
>  	}
>  
>  	if (require_pathspec && pathspec.nr == 0) {