* [PATCH 1/2] parse-options: introduce die_for_required_opt()
2026-06-03 11:10 [PATCH 0/2] parse-options: introduce die_for_required_opt() helper Siddharth Shrimali
@ 2026-06-03 11:10 ` Siddharth Shrimali
2026-06-03 19:48 ` Jean-Noël AVILA
2026-06-04 8:10 ` Christian Couder
2026-06-03 11:10 ` [PATCH 2/2] builtin/add: use die_for_required_opt() helper Siddharth Shrimali
2026-06-04 7:45 ` [PATCH 0/2] parse-options: introduce " Christian Couder
2 siblings, 2 replies; 8+ messages in thread
From: Siddharth Shrimali @ 2026-06-03 11:10 UTC (permalink / raw)
To: git; +Cc: gitster, christian.couder, toon, jn.avila, r.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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 1/2] parse-options: introduce die_for_required_opt()
2026-06-03 11:10 ` [PATCH 1/2] parse-options: introduce die_for_required_opt() Siddharth Shrimali
@ 2026-06-03 19:48 ` Jean-Noël AVILA
2026-06-04 8:00 ` Christian Couder
2026-06-04 8:10 ` Christian Couder
1 sibling, 1 reply; 8+ messages in thread
From: Jean-Noël AVILA @ 2026-06-03 19:48 UTC (permalink / raw)
To: git, Siddharth Shrimali
Cc: gitster, christian.couder, toon, r.siddharth.shrimali
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/2] parse-options: introduce die_for_required_opt()
2026-06-03 19:48 ` Jean-Noël AVILA
@ 2026-06-04 8:00 ` Christian Couder
0 siblings, 0 replies; 8+ messages in thread
From: Christian Couder @ 2026-06-04 8:00 UTC (permalink / raw)
To: Jean-Noël AVILA; +Cc: git, Siddharth Shrimali, gitster, toon
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] parse-options: introduce die_for_required_opt()
2026-06-03 11:10 ` [PATCH 1/2] parse-options: introduce die_for_required_opt() Siddharth Shrimali
2026-06-03 19:48 ` Jean-Noël AVILA
@ 2026-06-04 8:10 ` Christian Couder
1 sibling, 0 replies; 8+ messages in thread
From: Christian Couder @ 2026-06-04 8:10 UTC (permalink / raw)
To: Siddharth Shrimali; +Cc: git, gitster, toon, jn.avila
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] builtin/add: use die_for_required_opt() helper
2026-06-03 11:10 [PATCH 0/2] parse-options: introduce die_for_required_opt() helper Siddharth Shrimali
2026-06-03 11:10 ` [PATCH 1/2] parse-options: introduce die_for_required_opt() Siddharth Shrimali
@ 2026-06-03 11:10 ` Siddharth Shrimali
2026-06-04 8:27 ` Christian Couder
2026-06-04 7:45 ` [PATCH 0/2] parse-options: introduce " Christian Couder
2 siblings, 1 reply; 8+ messages in thread
From: Siddharth Shrimali @ 2026-06-03 11:10 UTC (permalink / raw)
To: git; +Cc: gitster, christian.couder, toon, jn.avila, r.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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 2/2] builtin/add: use die_for_required_opt() helper
2026-06-03 11:10 ` [PATCH 2/2] builtin/add: use die_for_required_opt() helper Siddharth Shrimali
@ 2026-06-04 8:27 ` Christian Couder
0 siblings, 0 replies; 8+ messages in thread
From: Christian Couder @ 2026-06-04 8:27 UTC (permalink / raw)
To: Siddharth Shrimali; +Cc: git, gitster, toon, jn.avila
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] parse-options: introduce die_for_required_opt() helper
2026-06-03 11:10 [PATCH 0/2] parse-options: introduce die_for_required_opt() helper Siddharth Shrimali
2026-06-03 11:10 ` [PATCH 1/2] parse-options: introduce die_for_required_opt() Siddharth Shrimali
2026-06-03 11:10 ` [PATCH 2/2] builtin/add: use die_for_required_opt() helper Siddharth Shrimali
@ 2026-06-04 7:45 ` Christian Couder
2 siblings, 0 replies; 8+ messages in thread
From: Christian Couder @ 2026-06-04 7:45 UTC (permalink / raw)
To: Siddharth Shrimali; +Cc: git, gitster, toon, jn.avila
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.
^ permalink raw reply [flat|nested] 8+ messages in thread