[PATCH 0/2] parse-options: introduce die_for_required_opt() helper

[PATCH 0/2] parse-options: introduce die_for_required_opt() helper

[Siddharth Shrimali]

Many built-in commands in Git manually check for option prerequisites 
(i.e., option X relies on option Y being present) using explicit 
conditional blocks and duplicated error message strings.

This short series comes out of a discussion with Christian about 
localization and code duplication. To address these issues, it 
introduces a centralized API helper that handles simple option 
prerequisites safely.

- Patch 1 introduces the `die_for_required_opt()` helper function 
  inside parse-options.
  
- Patch 2 cleans up `builtin/add.c` as a proof-of-concept by migrating 
  its manual prerequisite checks for '--ignore-missing' and 
  '--pathspec-file-nul' over to the new helper.

If this initial approach looks good, we can later extend the helper 
to handle more complex multi-option dependencies.

Siddharth Shrimali (2):
  parse-options: introduce die_for_required_opt()
  builtin/add: use die_for_required_opt() helper

 builtin/add.c   | 7 +++----
 parse-options.c | 7 +++++++
 parse-options.h | 3 +++
 3 files changed, 13 insertions(+), 4 deletions(-)

-- 
2.54.0

[PATCH 1/2] parse-options: introduce die_for_required_opt()

[Siddharth Shrimali]

Introduce a new helper function die_for_required_opt() to check if a
given option is present without its required prerequisite option.

This provides a centralized API for handling simple option dependencies
(i.e., X requires Y), matching the style of the existing mutual-exclusion
helpers like die_for_incompatible_opt{2,3,4}().

Suggested-by: Christian Couder <christian.couder@gmail.com>
Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
 parse-options.c | 7 +++++++
 parse-options.h | 3 +++
 2 files changed, 10 insertions(+)

diff --git a/parse-options.c b/parse-options.c
index a676da86f5..e100f9a0c1 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1558,3 +1558,10 @@ void die_for_incompatible_opt4(int opt1, const char *opt1_name,
 		break;
 	}
 }
+
+void die_for_required_opt(int opt1, const char *opt1_name,
+			  int opt2, const char *opt2_name)
+{
+	if (opt1 && !opt2)
+		die(_("the option '%s' requires '%s'"), opt1_name, opt2_name);
+}
diff --git a/parse-options.h b/parse-options.h
index 0d1f738f8d..99dc53325d 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -460,6 +460,9 @@ static inline void die_for_incompatible_opt2(int opt1, const char *opt1_name,
 				  0, "");
 }
 
