{"thread":{"id":"65745","subject":"[PATCH 0/2] parse-options: introduce die_for_required_opt() helper","startedAt":"2026-06-03T11:11:30Z","lastAt":"2026-06-08T17:00:13Z","messageCount":10,"participants":["Siddharth Shrimali","Jean-Noël AVILA","Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"544602","messageId":"20260603111044.39116-1-r.siddharth.shrimali@gmail.com","threadId":"65745","inReplyTo":null,"subject":"[PATCH 0/2] parse-options: introduce die_for_required_opt() helper","fromName":"Siddharth Shrimali","fromEmail":"r.siddharth.shrimali@gmail.com","sentAt":"2026-06-03T11:10:42Z","receivedAt":"2026-06-03T11:11:30Z","isPatch":true,"body":"Many built-in commands in Git manually check for option prerequisites \n(i.e., option X relies on option Y being present) using explicit \nconditional blocks and duplicated error message strings.\n\nThis short series comes out of a discussion with Christian about \nlocalization and code duplication. To address these issues, it \nintroduces a centralized API helper that handles simple option \nprerequisites safely.\n\n- Patch 1 introduces the `die_for_required_opt()` helper function \n inside parse-options.\n \n- Patch 2 cleans up `builtin/add.c` as a proof-of-concept by migrating \n its manual prerequisite checks for '--ignore-missing' and \n '--pathspec-file-nul' over to the new helper.\n\nIf this initial approach looks good, we can later extend the helper \nto handle more complex multi-option dependencies.\n\nSiddharth Shrimali (2):\n parse-options: introduce die_for_required_opt()\n builtin/add: use die_for_required_opt() helper\n\n builtin/add.c | 7 +++----\n parse-options.c | 7 +++++++\n parse-options.h | 3 +++\n 3 files changed, 13 insertions(+), 4 deletions(-)\n\n-- \n2.54.0\n\n"},{"id":"544603","messageId":"20260603111044.39116-2-r.siddharth.shrimali@gmail.com","threadId":"65745","inReplyTo":"20260603111044.39116-1-r.siddharth.shrimali@gmail.com","subject":"[PATCH 1/2] parse-options: introduce die_for_required_opt()","fromName":"Siddharth Shrimali","fromEmail":"r.siddharth.shrimali@gmail.com","sentAt":"2026-06-03T11:10:43Z","receivedAt":"2026-06-03T11:11:38Z","isPatch":true,"body":"Introduce a new helper function die_for_required_opt() to check if a\ngiven option is present without its required prerequisite option.\n\nThis provides a centralized API for handling simple option dependencies\n(i.e., X requires Y), matching the style of the existing mutual-exclusion\nhelpers like die_for_incompatible_opt{2,3,4}().\n\nSuggested-by: Christian Couder \nSigned-off-by: Siddharth Shrimali \n---\n parse-options.c | 7 +++++++\n parse-options.h | 3 +++\n 2 files changed, 10 insertions(+)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex a676da86f5..e100f9a0c1 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -1558,3 +1558,10 @@ void die_for_incompatible_opt4(int opt1, const char *opt1_name,\n \t\tbreak;\n \t}\n }\n+\n+void die_for_required_opt(int opt1, const char *opt1_name,\n+\t\t\t int opt2, const char *opt2_name)\n+{\n+\tif (opt1 && !opt2)\n+\t\tdie(_(\"the option '%s' requires '%s'\"), opt1_name, opt2_name);\n+}\ndiff --git a/parse-options.h b/parse-options.h\nindex 0d1f738f8d..99dc53325d 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -460,6 +460,9 @@ static inline void die_for_incompatible_opt2(int opt1, const char *opt1_name,\n \t\t\t\t 0, \"\");\n }\n \n+void die_for_required_opt(int opt1, const char *opt1_name,\n+\t\t\t int opt2, const char *opt2_name);\n+\n /*\n * Use these assertions for callbacks that expect to be called with NONEG and\n * NOARG respectively, and do not otherwise handle the \"unset\" and \"arg\"\n-- \n2.54.0\n\n"},{"id":"544604","messageId":"20260603111044.39116-3-r.siddharth.shrimali@gmail.com","threadId":"65745","inReplyTo":"20260603111044.39116-1-r.siddharth.shrimali@gmail.com","subject":"[PATCH 2/2] builtin/add: use die_for_required_opt() helper","fromName":"Siddharth Shrimali","fromEmail":"r.siddharth.shrimali@gmail.com","sentAt":"2026-06-03T11:10:44Z","receivedAt":"2026-06-03T11:11:45Z","isPatch":true,"body":"Clean up manual option dependency checks by replacing explicit conditional\nblocks with the newly introduced die_for_required_opt() helper function.\n\nSpecifically, simplify the prerequisite check logic for both\n'--ignore-missing' (which requires '--dry-run') and '--pathspec-file-nul'\n(which requires '--pathspec-from-file').\n\nSigned-off-by: Siddharth Shrimali \n---\n builtin/add.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex c859f66519..a5c91c6dcf 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -441,8 +441,7 @@ int cmd_add(int argc,\n \tif (addremove && take_worktree_changes)\n \t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"-A\", \"-u\");\n \n-\tif (!show_only && ignore_missing)\n-\t\tdie(_(\"the option '%s' requires '%s'\"), \"--ignore-missing\", \"--dry-run\");\n+\tdie_for_required_opt(ignore_missing, \"--ignore-missing\", show_only, \"--dry-run\");\n \n \tif (chmod_arg && ((chmod_arg[0] != '-' && chmod_arg[0] != '+') ||\n \t\t\t chmod_arg[1] != 'x' || chmod_arg[2]))\n@@ -462,6 +461,8 @@ int cmd_add(int argc,\n \t\t PATHSPEC_SYMLINK_LEADING_PATH,\n \t\t prefix, argv);\n \n+\tdie_for_required_opt(pathspec_file_nul, \"--pathspec-file-nul\",\n+\t\t\t\t!!pathspec_from_file, \"--pathspec-from-file\");\n \tif (pathspec_from_file) {\n \t\tif (pathspec.nr)\n \t\t\tdie(_(\"'%s' and pathspec arguments cannot be used together\"), \"--pathspec-from-file\");\n@@ -470,8 +471,6 @@ int cmd_add(int argc,\n \t\t\t\t PATHSPEC_PREFER_FULL |\n \t\t\t\t PATHSPEC_SYMLINK_LEADING_PATH,\n \t\t\t\t prefix, pathspec_from_file, pathspec_file_nul);\n-\t} else if (pathspec_file_nul) {\n-\t\tdie(_(\"the option '%s' requires '%s'\"), \"--pathspec-file-nul\", \"--pathspec-from-file\");\n \t}\n \n \tif (require_pathspec && pathspec.nr == 0) {\n-- \n2.54.0\n\n"},{"id":"544642","messageId":"2412618.ElGaqSPkdT@piment-oiseau","threadId":"65745","inReplyTo":"20260603111044.39116-2-r.siddharth.shrimali@gmail.com","subject":"Re: [PATCH 1/2] parse-options: introduce die_for_required_opt()","fromName":"Jean-Noël AVILA","fromEmail":"jn.avila@free.fr","sentAt":"2026-06-03T19:48:53Z","receivedAt":"2026-06-03T19:58:28Z","isPatch":true,"body":"On Wednesday, 3 June 2026 13:10:43 CEST Siddharth Shrimali wrote:\n> Introduce a new helper function die_for_required_opt() to check if a\n> given option is present without its required prerequisite option.\n> \n> This provides a centralized API for handling simple option dependencies\n> (i.e., X requires Y), matching the style of the existing mutual-exclusion\n> helpers like die_for_incompatible_opt{2,3,4}().\n> \n> Suggested-by: Christian Couder \n> Signed-off-by: Siddharth Shrimali \n> ---\n> parse-options.c | 7 +++++++\n> parse-options.h | 3 +++\n> 2 files changed, 10 insertions(+)\n> \n> diff --git a/parse-options.c b/parse-options.c\n> index a676da86f5..e100f9a0c1 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -1558,3 +1558,10 @@ void die_for_incompatible_opt4(int opt1, const char\n> *opt1_name, break;\n> \t}\n> }\n> +\n> +void die_for_required_opt(int opt1, const char *opt1_name,\n> +\t\t\t int opt2, const char *opt2_name)\n\nHello,\n\nFirst thanks for trying to uniformize/simplify option checking. The \ntranslators will be happy.\n\nTo me, \"die_for_required_opt\" is a misnomer as the function does not die for \nan existing \"required\" condition, unlike the other functions such as \ndie_for_incompatible_opt.\n\nThe names of the parameters do not indicate that the test is not symmetrical \n(not failing on XOR).\n\nMaybe something like \"die_for_missing_opt(int tested_opt, const char \n*tested_opt_name, int required_opt, const char *required_opt_name)\n\nwould make it more understandable.\n\n\n\n"},{"id":"544678","messageId":"CAP8UFD2=3V6wRRJU0c1KJ-tGdz_F1DtjNd1aV9dWoO8LK5oeSQ@mail.gmail.com","threadId":"65745","inReplyTo":"20260603111044.39116-1-r.siddharth.shrimali@gmail.com","subject":"Re: [PATCH 0/2] parse-options: introduce die_for_required_opt() helper","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-06-04T07:45:23Z","receivedAt":"2026-06-04T07:45:36Z","isPatch":true,"body":"On Wed, Jun 3, 2026 at 1:11 PM Siddharth Shrimali\n wrote:\n>\n> Many built-in commands in Git manually check for option prerequisites\n> (i.e., option X relies on option Y being present) using explicit\n> conditional blocks and duplicated error message strings.\n>\n> This short series comes out of a discussion with Christian about\n> localization and code duplication. To address these issues, it\n> introduces a centralized API helper that handles simple option\n> prerequisites safely.\n\nI think it would be nice to mention around here that the new function\nwas inspired by die_for_incompatible_opt2() and similar functions.\n\n> - Patch 1 introduces the `die_for_required_opt()` helper function\n> inside parse-options.\n>\n> - Patch 2 cleans up `builtin/add.c` as a proof-of-concept by migrating\n> its manual prerequisite checks for '--ignore-missing' and\n> '--pathspec-file-nul' over to the new helper.\n>\n> If this initial approach looks good, we can later extend the helper\n> to handle more complex multi-option dependencies.\n\nYeah, for functions with more arguments to address cases like \"option\nX requires both options Y and Z\" or \"option X requires either option Y\nor option Z\", I think it's not clear yet what would be the most useful\nand what's the best name for such functions.\n"},{"id":"544695","messageId":"CAP8UFD30eMS_8GSOw9BeQZCT5Xtjw4D4py9fR_JvdBd4Yo-hKg@mail.gmail.com","threadId":"65745","inReplyTo":"2412618.ElGaqSPkdT@piment-oiseau","subject":"Re: [PATCH 1/2] parse-options: introduce die_for_required_opt()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-06-04T08:00:53Z","receivedAt":"2026-06-04T08:01:05Z","isPatch":true,"body":"Hi,\n\nOn Wed, Jun 3, 2026 at 9:49 PM Jean-Noël AVILA wrote:\n\n> To me, \"die_for_required_opt\" is a misnomer as the function does not die for\n> an existing \"required\" condition, unlike the other functions such as\n> die_for_incompatible_opt.\n>\n> The names of the parameters do not indicate that the test is not symmetrical\n> (not failing on XOR).\n>\n> Maybe something like \"die_for_missing_opt(int tested_opt, const char\n> *tested_opt_name, int required_opt, const char *required_opt_name)\n>\n> would make it more understandable.\n\nYeah, I agree it's better.\n\nWith \"dependent_opt\" instead of \"tested_opt\", I think it would be even better.\n\nThanks.\n"},{"id":"544696","messageId":"CAP8UFD39G1CQXyxPVEmQSrdnHZ9BxPCH=QLmYBEFMcCnL8hjgg@mail.gmail.com","threadId":"65745","inReplyTo":"20260603111044.39116-2-r.siddharth.shrimali@gmail.com","subject":"Re: [PATCH 1/2] parse-options: introduce die_for_required_opt()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-06-04T08:10:58Z","receivedAt":"2026-06-04T08:11:11Z","isPatch":true,"body":"On Wed, Jun 3, 2026 at 1:11 PM Siddharth Shrimali\n wrote:\n>\n> Introduce a new helper function die_for_required_opt() to check if a\n> given option is present without its required prerequisite option.\n>\n> This provides a centralized API for handling simple option dependencies\n> (i.e., X requires Y), matching the style of the existing mutual-exclusion\n> helpers like die_for_incompatible_opt{2,3,4}().\n>\n> Suggested-by: Christian Couder \n\nIn general it's simpler for GSoC contributors to mention all your\nmentors in \"Mentored-by: ...\" trailers in all your patches during your\nGSoC, rather than keeping track of who helped you with each patch.\n\n> Signed-off-by: Siddharth Shrimali \n> ---\n> parse-options.c | 7 +++++++\n> parse-options.h | 3 +++\n> 2 files changed, 10 insertions(+)\n\nI think it would be nice if the new function could actually be used in\na single *.c file. It would be even nicer if there was an existing\ntest that already checked that the dependent option needs the required\noption. This way we would also already ensure that the new helper is\nworking properly.\n"},{"id":"544698","messageId":"CAP8UFD2A6GGwt0=NQQu9oUM2fz+dQzRkB8oHNwE-PJsFdh9wsA@mail.gmail.com","threadId":"65745","inReplyTo":"20260603111044.39116-3-r.siddharth.shrimali@gmail.com","subject":"Re: [PATCH 2/2] builtin/add: use die_for_required_opt() helper","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-06-04T08:27:35Z","receivedAt":"2026-06-04T08:27:49Z","isPatch":true,"body":"On Wed, Jun 3, 2026 at 1:11 PM Siddharth Shrimali\n wrote:\n>\n> Clean up manual option dependency checks by replacing explicit conditional\n> blocks with the newly introduced die_for_required_opt() helper function.\n>\n> Specifically, simplify the prerequisite check logic for both\n> '--ignore-missing' (which requires '--dry-run') and '--pathspec-file-nul'\n> (which requires '--pathspec-from-file').\n\nIt's a good idea to use the new helper function for\n'--pathspec-file-nul' requiring '--pathspec-from-file' because it\nlooks like this is tested a lot already:\n\n$ git grep requires | grep 'the option'\nt2026-checkout-pathspec-file.sh: test_grep -e \"the option\n.--pathspec-file-nul. requires .--pathspec-from-file.\" err\nt2072-restore-pathspec-file.sh: test_grep -e \"the option\n.--pathspec-file-nul. requires .--pathspec-from-file.\" err &&\nt3601-rm-pathspec-file.sh: test_grep -e \"the option\n.--pathspec-file-nul. requires .--pathspec-from-file.\" err &&\nt3704-add-pathspec-file.sh: test_grep -e \"the option\n.--pathspec-file-nul. requires .--pathspec-from-file.\" err &&\nt3909-stash-pathspec-file.sh: test_grep -e \"the option\n.--pathspec-file-nul. requires .--pathspec-from-file.\" err\nt7107-reset-pathspec-file.sh: test_grep -e \"the option\n.--pathspec-file-nul. requires .--pathspec-from-file.\" err &&\nt7526-commit-pathspec-file.sh: test_grep -e \"the option\n.--pathspec-file-nul. requires .--pathspec-from-file.\" err &&\n\nYou could mention this in the commit message.\n\nAlso it might be worth squashing this patch into the previous one.\n\nThanks.\n"},{"id":"544916","messageId":"20260608124438.42922-1-r.siddharth.shrimali@gmail.com","threadId":"65745","inReplyTo":"20260603111044.39116-1-r.siddharth.shrimali@gmail.com","subject":"[PATCH v2] parse-options: introduce die_for_missing_opt()","fromName":"Siddharth Shrimali","fromEmail":"r.siddharth.shrimali@gmail.com","sentAt":"2026-06-08T12:44:38Z","receivedAt":"2026-06-08T12:45:30Z","isPatch":true,"body":"Introduce die_for_missing_opt() to check if a dependent option is\npresent without its required prerequisite. This provides a centralized\nAPI for simple option dependencies (X requires Y), inspired by and\nmatching the style of die_for_incompatible_opt{2,3,4}().\n\nUse the new helper in builtin/add.c to replace the manual prerequisite\ncheck for '--pathspec-file-nul' (requires '--pathspec-from-file'). This\ncase is already exercised by existing tests in t3704-add-pathspec-file.sh\nand several other pathspec-file test scripts, ensuring the new helper is\nverified without additional test code.\n\nSuggested-by: Christian Couder \nSuggested-by: Jean-Noël AVILA \nMentored-by: Christian Couder \nMentored-by: Siddharth Asthana \nSigned-off-by: Siddharth Shrimali \n---\nChanges since v1:\n - Squashed the implementation patch and the caller patch into a single,\n unified patch as suggested by Christian.\n - Renamed the helper function from die_for_require_opt() to\n die_for_missing_opt() to improve clarity.\n - Updated the argument names and logic order to better match the style of\n die_for_incompatible_opt*().\n - Dropped the conversion of the '--ignore-missing' check in builtin/add.c\n to keep this initial iteration strictly focused on a single, clean\n example ('--pathspec-file-nul').\n\n builtin/add.c | 4 ++--\n parse-options.c | 7 +++++++\n parse-options.h | 3 +++\n 3 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex c859f66519..505834ad3f 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -462,6 +462,8 @@ int cmd_add(int argc,\n \t\t PATHSPEC_SYMLINK_LEADING_PATH,\n \t\t prefix, argv);\n \n+\tdie_for_missing_opt(pathspec_file_nul, \"--pathspec-file-nul\",\n+\t\t\t !!pathspec_from_file, \"--pathspec-from-file\");\n \tif (pathspec_from_file) {\n \t\tif (pathspec.nr)\n \t\t\tdie(_(\"'%s' and pathspec arguments cannot be used together\"), \"--pathspec-from-file\");\n@@ -470,8 +472,6 @@ int cmd_add(int argc,\n \t\t\t\t PATHSPEC_PREFER_FULL |\n \t\t\t\t PATHSPEC_SYMLINK_LEADING_PATH,\n \t\t\t\t prefix, pathspec_from_file, pathspec_file_nul);\n-\t} else if (pathspec_file_nul) {\n-\t\tdie(_(\"the option '%s' requires '%s'\"), \"--pathspec-file-nul\", \"--pathspec-from-file\");\n \t}\n \n \tif (require_pathspec && pathspec.nr == 0) {\ndiff --git a/parse-options.c b/parse-options.c\nindex a676da86f5..11e40669eb 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -1558,3 +1558,10 @@ void die_for_incompatible_opt4(int opt1, const char *opt1_name,\n \t\tbreak;\n \t}\n }\n+\n+void die_for_missing_opt(int dependent_opt, const char *dependent_opt_name,\n+\t\t\t int required_opt, const char *required_opt_name)\n+{\n+\tif (dependent_opt && !required_opt)\n+\t\tdie(_(\"the option '%s' requires '%s'\"), dependent_opt_name, required_opt_name);\n+}\ndiff --git a/parse-options.h b/parse-options.h\nindex 0d1f738f8d..5b41d2fd39 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -460,6 +460,9 @@ static inline void die_for_incompatible_opt2(int opt1, const char *opt1_name,\n \t\t\t\t 0, \"\");\n }\n \n+void die_for_missing_opt(int dependent_opt, const char *dependent_opt_name,\n+\t\t\t int required_opt, const char *required_opt_name);\n+\n /*\n * Use these assertions for callbacks that expect to be called with NONEG and\n * NOARG respectively, and do not otherwise handle the \"unset\" and \"arg\"\n-- \n2.54.0\n\n"},{"id":"544950","messageId":"xmqqecihvug6.fsf@gitster.g","threadId":"65745","inReplyTo":"20260603111044.39116-3-r.siddharth.shrimali@gmail.com","subject":"Re: [PATCH 2/2] builtin/add: use die_for_required_opt() helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-08T17:00:09Z","receivedAt":"2026-06-08T17:00:13Z","isPatch":true,"body":"Siddharth Shrimali writes:\n\n> -\tif (!show_only && ignore_missing)\n> -\t\tdie(_(\"the option '%s' requires '%s'\"), \"--ignore-missing\", \"--dry-run\");\n> +\tdie_for_required_opt(ignore_missing, \"--ignore-missing\", show_only, \"--dry-run\");\n\nAs builtin_add_options[] knows that ignore_missing (variable) comes\nfrom the use of \"--ignore-missing\" (option), and similarly the value\nof show_only (variable) is tightly linked to \"--dry-run\" (option),\nit feels quite wasteful having to pass both.\n\nI wonder if we can do this more declaratively, perhaps by\nintroducing extra types of elements in struct option[] that tells\n\"--ignore-missing\" requires \"--dry-run\", so that the client code\ndoes not have to do anything more than calling parse_options() to\nimplement this?\n\nA possible counter-argument may be that the value of, say,\nignore_missing may be different at this point in the code from what\nwas set by parse_options() when the command line was processed, but\nthen it means that the message (with or without your patch) is\nmisleading, so I am not sure if that counter-argument is valid.\n\n> \tif (chmod_arg && ((chmod_arg[0] != '-' && chmod_arg[0] != '+') ||\n> \t\t\t chmod_arg[1] != 'x' || chmod_arg[2]))\n> @@ -462,6 +461,8 @@ int cmd_add(int argc,\n> \t\t PATHSPEC_SYMLINK_LEADING_PATH,\n> \t\t prefix, argv);\n> \n> +\tdie_for_required_opt(pathspec_file_nul, \"--pathspec-file-nul\",\n> +\t\t\t\t!!pathspec_from_file, \"--pathspec-from-file\");\n> \tif (pathspec_from_file) {\n> \t\tif (pathspec.nr)\n> \t\t\tdie(_(\"'%s' and pathspec arguments cannot be used together\"), \"--pathspec-from-file\");\n> @@ -470,8 +471,6 @@ int cmd_add(int argc,\n> \t\t\t\t PATHSPEC_PREFER_FULL |\n> \t\t\t\t PATHSPEC_SYMLINK_LEADING_PATH,\n> \t\t\t\t prefix, pathspec_from_file, pathspec_file_nul);\n> -\t} else if (pathspec_file_nul) {\n> -\t\tdie(_(\"the option '%s' requires '%s'\"), \"--pathspec-file-nul\", \"--pathspec-from-file\");\n> \t}\n> \n> \tif (require_pathspec && pathspec.nr == 0) {\n"}]}