+void die_for_required_opt(int opt1, const char *opt1_name,
+			  int opt2, const char *opt2_name);
+
 /*
  * Use these assertions for callbacks that expect to be called with NONEG and
  * NOARG respectively, and do not otherwise handle the "unset" and "arg"
-- 
2.54.0

[PATCH 2/2] builtin/add: use die_for_required_opt() helper

[Siddharth Shrimali]

Clean up manual option dependency checks by replacing explicit conditional
blocks with the newly introduced die_for_required_opt() helper function.

Specifically, simplify the prerequisite check logic for both
'--ignore-missing' (which requires '--dry-run') and '--pathspec-file-nul'
(which requires '--pathspec-from-file').

Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
 builtin/add.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index c859f66519..a5c91c6dcf 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -441,8 +441,7 @@ int cmd_add(int argc,
 	if (addremove && take_worktree_changes)
 		die(_("options '%s' and '%s' cannot be used together"), "-A", "-u");
 
-	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");
 
 	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) {
-- 
2.54.0

Re: [PATCH 1/2] parse-options: introduce die_for_required_opt()

[Jean-Noël AVILA]

On Wednesday, 3 June 2026 13:10:43 CEST Siddharth Shrimali wrote:
> Introduce a new helper function die_for_required_opt() to check if a
> given option is present without its required prerequisite option.
> 
> This provides a centralized API for handling simple option dependencies
> (i.e., X requires Y), matching the style of the existing mutual-exclusion
> helpers like die_for_incompatible_opt{2,3,4}().
> 
> Suggested-by: Christian Couder <christian.couder@gmail.com>
> Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
> ---
>  parse-options.c | 7 +++++++
>  parse-options.h | 3 +++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/parse-options.c b/parse-options.c
> index a676da86f5..e100f9a0c1 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -1558,3 +1558,10 @@ void die_for_incompatible_opt4(int opt1, const char
> *opt1_name, break;
>  	}
>  }
> +
> +void die_for_required_opt(int opt1, const char *opt1_name,
> +			  int opt2, const char *opt2_name)

Hello,

First thanks for trying to uniformize/simplify option checking. The 
translators will be happy.

To me, "die_for_required_opt" is a misnomer as the function does not die for 
an existing "required" condition, unlike the other functions such as 
die_for_incompatible_opt<n>.

The names of the parameters do not indicate that the test is not symmetrical 
(not failing on XOR).

Maybe something like "die_for_missing_opt(int tested_opt, const char 
*tested_opt_name, int required_opt, const char *required_opt_name)

would make it more understandable.

Re: [PATCH 0/2] parse-options: introduce die_for_required_opt() helper

[Christian Couder]

On Wed, Jun 3, 2026 at 1:11 PM Siddharth Shrimali
<r.siddharth.shrimali@gmail.com> wrote:
>
> Many built-in commands in Git manually check for option prerequisites
> (i.e., option X relies on option Y being present) using explicit
> conditional blocks and duplicated error message strings.
>
> This short series comes out of a discussion with Christian about
> localization and code duplication. To address these issues, it
> introduces a centralized API helper that handles simple option
> prerequisites safely.

I think it would be nice to mention around here that the new function
was inspired by die_for_incompatible_opt2() and similar functions.

> - Patch 1 introduces the `die_for_required_opt()` helper function
>   inside parse-options.
>
> - Patch 2 cleans up `builtin/add.c` as a proof-of-concept by migrating
>   its manual prerequisite checks for '--ignore-missing' and
>   '--pathspec-file-nul' over to the new helper.
>
> If this initial approach looks good, we can later extend the helper
> to handle more complex multi-option dependencies.

Yeah, for functions with more arguments to address cases like "option
X requires both options Y and Z" or "option X requires either option Y
or option Z", I think it's not clear yet what would be the most useful
and what's the best name for such functions.

Re: [PATCH 1/2] parse-options: introduce die_for_required_opt()

[Christian Couder]

Hi,

On Wed, Jun 3, 2026 at 9:49 PM Jean-Noël AVILA <jn.avila@free.fr> wrote:

> To me, "die_for_required_opt" is a misnomer as the function does not die for
> an existing "required" condition, unlike the other functions such as
> die_for_incompatible_opt<n>.
>
> The names of the parameters do not indicate that the test is not symmetrical
> (not failing on XOR).
>
> Maybe something like "die_for_missing_opt(int tested_opt, const char
> *tested_opt_name, int required_opt, const char *required_opt_name)
>
> would make it more understandable.

Yeah, I agree it's better.

With "dependent_opt" instead of "tested_opt", I think it would be even better.

Thanks.

Re: [PATCH 1/2] parse-options: introduce die_for_required_opt()

[Christian Couder]

On Wed, Jun 3, 2026 at 1:11 PM Siddharth Shrimali
<r.siddharth.shrimali@gmail.com> wrote:
>
> Introduce a new helper function die_for_required_opt() to check if a
> given option is present without its required prerequisite option.
>
> This provides a centralized API for handling simple option dependencies
> (i.e., X requires Y), matching the style of the existing mutual-exclusion
> helpers like die_for_incompatible_opt{2,3,4}().
>
> Suggested-by: Christian Couder <christian.couder@gmail.com>

In general it's simpler for GSoC contributors to mention all your
mentors in "Mentored-by: ..." trailers in all your patches during your
GSoC, rather than keeping track of who helped you with each patch.

> Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
> ---
>  parse-options.c | 7 +++++++
>  parse-options.h | 3 +++
>  2 files changed, 10 insertions(+)

I think it would be nice if the new function could actually be used in
a single *.c file. It would be even nicer if there was an existing
test that already checked that the dependent option needs the required
option. This way we would also already ensure that the new helper is
working properly.

Re: [PATCH 2/2] builtin/add: use die_for_required_opt() helper

[Christian Couder]

On Wed, Jun 3, 2026 at 1:11 PM Siddharth Shrimali
<r.siddharth.shrimali@gmail.com> wrote:
>
> Clean up manual option dependency checks by replacing explicit conditional
> blocks with the newly introduced die_for_required_opt() helper function.
>
> Specifically, simplify the prerequisite check logic for both
> '--ignore-missing' (which requires '--dry-run') and '--pathspec-file-nul'
> (which requires '--pathspec-from-file').

It's a good idea to use the new helper function for
'--pathspec-file-nul' requiring '--pathspec-from-file' because it
looks like this is tested a lot already:

$ git grep requires | grep 'the option'
t2026-checkout-pathspec-file.sh:    test_grep -e "the option
.--pathspec-file-nul. requires .--pathspec-from-file." err
t2072-restore-pathspec-file.sh:    test_grep -e "the option
.--pathspec-file-nul. requires .--pathspec-from-file." err &&
t3601-rm-pathspec-file.sh:    test_grep -e "the option
.--pathspec-file-nul. requires .--pathspec-from-file." err &&
t3704-add-pathspec-file.sh:    test_grep -e "the option
.--pathspec-file-nul. requires .--pathspec-from-file." err &&
t3909-stash-pathspec-file.sh:    test_grep -e "the option
.--pathspec-file-nul. requires .--pathspec-from-file." err
t7107-reset-pathspec-file.sh:    test_grep -e "the option
.--pathspec-file-nul. requires .--pathspec-from-file." err &&
t7526-commit-pathspec-file.sh:    test_grep -e "the option
.--pathspec-file-nul. requires .--pathspec-from-file." err &&

You could mention this in the commit message.

Also it might be worth squashing this patch into the previous one.

Thanks.

[PATCH v2] parse-options: introduce die_for_missing_opt()

[Siddharth Shrimali]

Introduce die_for_missing_opt() to check if a dependent option is
present without its required prerequisite. This provides a centralized
API for simple option dependencies (X requires Y), inspired by and
matching the style of die_for_incompatible_opt{2,3,4}().

Use the new helper in builtin/add.c to replace the manual prerequisite
check for '--pathspec-file-nul' (requires '--pathspec-from-file'). This
case is already exercised by existing tests in t3704-add-pathspec-file.sh
and several other pathspec-file test scripts, ensuring the new helper is
verified without additional test code.

Suggested-by: Christian Couder <christian.couder@gmail.com>
Suggested-by: Jean-Noël AVILA <jn.avila@free.fr>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Siddharth Asthana <siddharthasthana31@gmail.com>
Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
Changes since v1:
  - Squashed the implementation patch and the caller patch into a single,
    unified patch as suggested by Christian.
  - Renamed the helper function from die_for_require_opt() to
    die_for_missing_opt() to improve clarity.
  - Updated the argument names and logic order to better match the style of
    die_for_incompatible_opt*().
  - Dropped the conversion of the '--ignore-missing' check in builtin/add.c
    to keep this initial iteration strictly focused on a single, clean
    example ('--pathspec-file-nul').

 builtin/add.c   | 4 ++--
 parse-options.c | 7 +++++++
 parse-options.h | 3 +++
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index c859f66519..505834ad3f 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -462,6 +462,8 @@ int cmd_add(int argc,
 		       PATHSPEC_SYMLINK_LEADING_PATH,
 		       prefix, argv);
 
+	die_for_missing_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 +472,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) {
diff --git a/parse-options.c b/parse-options.c
index a676da86f5..11e40669eb 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1558,3 +1558,10 @@ void die_for_incompatible_opt4(int opt1, const char *opt1_name,
 		break;
 	}
 }
+
+void die_for_missing_opt(int dependent_opt, const char *dependent_opt_name,
+			 int required_opt, const char *required_opt_name)
+{
+	if (dependent_opt && !required_opt)
+		die(_("the option '%s' requires '%s'"), dependent_opt_name, required_opt_name);
+}
diff --git a/parse-options.h b/parse-options.h
index 0d1f738f8d..5b41d2fd39 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -460,6 +460,9 @@ static inline void die_for_incompatible_opt2(int opt1, const char *opt1_name,
 				  0, "");
 }
 
+void die_for_missing_opt(int dependent_opt, const char *dependent_opt_name,
+			 int required_opt, const char *required_opt_name);
+
 /*
  * Use these assertions for callbacks that expect to be called with NONEG and
  * NOARG respectively, and do not otherwise handle the "unset" and "arg"
-- 
2.54.0

Re: [PATCH 2/2] builtin/add: use die_for_required_opt() helper

[Junio C Hamano]

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) {