{"thread":{"id":"56234","subject":"[PATCH 0/1] blame: Skip missing ignore-revs file","startedAt":"2021-08-07T20:28:38Z","lastAt":"2025-11-04T18:22:39Z","messageCount":47,"participants":["Noah Pendleton","Junio C Hamano","Thranur Andul","Patrick Steinhardt","Phillip Wood","D. Ben Knoble","Ben Knoble","Kristoffer Haugsbakk","Johannes Sixt","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"432255","messageId":"20210807202752.1278672-1-noah.pendleton@gmail.com","threadId":"56234","inReplyTo":null,"subject":"[PATCH 0/1] blame: Skip missing ignore-revs file","fromName":"Noah Pendleton","fromEmail":"noah.pendleton@gmail.com","sentAt":"2021-08-07T20:27:51Z","receivedAt":"2021-08-07T20:28:38Z","isPatch":true,"sender":{"key":"noah.pendleton@gmail.com","avatar":"https://gravatar.com/avatar/254bdb2e93ea199f3615ec3880e1a1cee7664e079a0dce9bec08a38ce7e882ba?d=mp&s=160"},"body":"Setting a global `blame.ignoreRevsFile` can be convenient, since I\nusually use `.git-blame-ignore-revs` in repos. If the file is missing,\nthough, `git blame` exits with failure. This patch changes it to skip\nover non-existent ignore-rev files instead of erroring.\n\n\nNoah Pendleton (1):\n blame: skip missing ignore-revs-file's\n\n Documentation/blame-options.txt | 2 +-\n Documentation/config/blame.txt | 3 ++-\n builtin/blame.c | 2 +-\n t/t8013-blame-ignore-revs.sh | 10 ++++++----\n 4 files changed, 10 insertions(+), 7 deletions(-)\n\n-- \n2.32.0\n\n"},{"id":"432258","messageId":"xmqqr1f5hszw.fsf@gitster.g","threadId":"56234","inReplyTo":"20210807202752.1278672-1-noah.pendleton@gmail.com","subject":"Re: [PATCH 0/1] blame: Skip missing ignore-revs file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-07T20:58:27Z","receivedAt":"2021-08-07T20:58:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Noah Pendleton writes:\n\n> Setting a global `blame.ignoreRevsFile` can be convenient, since I\n> usually use `.git-blame-ignore-revs` in repos. If the file is missing,\n> though, `git blame` exits with failure. This patch changes it to skip\n> over non-existent ignore-rev files instead of erroring.\n\nThat cuts both ways, though. Failing upon missing configuration\nfile is a way to catch misconfiguration that is hard to diagnose.\n\nI wonder if we can easily learn where the configuration variable\ncame from in the codepath that diagnoses it as a misconfiguration.\n\nIf it came from a per-repo configuration and names a non-existent\nfile, it clearly is a misconfiguration that we want to flag as an\nerror. Even if it came from a per-user configuration, if it was\nspecified in a conditionally included file, it is likely to be a\nmisconfiguration. If it came from a per-user configuration that\napplies without any condition, it can be a good convenience feature\nto silently (or with a warning) ignore missing file.\n"},{"id":"432259","messageId":"CADm0i3-ToKo1gNTXXLHH6i2d4qpz771VeRjDsfJjgbgMfhx6rA@mail.gmail.com","threadId":"56234","inReplyTo":"xmqqr1f5hszw.fsf@gitster.g","subject":"Re: [PATCH 0/1] blame: Skip missing ignore-revs file","fromName":"Noah Pendleton","fromEmail":"noah.pendleton@gmail.com","sentAt":"2021-08-07T21:34:56Z","receivedAt":"2021-08-07T21:35:11Z","isPatch":true,"sender":{"key":"noah.pendleton@gmail.com","avatar":"https://gravatar.com/avatar/254bdb2e93ea199f3615ec3880e1a1cee7664e079a0dce9bec08a38ce7e882ba?d=mp&s=160"},"body":"Thanks for the quick response!\n\nVery good point about no longer catching misconfiguration. For\ndetecting provenance of a setting, I think we'd need to tag the config\noptions with it when they're loaded, possibly in 'struct\nconfig_set_element' or similar. What do you think about instead\nemitting a warning message on stderr in the case of misconfiguration,\nbut still continuing? Eg:\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex e5b45eddf4..6ee8f29313 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -835,7 +835,9 @@ static void build_ignorelist(struct blame_scoreboard *sb,\n for_each_string_list_item(i, ignore_revs_file_list) {\n if (!strcmp(i->string, \"\"))\n oidset_clear(&sb->ignore_list);\n- else if (file_exists(i->string))\n+ else if (!file_exists(i->string))\n+ warning(_(\"skipping ignore-revs-file %s\"), i->string);\n+ else\n oidset_parse_file_carefully(&sb->ignore_list, i->string,\n peel_to_commit_oid, sb);\n }\n\nOn Sat, Aug 7, 2021, 16:58 Junio C Hamano wrote:\n>\n> Noah Pendleton writes:\n>\n> > Setting a global `blame.ignoreRevsFile` can be convenient, since I\n> > usually use `.git-blame-ignore-revs` in repos. If the file is missing,\n> > though, `git blame` exits with failure. This patch changes it to skip\n> > over non-existent ignore-rev files instead of erroring.\n>\n> That cuts both ways, though. Failing upon missing configuration\n> file is a way to catch misconfiguration that is hard to diagnose.\n>\n> I wonder if we can easily learn where the configuration variable\n> came from in the codepath that diagnoses it as a misconfiguration.\n>\n> If it came from a per-repo configuration and names a non-existent\n> file, it clearly is a misconfiguration that we want to flag as an\n> error. Even if it came from a per-user configuration, if it was\n> specified in a conditionally included file, it is likely to be a\n> misconfiguration. If it came from a per-user configuration that\n> applies without any condition, it can be a good convenience feature\n> to silently (or with a warning) ignore missing file.\n"},{"id":"432266","messageId":"xmqqtuk0h4ph.fsf@gitster.g","threadId":"56234","inReplyTo":"CADm0i3-ToKo1gNTXXLHH6i2d4qpz771VeRjDsfJjgbgMfhx6rA@mail.gmail.com","subject":"Re: [PATCH 0/1] blame: Skip missing ignore-revs file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-08T05:43:06Z","receivedAt":"2021-08-08T05:43:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Noah Pendleton writes:\n\n> Very good point about no longer catching misconfiguration. For\n> detecting provenance of a setting, I think we'd need to tag the config\n> options with it when they're loaded, possibly in 'struct\n> config_set_element' or similar. What do you think about instead\n> emitting a warning message on stderr in the case of misconfiguration,\n> but still continuing? Eg:\n\nUnconditionally continuing with just a warning would not be a good\napproach for at least two reasons. (1) the user may truly have\nintended that ignoreRevsFile to be optional, in which case the\nwarning is a nuisance that does not add any value, and (2) it truly\nmay be a misconfiguration that the named file did not exist, but the\noutput from \"git blame\" will wipe the display and the warning would\nvery well go unnoticed (or more likely the user may notice that\nthere was a warning, but it will go away before the user has a\nchance to really read it, which is a lot worse and frustrating\nexperience).\n\nI think an easier way out is to introduce a new configuration\nvariable blame.ignoreRevsFileIsOptional which takes a boolean value,\nand when it is set to true, silently ignore when the named file does\nnot exist without any warning. When the variable is set to false\n(or the variable does not exist), we can keep the current behaviour\nof noticing a misconfigured blame.ignoreRevsFile and error out.\n\nThat way, the current users who rely on the typo detection feature\ncan keep relying on it, and those who want to make it optional can\ndo so without getting annoyed by a warning.\n"},{"id":"432270","messageId":"20210808174847.16590-1-noah.pendleton@gmail.com","threadId":"56234","inReplyTo":"20210807202752.1278672-1-noah.pendleton@gmail.com","subject":"[PATCH v2] blame: add config `blame.ignoreRevsFileIsOptional`","fromName":"Noah Pendleton","fromEmail":"noah.pendleton@gmail.com","sentAt":"2021-08-08T17:48:47Z","receivedAt":"2021-08-08T17:49:28Z","isPatch":true,"sender":{"key":"noah.pendleton@gmail.com","avatar":"https://gravatar.com/avatar/254bdb2e93ea199f3615ec3880e1a1cee7664e079a0dce9bec08a38ce7e882ba?d=mp&s=160"},"body":"Setting the config option `blame.ignoreRevsFile` globally to eg\n`.git-blame-ignore-revs` causes `git blame` to error when the file\ndoesn't exist in the current repository:\n\n```\nfatal: could not open object name list: .git-blame-ignore-revs\n```\n\nAdd a new config option, `blame.ignoreRevsFileIsOptional`, that when set\nto true, `git blame` will silently ignore any missing ignoreRevsFile's.\n\nSigned-off-by: Noah Pendleton \n---\nReworked this patch to add a new config\n`blame.ignoreRevsFileIsOptional`, which controls whether missing\nfiles specified by ignoreRevsFile cause an error or are silently\nignored.\n\nUpdated tests and docs to match.\n\n Documentation/blame-options.txt | 3 ++-\n Documentation/config/blame.txt | 5 +++++\n builtin/blame.c | 7 ++++++-\n t/t8013-blame-ignore-revs.sh | 14 ++++++++++----\n 4 files changed, 23 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt\nindex 117f4cf806..199a28ab79 100644\n--- a/Documentation/blame-options.txt\n+++ b/Documentation/blame-options.txt\n@@ -134,7 +134,8 @@ take effect.\n \t`fsck.skipList`. This option may be repeated, and these files will be\n \tprocessed after any files specified with the `blame.ignoreRevsFile` config\n \toption. An empty file name, `\"\"`, will clear the list of revs from\n-\tpreviously processed files.\n+\tpreviously processed files. If `blame.ignoreRevsFileIsOptional` is true,\n+\tmissing files will be silently ignored.\n \n -h::\n \tShow help message.\ndiff --git a/Documentation/config/blame.txt b/Documentation/config/blame.txt\nindex 4d047c1790..2aae851e4b 100644\n--- a/Documentation/config/blame.txt\n+++ b/Documentation/config/blame.txt\n@@ -27,6 +27,11 @@ blame.ignoreRevsFile::\n \tfile names will reset the list of ignored revisions. This option will\n \tbe handled before the command line option `--ignore-revs-file`.\n \n+blame.ignoreRevsFileIsOptional::\n+\tSilently skip missing files specified by ignoreRevsFile or the command line\n+\toption `--ignore-revs-file`. If unset, or set to false, missing files will\n+\tcause a nonrecoverable error.\n+\n blame.markUnblamableLines::\n \tMark lines that were changed by an ignored revision that we could not\n \tattribute to another commit with a '*' in the output of\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 641523ff9a..df132b34ce 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -56,6 +56,7 @@ static int coloring_mode;\n static struct string_list ignore_revs_file_list = STRING_LIST_INIT_NODUP;\n static int mark_unblamable_lines;\n static int mark_ignored_lines;\n+static int ignorerevsfileisoptional;\n \n static struct date_mode blame_date_mode = { DATE_ISO8601 };\n static size_t blame_date_width;\n@@ -715,6 +716,9 @@ static int git_blame_config(const char *var, const char *value, void *cb)\n \t\tstring_list_insert(&ignore_revs_file_list, str);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"blame.ignorerevsfileisoptional\")) {\n+\t\tignorerevsfileisoptional = git_config_bool(var, value);\n+\t}\n \tif (!strcmp(var, \"blame.markunblamablelines\")) {\n \t\tmark_unblamable_lines = git_config_bool(var, value);\n \t\treturn 0;\n@@ -835,7 +839,8 @@ static void build_ignorelist(struct blame_scoreboard *sb,\n \tfor_each_string_list_item(i, ignore_revs_file_list) {\n \t\tif (!strcmp(i->string, \"\"))\n \t\t\toidset_clear(&sb->ignore_list);\n-\t\telse\n+\t\t/* skip non-existent files if ignorerevsfileisoptional is set */\n+\t\telse if (!ignorerevsfileisoptional || file_exists(i->string))\n \t\t\toidset_parse_file_carefully(&sb->ignore_list, i->string,\n \t\t\t\t\t\t peel_to_commit_oid, sb);\n \t}\ndiff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh\nindex b18633dee1..f789426cbf 100755\n--- a/t/t8013-blame-ignore-revs.sh\n+++ b/t/t8013-blame-ignore-revs.sh\n@@ -127,18 +127,24 @@ test_expect_success override_ignore_revs_file '\n \tgrep -E \"^[0-9a-f]+ [0-9]+ 2\" blame_raw | sed -e \"s/ .*//\" >actual &&\n \ttest_cmp expect actual\n \t'\n-test_expect_success bad_files_and_revs '\n+test_expect_success bad_revs '\n \ttest_must_fail git blame file --ignore-rev NOREV 2>err &&\n \ttest_i18ngrep \"cannot find revision NOREV to ignore\" err &&\n \n-\ttest_must_fail git blame file --ignore-revs-file NOFILE 2>err &&\n-\ttest_i18ngrep \"could not open.*: NOFILE\" err &&\n-\n \techo NOREV >ignore_norev &&\n \ttest_must_fail git blame file --ignore-revs-file ignore_norev 2>err &&\n \ttest_i18ngrep \"invalid object name: NOREV\" err\n '\n \n+# Non-existent ignore-revs-file should fail unless\n+# blame.ignoreRevsFileIsOptional is set\n+test_expect_success bad_file '\n+\ttest_must_fail git blame file --ignore-revs-file NOFILE &&\n+\n+\tgit config --add blame.ignorerevsfileisoptional true &&\n+\tgit blame file --ignore-revs-file NOFILE\n+'\n+\n # For ignored revs that have added 'unblamable' lines, mark those lines with a\n # '*'\n # \tA--B--X--Y\n-- \n2.32.0\n\n"},{"id":"432271","messageId":"xmqqim0fhlm1.fsf@gitster.g","threadId":"56234","inReplyTo":"xmqqtuk0h4ph.fsf@gitster.g","subject":"Re: [PATCH 0/1] blame: Skip missing ignore-revs file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-08T17:50:14Z","receivedAt":"2021-08-08T17:50:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I think an easier way out is to introduce a new configuration\n> variable blame.ignoreRevsFileIsOptional which takes a boolean value,\n> and when it is set to true, silently ignore when the named file does\n> not exist without any warning. When the variable is set to false\n> (or the variable does not exist), we can keep the current behaviour\n> of noticing a misconfigured blame.ignoreRevsFile and error out.\n>\n> That way, the current users who rely on the typo detection feature\n> can keep relying on it, and those who want to make it optional can\n> do so without getting annoyed by a warning.\n\nA bit more ambitious might want to consider another more generally\napplicable avenue, which would help the userbase a lot more, before\ncontinuing.\n\nWe start from the realization that this is not the only\nconfiguration variable that specifies a filename that could be\nmissing. There may be other variables that name files to be used\n(\"git config --help\" would hopefully be the most comprehensive, but\n\"git grep -e git_config_pathname \\*.c\" would give us quicker\nstarting point to gauge how big an impact to the system we would be\ntalking about).\n\nWhat do the codepaths that use these variables do when they find\nthat the named files are missing? Do some of them die, some\nothers just warn, and yet some others silently ignore? Would such\nan inconsistency hurt our users?\n\nAmong the ones that die, are there ones that could reasonably\ncontinue as if the configuration variable weren't there and no file\nwas specified (i.e. similar to what you want blame.ignoreRevsFile to\ndo)? Among the ones that are silently ignored, are there ones that\nmay benefit by having a typo-detection? Do all of them benefit if\nthe behaviour upon missing files can be configurable by the end-user?\n\nDepending on the answers to the above questions, it might be that it\nis not a desirable approach to add \"blame.ignoreRevsFileIsOptional\"\nconfiguration variable, as all the existing configuration variables\nthat name files would want to add their own. We might be better off\ninventing a syntax for the value of blame.ignoreRevsFile (and other\nvariables that name files) to mark if the file is optional (i.e.\nsilently ignore if the named file does not exist) or required (i.e.\ndiagnose as a configuration error). For example, we may borrow from\nthe \"magic\" syntax for pathspecs that begin with \":(\", with comma\nseparated \"magic\" keywords and ends with \")\" and specify optional\npathname configuration like so:\n\n [blame] ignoreRevsFile = :(optional).gitignorerevs\n\nand teach the config parser to pretend as if it saw nothing when it\nnotices that the named file is missing. That approach would cover\nnot just this single variable, but other variables that are parsed\nusing git_config_pathname() may benefit the same way (of course, the\ncallsites for git_config_pathmame() must be inspected and adjusted\nfor this to happen).\n\nThanks.\n\n"},{"id":"432274","messageId":"CADm0i39LV91kochHSGVHovaTbDOd0COrQPXHD3x8rEj-1Y+eMA@mail.gmail.com","threadId":"56234","inReplyTo":"xmqqim0fhlm1.fsf@gitster.g","subject":"Re: [PATCH 0/1] blame: Skip missing ignore-revs file","fromName":"Noah Pendleton","fromEmail":"noah.pendleton@gmail.com","sentAt":"2021-08-08T18:21:44Z","receivedAt":"2021-08-08T18:21:57Z","isPatch":true,"sender":{"key":"noah.pendleton@gmail.com","avatar":"https://gravatar.com/avatar/254bdb2e93ea199f3615ec3880e1a1cee7664e079a0dce9bec08a38ce7e882ba?d=mp&s=160"},"body":"Very good point- I see about 21 call sites for `git_config_pathname`,\nplus a few others (`git_config_get_pathname`) that bottom out in the\nsame function. I could see the utility of optional paths for some of\nthem: for example, `commit.template`, `core.excludesfile`. Some of the\nothers seem a little more ambiguous, eg `http.sslcert` probably wants\nto always fail in case of missing file.\n\nThere seems to be a mix of fail-hard on invalid paths, printing a\nwarning message and skipping, and silently ignoring.\n\nHard for me to predict what the least confusing behavior is around\npath configuration values, though, so maybe adding support for the\n`:(optional)` (and maybe additionally a `:(required)`) tag across the\nboard to pathname configs is the right move.\n\nThat patch might be beyond what I'm capable of, though I'm happy to\nput up a draft that applies it to the original `ignoreRevsFile` case\nas a starting point.\n\nOn Sun, Aug 8, 2021 at 1:50 PM Junio C Hamano wrote:\n>\n> Junio C Hamano writes:\n>\n> > I think an easier way out is to introduce a new configuration\n> > variable blame.ignoreRevsFileIsOptional which takes a boolean value,\n> > and when it is set to true, silently ignore when the named file does\n> > not exist without any warning. When the variable is set to false\n> > (or the variable does not exist), we can keep the current behaviour\n> > of noticing a misconfigured blame.ignoreRevsFile and error out.\n> >\n> > That way, the current users who rely on the typo detection feature\n> > can keep relying on it, and those who want to make it optional can\n> > do so without getting annoyed by a warning.\n>\n> A bit more ambitious might want to consider another more generally\n> applicable avenue, which would help the userbase a lot more, before\n> continuing.\n>\n> We start from the realization that this is not the only\n> configuration variable that specifies a filename that could be\n> missing. There may be other variables that name files to be used\n> (\"git config --help\" would hopefully be the most comprehensive, but\n> \"git grep -e git_config_pathname \\*.c\" would give us quicker\n> starting point to gauge how big an impact to the system we would be\n> talking about).\n>\n> What do the codepaths that use these variables do when they find\n> that the named files are missing? Do some of them die, some\n> others just warn, and yet some others silently ignore? Would such\n> an inconsistency hurt our users?\n>\n> Among the ones that die, are there ones that could reasonably\n> continue as if the configuration variable weren't there and no file\n> was specified (i.e. similar to what you want blame.ignoreRevsFile to\n> do)? Among the ones that are silently ignored, are there ones that\n> may benefit by having a typo-detection? Do all of them benefit if\n> the behaviour upon missing files can be configurable by the end-user?\n>\n> Depending on the answers to the above questions, it might be that it\n> is not a desirable approach to add \"blame.ignoreRevsFileIsOptional\"\n> configuration variable, as all the existing configuration variables\n> that name files would want to add their own. We might be better off\n> inventing a syntax for the value of blame.ignoreRevsFile (and other\n> variables that name files) to mark if the file is optional (i.e.\n> silently ignore if the named file does not exist) or required (i.e.\n> diagnose as a configuration error). For example, we may borrow from\n> the \"magic\" syntax for pathspecs that begin with \":(\", with comma\n> separated \"magic\" keywords and ends with \")\" and specify optional\n> pathname configuration like so:\n>\n> [blame] ignoreRevsFile = :(optional).gitignorerevs\n>\n> and teach the config parser to pretend as if it saw nothing when it\n> notices that the named file is missing. That approach would cover\n> not just this single variable, but other variables that are parsed\n> using git_config_pathname() may benefit the same way (of course, the\n> callsites for git_config_pathmame() must be inspected and adjusted\n> for this to happen).\n>\n> Thanks.\n>\n"},{"id":"432308","messageId":"xmqq5ywehb69.fsf@gitster.g","threadId":"56234","inReplyTo":"CADm0i39LV91kochHSGVHovaTbDOd0COrQPXHD3x8rEj-1Y+eMA@mail.gmail.com","subject":"Re: [PATCH 0/1] blame: Skip missing ignore-revs file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-09T15:47:58Z","receivedAt":"2021-08-09T15:48:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Noah Pendleton writes:\n\n> Very good point- I see about 21 call sites for `git_config_pathname`,\n> plus a few others (`git_config_get_pathname`) that bottom out in the\n> same function. I could see the utility of optional paths for some of\n> them: for example, `commit.template`, `core.excludesfile`. Some of the\n> others seem a little more ambiguous, eg `http.sslcert` probably wants\n> to always fail in case of missing file.\n\nThanks for already doing initial surveillance. Very useful.\n\n> There seems to be a mix of fail-hard on invalid paths, printing a\n> warning message and skipping, and silently ignoring.\n>\n> Hard for me to predict what the least confusing behavior is around\n> path configuration values, though, so maybe adding support for the\n> `:(optional)` (and maybe additionally a `:(required)`) tag across the\n> board to pathname configs is the right move.\n\nI originally hoped only \":(optional)\" would be necessary, but to\nkeep the continuity in behaviour for those currently that do not die\nupon seeing a missing file, we probably should treat an unadorned\nvalue as asking for the \"traditional\" behaviour, at least in the\nshorter term, and allow those users who want to detect typos to\ntighten the rule using \":(required)\". I dunno.\n\n> That patch might be beyond what I'm capable of, though I'm happy to\n> put up a draft that applies it to the original `ignoreRevsFile` case\n> as a starting point.\n\nThanks for an offer. We are not in a hurry (especially during the\npre-release feature freeze), and hopefully this discussion would\npique other developers' interest to nudge them to help ;-)\n"},{"id":"450366","messageId":"a30ebbe3-596e-84a5-9023-b53402dfe70c@gmail.com","threadId":"56234","inReplyTo":"xmqqr1f5hszw.fsf@gitster.g","subject":"Re: [PATCH 0/1] blame: Skip missing ignore-revs file","fromName":"Thranur Andul","fromEmail":"thranur@gmail.com","sentAt":"2022-03-04T09:51:43Z","receivedAt":"2022-03-04T09:51:46Z","isPatch":true,"sender":{"key":"thranur@gmail.com","avatar":null},"body":"\n\nOn 07/08/2021 22:58, Junio C Hamano wrote:\n> Noah Pendleton writes:\n> \n> \n> That cuts both ways, though. Failing upon missing configuration\n> file is a way to catch misconfiguration that is hard to diagnose.\n> \n> I wonder if we can easily learn where the configuration variable\n> came from in the codepath that diagnoses it as a misconfiguration.\n> \n> If it came from a per-repo configuration and names a non-existent\n> file, it clearly is a misconfiguration that we want to flag as an\n> error. Even if it came from a per-user configuration, if it was\n> specified in a conditionally included file, it is likely to be a\n> misconfiguration. If it came from a per-user configuration that\n> applies without any condition, it can be a good convenience feature\n> to silently (or with a warning) ignore missing file.\n>\nI am very interested in this feature, but I'd like to add another point \nto the discussion: in the case of ignoreRevsFile in particular, no one \ncreates a repository with such a file; it is always added later. \nHowever, when bisecting (a typical usage scenario for git-blame), we may \nend up returning back to a point _before_ the file had been added, and \nthen, git-blame fails. This often happens to me, and I am then forced to \n`touch` the file to create it again, only to ensure git-blame keeps \nworking. And then, when I want to return to the HEAD commit, the file \nmust be erased again otherwise there is a conflict. So, for me, the \n\"ignore if absent\" behavior seems to me like it should be the default.\n"},{"id":"505053","messageId":"20241014204427.1712182-1-gitster@pobox.com","threadId":"56234","inReplyTo":"xmqq5ywehb69.fsf@gitster.g","subject":"[PATCH 0/3] specifying a file that can optionally exist","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-14T20:44:24Z","receivedAt":"2024-10-14T20:44:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"In a discussion a few years ago (cf. ),\nwe wondered if it is a good idea to allow a configuration variable\n(or a command line option for that matter) to name an \"optional\"\nfile, and pretend as if such a configuration setting or a command\nline option was not even given when the named file did not exist or\nempty.\n\nHere are a few patches I did while passing time without anything\nbetter to do.\n\nEven though I updated the documentation for the configuration\nvariables, I didn't find a good central place to do the same for\nparse-options. I'll leave it as an exercise for the readers ;-).\n\nThe first patch is a preliminary clean-up for test script that is\nused to house tests added by the later patches.\n\nThe second patch is for configuration variables, and the last one is\nfor command line options.\n\nJunio C Hamano (3):\n t7500: make each piece more independent\n config: values of pathname type can be prefixed with :(optional)\n parseopt: values of pathname type can be prefixed with :(optional)\n\n Documentation/config.txt | 5 +++-\n config.c | 16 +++++++++--\n parse-options.c | 31 +++++++++++++-------\n t/t7500-commit-template-squash-signoff.sh | 35 +++++++++++++++++------\n 4 files changed, 65 insertions(+), 22 deletions(-)\n\n-- \n2.47.0-148-g19c85929c5\n\n"},{"id":"505054","messageId":"20241014204427.1712182-2-gitster@pobox.com","threadId":"56234","inReplyTo":"20241014204427.1712182-1-gitster@pobox.com","subject":"[PATCH 1/3] t7500: make each piece more independent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-14T20:44:25Z","receivedAt":"2024-10-14T20:44:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"These tests prepare the working tree & index state to have something\nto be committed, and try a sequence of \"test_must_fail git commit\".\nIf an earlier one did not fail by a bug, a later one will fail for\na wrong reason (namely, \"nothing to commit\").\n\nGive them \"--allow-empty\" to make sure that they would work even\nwhen there is nothing to commit by accident.\n\nSigned-off-by: Junio C Hamano \n---\n t/t7500-commit-template-squash-signoff.sh | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\nindex 4dca8d97a7..4927b7260d 100755\n--- a/t/t7500-commit-template-squash-signoff.sh\n+++ b/t/t7500-commit-template-squash-signoff.sh\n@@ -50,33 +50,33 @@ test_expect_success 'nonexistent template file in config should return error' '\n TEMPLATE=\"$PWD\"/template\n \n test_expect_success 'unedited template should not commit' '\n-\techo \"template line\" > \"$TEMPLATE\" &&\n-\ttest_must_fail git commit --template \"$TEMPLATE\"\n+\techo \"template line\" >\"$TEMPLATE\" &&\n+\ttest_must_fail git commit --allow-empty --template \"$TEMPLATE\"\n '\n \n test_expect_success 'unedited template with comments should not commit' '\n-\techo \"# comment in template\" >> \"$TEMPLATE\" &&\n-\ttest_must_fail git commit --template \"$TEMPLATE\"\n+\techo \"# comment in template\" >>\"$TEMPLATE\" &&\n+\ttest_must_fail git commit --allow-empty --template \"$TEMPLATE\"\n '\n \n test_expect_success 'a Signed-off-by line by itself should not commit' '\n \t(\n \t\ttest_set_editor \"$TEST_DIRECTORY\"/t7500/add-signed-off &&\n-\t\ttest_must_fail git commit --template \"$TEMPLATE\"\n+\t\ttest_must_fail git commit --allow-empty --template \"$TEMPLATE\"\n \t)\n '\n \n test_expect_success 'adding comments to a template should not commit' '\n \t(\n \t\ttest_set_editor \"$TEST_DIRECTORY\"/t7500/add-comments &&\n-\t\ttest_must_fail git commit --template \"$TEMPLATE\"\n+\t\ttest_must_fail git commit --allow-empty --template \"$TEMPLATE\"\n \t)\n '\n \n test_expect_success 'adding real content to a template should commit' '\n \t(\n \t\ttest_set_editor \"$TEST_DIRECTORY\"/t7500/add-content &&\n-\t\tgit commit --template \"$TEMPLATE\"\n+\t\tgit commit --allow-empty --template \"$TEMPLATE\"\n \t) &&\n \tcommit_msg_is \"template linecommit message\"\n '\n-- \n2.47.0-148-g19c85929c5\n\n"},{"id":"505055","messageId":"20241014204427.1712182-3-gitster@pobox.com","threadId":"56234","inReplyTo":"20241014204427.1712182-1-gitster@pobox.com","subject":"[PATCH 2/3] config: values of pathname type can be prefixed with :(optional)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-14T20:44:26Z","receivedAt":"2024-10-14T20:44:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sometimes people want to specify additional configuration data\nas \"best effort\" basis. Maybe commit.template configuration file points\nat somewhere in ~/template/ but on a particular system, the file may not\nexist and the user may be OK without using the template in such a case.\n\nWhen the value given to a configuration variable whose type is\npathname wants to signal such an optional file, it can be marked by\nprepending \":(optional)\" in front of it. Such a setting that is\nmarked optional would avoid getting the command barf for a missing\nfile, as an optional configuration setting that names a missing or\nan empty file is not even seen.\n\ncf. \n\nSigned-off-by: Junio C Hamano \n---\n Documentation/config.txt | 5 ++++-\n config.c | 16 ++++++++++++++--\n t/t7500-commit-template-squash-signoff.sh | 9 +++++++++\n 3 files changed, 27 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 8c0b3ed807..199e29ccea 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -358,7 +358,10 @@ compiled without runtime prefix support, the compiled-in prefix will be\n substituted instead. In the unlikely event that a literal path needs to\n be specified that should _not_ be expanded, it needs to be prefixed by\n `./`, like so: `./%(prefix)/bin`.\n-\n++\n+If prefixed with `:(optional)`, the configuration variable is treated\n+as if it does not exist, if the named path does not exist or names an\n+empty file.\n \n Variables\n ~~~~~~~~~\ndiff --git a/config.c b/config.c\nindex a11bb85da3..4a060f1d82 100644\n--- a/config.c\n+++ b/config.c\n@@ -1364,11 +1364,23 @@ int git_config_string(char **dest, const char *var, const char *value)\n \n int git_config_pathname(char **dest, const char *var, const char *value)\n {\n+\tint is_optional;\n+\tchar *path;\n+\n \tif (!value)\n \t\treturn config_error_nonbool(var);\n-\t*dest = interpolate_path(value, 0);\n-\tif (!*dest)\n+\n+\tis_optional = skip_prefix(value, \":(optional)\", &value);\n+\tpath = interpolate_path(value, 0);\n+\tif (!path)\n \t\tdie(_(\"failed to expand user dir in: '%s'\"), value);\n+\n+\tif (is_optional && is_empty_or_missing_file(path)) {\n+\t\tfree(path);\n+\t\treturn 0;\n+\t}\n+\n+\t*dest = path;\n \treturn 0;\n }\n \ndiff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\nindex 4927b7260d..e28a79987d 100755\n--- a/t/t7500-commit-template-squash-signoff.sh\n+++ b/t/t7500-commit-template-squash-signoff.sh\n@@ -46,6 +46,15 @@ test_expect_success 'nonexistent template file in config should return error' '\n \t)\n '\n \n+test_expect_success 'nonexistent optional template file in config' '\n+\ttest_config commit.template \":(optional)$PWD\"/notexist &&\n+\t(\n+\t\tGIT_EDITOR=\"echo hello >\\\"\\$1\\\"\" &&\n+\t\texport GIT_EDITOR &&\n+\t\tgit commit --allow-empty\n+\t)\n+'\n+\n # From now on we'll use a template file that exists.\n TEMPLATE=\"$PWD\"/template\n \n-- \n2.47.0-148-g19c85929c5\n\n"},{"id":"505056","messageId":"20241014204427.1712182-4-gitster@pobox.com","threadId":"56234","inReplyTo":"20241014204427.1712182-1-gitster@pobox.com","subject":"[PATCH 3/3] parseopt: values of pathname type can be prefixed with :(optional)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-14T20:44:27Z","receivedAt":"2024-10-14T20:44:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"In the previous step, we introduced an optional filename that can be\ngiven to a configuration variable, and nullify the fact that such a\nconfiguration setting even existed if the named path is missing or\nempty.\n\nLet's do the same for command line options that name a pathname.\n\nSigned-off-by: Junio C Hamano \n---\n parse-options.c | 31 +++++++++++++++--------\n t/t7500-commit-template-squash-signoff.sh | 12 ++++++++-\n 2 files changed, 31 insertions(+), 12 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 33bfba0ed4..7a2a3b1f08 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -75,7 +75,6 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n {\n \tconst char *s, *arg;\n \tconst int unset = flags & OPT_UNSET;\n-\tint err;\n \n \tif (unset && p->opt)\n \t\treturn error(_(\"%s takes no value\"), optname(opt, flags));\n@@ -131,21 +130,31 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \tcase OPTION_FILENAME:\n \t{\n \t\tconst char *value;\n-\n-\t\tFREE_AND_NULL(*(char **)opt->value);\n-\n-\t\terr = 0;\n+\t\tint is_optional;\n \n \t\tif (unset)\n \t\t\tvalue = NULL;\n \t\telse if (opt->flags & PARSE_OPT_OPTARG && !p->opt)\n-\t\t\tvalue = (const char *) opt->defval;\n-\t\telse\n-\t\t\terr = get_arg(p, opt, flags, &value);\n+\t\t\tvalue = (char *)opt->defval;\n+\t\telse {\n+\t\t\tint err = get_arg(p, opt, flags, &value);\n+\t\t\tif (err)\n+\t\t\t\treturn err;\n+\t\t}\n+\t\tif (!value)\n+\t\t\treturn 0;\n \n-\t\tif (!err)\n-\t\t\t*(char **)opt->value = fix_filename(p->prefix, value);\n-\t\treturn err;\n+\t\tis_optional = skip_prefix(value, \":(optional)\", &value);\n+\t\tif (!value)\n+\t\t\tis_optional = 0;\n+\t\tvalue = fix_filename(p->prefix, value);\n+\t\tif (is_optional && is_empty_or_missing_file(value)) {\n+\t\t\tfree((char *)value);\n+\t\t} else {\n+\t\t\tFREE_AND_NULL(*(char **)opt->value);\n+\t\t\t*(const char **)opt->value = value;\n+\t\t}\n+\t\treturn 0;\n \t}\n \tcase OPTION_CALLBACK:\n \t{\ndiff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\nindex e28a79987d..c065f12baf 100755\n--- a/t/t7500-commit-template-squash-signoff.sh\n+++ b/t/t7500-commit-template-squash-signoff.sh\n@@ -37,12 +37,22 @@ test_expect_success 'nonexistent template file should return error' '\n \t)\n '\n \n+test_expect_success 'nonexistent optional template file on command line' '\n+\techo changes >> foo &&\n+\tgit add foo &&\n+\t(\n+\t\tGIT_EDITOR=\"echo hello >\\\"\\$1\\\"\" &&\n+\t\texport GIT_EDITOR &&\n+\t\tgit commit --template \":(optional)$PWD/notexist\"\n+\t)\n+'\n+\n test_expect_success 'nonexistent template file in config should return error' '\n \ttest_config commit.template \"$PWD\"/notexist &&\n \t(\n \t\tGIT_EDITOR=\"echo hello >\\\"\\$1\\\"\" &&\n \t\texport GIT_EDITOR &&\n-\t\ttest_must_fail git commit\n+\t\ttest_must_fail git commit --allow-empty\n \t)\n '\n \n-- \n2.47.0-148-g19c85929c5\n\n"},{"id":"517069","messageId":"20250501214057.371711-1-gitster@pobox.com","threadId":"56234","inReplyTo":"xmqq5ywehb69.fsf@gitster.g","subject":"[PATCH 0/3] specifying a file that can optionally exist","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-01T21:40:54Z","receivedAt":"2025-05-01T21:40:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"In a discussion some years ago (cf. ),\nwe wondered if it is a good idea to allow a configuration variable\n(or a command line option for that matter) to name an \"optional\"\nfile, and pretend as if such a configuration setting or a command\nline option was not even given when the named file did not exist or\nempty. Then I floated a set of patches to implement the feature,\nbut the topic did not get any traction and was dropped.\n\nI am resurrecting the patches after seeing some interest in it in\nrecent discussion threads; it would be easier for people to\ncomment on, if they are in more recent parts of their mailbox.\nI didn't change anything in the patch; they are verbatim copies\nthat I happened to have found lying somewhere in my filesystem.\n\nEven though I updated the documentation for the configuration\nvariables, I didn't find a good central place to do the same for\nparse-options. I'll leave it as an exercise for the readers ;-).\n\nThe first patch is a preliminary clean-up for test script that is\nused to house tests added by the later patches.\n\nThe second patch is for configuration variables, and the last one is\nfor command line options.\n\nJunio C Hamano (3):\n t7500: make each piece more independent\n config: values of pathname type can be prefixed with :(optional)\n parseopt: values of pathname type can be prefixed with :(optional)\n\n Documentation/config.txt | 5 +++-\n config.c | 16 +++++++++--\n parse-options.c | 31 +++++++++++++-------\n t/t7500-commit-template-squash-signoff.sh | 35 +++++++++++++++++------\n 4 files changed, 65 insertions(+), 22 deletions(-)\n\n-- \n2.47.0-148-g19c85929c5\n\n"},{"id":"517070","messageId":"20250501214057.371711-2-gitster@pobox.com","threadId":"56234","inReplyTo":"20250501214057.371711-1-gitster@pobox.com","subject":"[PATCH 1/3] t7500: make each piece more independent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-01T21:40:55Z","receivedAt":"2025-05-01T21:41:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"These tests prepare the working tree & index state to have something\nto be committed, and try a sequence of \"test_must_fail git commit\".\nIf an earlier one did not fail by a bug, a later one will fail for\na wrong reason (namely, \"nothing to commit\").\n\nGive them \"--allow-empty\" to make sure that they would work even\nwhen there is nothing to commit by accident.\n\nSigned-off-by: Junio C Hamano \n---\n t/t7500-commit-template-squash-signoff.sh | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\nindex 4dca8d97a7..4927b7260d 100755\n--- a/t/t7500-commit-template-squash-signoff.sh\n+++ b/t/t7500-commit-template-squash-signoff.sh\n@@ -50,33 +50,33 @@ test_expect_success 'nonexistent template file in config should return error' '\n TEMPLATE=\"$PWD\"/template\n \n test_expect_success 'unedited template should not commit' '\n-\techo \"template line\" > \"$TEMPLATE\" &&\n-\ttest_must_fail git commit --template \"$TEMPLATE\"\n+\techo \"template line\" >\"$TEMPLATE\" &&\n+\ttest_must_fail git commit --allow-empty --template \"$TEMPLATE\"\n '\n \n test_expect_success 'unedited template with comments should not commit' '\n-\techo \"# comment in template\" >> \"$TEMPLATE\" &&\n-\ttest_must_fail git commit --template \"$TEMPLATE\"\n+\techo \"# comment in template\" >>\"$TEMPLATE\" &&\n+\ttest_must_fail git commit --allow-empty --template \"$TEMPLATE\"\n '\n \n test_expect_success 'a Signed-off-by line by itself should not commit' '\n \t(\n \t\ttest_set_editor \"$TEST_DIRECTORY\"/t7500/add-signed-off &&\n-\t\ttest_must_fail git commit --template \"$TEMPLATE\"\n+\t\ttest_must_fail git commit --allow-empty --template \"$TEMPLATE\"\n \t)\n '\n \n test_expect_success 'adding comments to a template should not commit' '\n \t(\n \t\ttest_set_editor \"$TEST_DIRECTORY\"/t7500/add-comments &&\n-\t\ttest_must_fail git commit --template \"$TEMPLATE\"\n+\t\ttest_must_fail git commit --allow-empty --template \"$TEMPLATE\"\n \t)\n '\n \n test_expect_success 'adding real content to a template should commit' '\n \t(\n \t\ttest_set_editor \"$TEST_DIRECTORY\"/t7500/add-content &&\n-\t\tgit commit --template \"$TEMPLATE\"\n+\t\tgit commit --allow-empty --template \"$TEMPLATE\"\n \t) &&\n \tcommit_msg_is \"template linecommit message\"\n '\n-- \n2.47.0-148-g19c85929c5\n\n"},{"id":"517071","messageId":"20250501214057.371711-3-gitster@pobox.com","threadId":"56234","inReplyTo":"20250501214057.371711-1-gitster@pobox.com","subject":"[PATCH 2/3] config: values of pathname type can be prefixed with :(optional)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-01T21:40:56Z","receivedAt":"2025-05-01T21:41:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sometimes people want to specify additional configuration data\nas \"best effort\" basis. Maybe commit.template configuration file points\nat somewhere in ~/template/ but on a particular system, the file may not\nexist and the user may be OK without using the template in such a case.\n\nWhen the value given to a configuration variable whose type is\npathname wants to signal such an optional file, it can be marked by\nprepending \":(optional)\" in front of it. Such a setting that is\nmarked optional would avoid getting the command barf for a missing\nfile, as an optional configuration setting that names a missing or\nan empty file is not even seen.\n\ncf. \n\nSigned-off-by: Junio C Hamano \n---\n Documentation/config.txt | 5 ++++-\n config.c | 16 ++++++++++++++--\n t/t7500-commit-template-squash-signoff.sh | 9 +++++++++\n 3 files changed, 27 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 8c0b3ed807..199e29ccea 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -358,7 +358,10 @@ compiled without runtime prefix support, the compiled-in prefix will be\n substituted instead. In the unlikely event that a literal path needs to\n be specified that should _not_ be expanded, it needs to be prefixed by\n `./`, like so: `./%(prefix)/bin`.\n-\n++\n+If prefixed with `:(optional)`, the configuration variable is treated\n+as if it does not exist, if the named path does not exist or names an\n+empty file.\n \n Variables\n ~~~~~~~~~\ndiff --git a/config.c b/config.c\nindex a11bb85da3..4a060f1d82 100644\n--- a/config.c\n+++ b/config.c\n@@ -1364,11 +1364,23 @@ int git_config_string(char **dest, const char *var, const char *value)\n \n int git_config_pathname(char **dest, const char *var, const char *value)\n {\n+\tint is_optional;\n+\tchar *path;\n+\n \tif (!value)\n \t\treturn config_error_nonbool(var);\n-\t*dest = interpolate_path(value, 0);\n-\tif (!*dest)\n+\n+\tis_optional = skip_prefix(value, \":(optional)\", &value);\n+\tpath = interpolate_path(value, 0);\n+\tif (!path)\n \t\tdie(_(\"failed to expand user dir in: '%s'\"), value);\n+\n+\tif (is_optional && is_empty_or_missing_file(path)) {\n+\t\tfree(path);\n+\t\treturn 0;\n+\t}\n+\n+\t*dest = path;\n \treturn 0;\n }\n \ndiff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\nindex 4927b7260d..e28a79987d 100755\n--- a/t/t7500-commit-template-squash-signoff.sh\n+++ b/t/t7500-commit-template-squash-signoff.sh\n@@ -46,6 +46,15 @@ test_expect_success 'nonexistent template file in config should return error' '\n \t)\n '\n \n+test_expect_success 'nonexistent optional template file in config' '\n+\ttest_config commit.template \":(optional)$PWD\"/notexist &&\n+\t(\n+\t\tGIT_EDITOR=\"echo hello >\\\"\\$1\\\"\" &&\n+\t\texport GIT_EDITOR &&\n+\t\tgit commit --allow-empty\n+\t)\n+'\n+\n # From now on we'll use a template file that exists.\n TEMPLATE=\"$PWD\"/template\n \n-- \n2.47.0-148-g19c85929c5\n\n"},{"id":"517072","messageId":"20250501214057.371711-4-gitster@pobox.com","threadId":"56234","inReplyTo":"20250501214057.371711-1-gitster@pobox.com","subject":"[PATCH 3/3] parseopt: values of pathname type can be prefixed with :(optional)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-01T21:40:57Z","receivedAt":"2025-05-01T21:41:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"In the previous step, we introduced an optional filename that can be\ngiven to a configuration variable, and nullify the fact that such a\nconfiguration setting even existed if the named path is missing or\nempty.\n\nLet's do the same for command line options that name a pathname.\n\nSigned-off-by: Junio C Hamano \n---\n parse-options.c | 31 +++++++++++++++--------\n t/t7500-commit-template-squash-signoff.sh | 12 ++++++++-\n 2 files changed, 31 insertions(+), 12 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 33bfba0ed4..7a2a3b1f08 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -75,7 +75,6 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n {\n \tconst char *s, *arg;\n \tconst int unset = flags & OPT_UNSET;\n-\tint err;\n \n \tif (unset && p->opt)\n \t\treturn error(_(\"%s takes no value\"), optname(opt, flags));\n@@ -131,21 +130,31 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \tcase OPTION_FILENAME:\n \t{\n \t\tconst char *value;\n-\n-\t\tFREE_AND_NULL(*(char **)opt->value);\n-\n-\t\terr = 0;\n+\t\tint is_optional;\n \n \t\tif (unset)\n \t\t\tvalue = NULL;\n \t\telse if (opt->flags & PARSE_OPT_OPTARG && !p->opt)\n-\t\t\tvalue = (const char *) opt->defval;\n-\t\telse\n-\t\t\terr = get_arg(p, opt, flags, &value);\n+\t\t\tvalue = (char *)opt->defval;\n+\t\telse {\n+\t\t\tint err = get_arg(p, opt, flags, &value);\n+\t\t\tif (err)\n+\t\t\t\treturn err;\n+\t\t}\n+\t\tif (!value)\n+\t\t\treturn 0;\n \n-\t\tif (!err)\n-\t\t\t*(char **)opt->value = fix_filename(p->prefix, value);\n-\t\treturn err;\n+\t\tis_optional = skip_prefix(value, \":(optional)\", &value);\n+\t\tif (!value)\n+\t\t\tis_optional = 0;\n+\t\tvalue = fix_filename(p->prefix, value);\n+\t\tif (is_optional && is_empty_or_missing_file(value)) {\n+\t\t\tfree((char *)value);\n+\t\t} else {\n+\t\t\tFREE_AND_NULL(*(char **)opt->value);\n+\t\t\t*(const char **)opt->value = value;\n+\t\t}\n+\t\treturn 0;\n \t}\n \tcase OPTION_CALLBACK:\n \t{\ndiff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\nindex e28a79987d..c065f12baf 100755\n--- a/t/t7500-commit-template-squash-signoff.sh\n+++ b/t/t7500-commit-template-squash-signoff.sh\n@@ -37,12 +37,22 @@ test_expect_success 'nonexistent template file should return error' '\n \t)\n '\n \n+test_expect_success 'nonexistent optional template file on command line' '\n+\techo changes >> foo &&\n+\tgit add foo &&\n+\t(\n+\t\tGIT_EDITOR=\"echo hello >\\\"\\$1\\\"\" &&\n+\t\texport GIT_EDITOR &&\n+\t\tgit commit --template \":(optional)$PWD/notexist\"\n+\t)\n+'\n+\n test_expect_success 'nonexistent template file in config should return error' '\n \ttest_config commit.template \"$PWD\"/notexist &&\n \t(\n \t\tGIT_EDITOR=\"echo hello >\\\"\\$1\\\"\" &&\n \t\texport GIT_EDITOR &&\n-\t\ttest_must_fail git commit\n+\t\ttest_must_fail git commit --allow-empty\n \t)\n '\n \n-- \n2.47.0-148-g19c85929c5\n\n"},{"id":"517097","messageId":"aBSHugZcH8NusOcI@pks.im","threadId":"56234","inReplyTo":"20250501214057.371711-3-gitster@pobox.com","subject":"Re: [PATCH 2/3] config: values of pathname type can be prefixed with :(optional)","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-02T08:52:10Z","receivedAt":"2025-05-02T08:52:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, May 01, 2025 at 02:40:56PM -0700, Junio C Hamano wrote:\n> Sometimes people want to specify additional configuration data\n> as \"best effort\" basis. Maybe commit.template configuration file points\n> at somewhere in ~/template/ but on a particular system, the file may not\n> exist and the user may be OK without using the template in such a case.\n> \n> When the value given to a configuration variable whose type is\n> pathname wants to signal such an optional file, it can be marked by\n> prepending \":(optional)\" in front of it. Such a setting that is\n> marked optional would avoid getting the command barf for a missing\n> file, as an optional configuration setting that names a missing or\n> an empty file is not even seen.\n> \n> cf. \n> \n> Signed-off-by: Junio C Hamano \n> ---\n> Documentation/config.txt | 5 ++++-\n> config.c | 16 ++++++++++++++--\n> t/t7500-commit-template-squash-signoff.sh | 9 +++++++++\n> 3 files changed, 27 insertions(+), 3 deletions(-)\n> \n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 8c0b3ed807..199e29ccea 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -358,7 +358,10 @@ compiled without runtime prefix support, the compiled-in prefix will be\n> substituted instead. In the unlikely event that a literal path needs to\n> be specified that should _not_ be expanded, it needs to be prefixed by\n> `./`, like so: `./%(prefix)/bin`.\n> -\n> ++\n> +If prefixed with `:(optional)`, the configuration variable is treated\n> +as if it does not exist, if the named path does not exist or names an\n> +empty file.\n\nI can see why it may be useful to allow for non-existent paths. But I\nwonder whether we really should be skipping over empty files, as well,\nas it may be assuming too much about the semantics of a given config\nkey. In other words, are we reasonably sure that there won't ever be a\nusecase where you may want to specify an optional and empty file? And\nare there any use cases where an empty file should be ignored?\n\nPatrick\n"},{"id":"517114","messageId":"c492a392-8914-4fa3-8356-c583f0a3fa81@gmail.com","threadId":"56234","inReplyTo":"aBSHugZcH8NusOcI@pks.im","subject":"Re: [PATCH 2/3] config: values of pathname type can be prefixed with :(optional)","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-05-02T14:28:05Z","receivedAt":"2025-05-02T14:28:25Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 02/05/2025 09:52, Patrick Steinhardt wrote:\n> On Thu, May 01, 2025 at 02:40:56PM -0700, Junio C Hamano wrote:\n> \n>> ++\n>> +If prefixed with `:(optional)`, the configuration variable is treated\n>> +as if it does not exist, if the named path does not exist or names an\n>> +empty file.\n> \n> I can see why it may be useful to allow for non-existent paths. But I\n> wonder whether we really should be skipping over empty files, as well,\n> as it may be assuming too much about the semantics of a given config\n> key. In other words, are we reasonably sure that there won't ever be a\n> usecase where you may want to specify an optional and empty file? And\n> are there any use cases where an empty file should be ignored?\n\nThat's my thought too - ignoring a missing file sounds like a good idea \nbut why an empty file too?\n\nBest Wishes\n\nPhillip\n\n"},{"id":"517129","messageId":"xmqq1pt6vkve.fsf@gitster.g","threadId":"56234","inReplyTo":"aBSHugZcH8NusOcI@pks.im","subject":"Re: [PATCH 2/3] config: values of pathname type can be prefixed with :(optional)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-02T20:05:41Z","receivedAt":"2025-05-02T20:05:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> I can see why it may be useful to allow for non-existent paths. But I\n> wonder whether we really should be skipping over empty files, as well,\n> as it may be assuming too much about the semantics of a given config\n> key. In other words, are we reasonably sure that there won't ever be a\n> usecase where you may want to specify an optional and empty file? And\n> are there any use cases where an empty file should be ignored?\n\nIf somebody goes back to the original discussion that happened a few\nyears before the patches were originally written, they might find a\nuse case where it is more convenient to ignore an empty file, but it\nis an old patch series, so I do not remember the details.\n\nI would not be surprised if the design decision for an empty blob\nwas done without any deep thought or motivationg use case. After\nall, this was \"I had nothing better to do, so wrote these out of\nboredom\" patchset, as its cover letter said.\n\nIf somebody wants to carry these patches forward (which I am hoping\nbecause there were a few people who expressed interest recently, and\nbecause I am not all that interested, certainly not more than those\nwho wanted to have this feature), I think that the right approach is\nto extend the system by taking advantage of the syntax that was\ndesigned to be extensible. In addition to \":(optional)\", we could\nadd different variants like \":(optional,ignore-empty)\" with desired\nsemantics, for example.\n\nThanks.\n"},{"id":"527519","messageId":"cover.1759094936.git.ben.knoble+github@gmail.com","threadId":"56234","inReplyTo":"20250501214057.371711-1-gitster@pobox.com","subject":"[PATCH v2 0/3] Support :(optional) filepaths","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-09-28T21:29:13Z","receivedAt":"2025-09-28T21:30:14Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Notes:\n- Based on commit 2da08f2c3d (parseopt: values of pathname type can be\n prefixed with :(optional), 2024-10-14) (broken-out/wip/optional-path)\n- Rebased on v2.51.0\n- I'm least sure of the 3rd patch and am happy to drop it in support of\n the first 2. I think it might be better to (a) integrate :(optional)\n support as pathspec magic and (b) use pathspec magic in parse-options\n when getting filenames. But I'm not sure, and this has other\n ramifications I'm not prepared to deal with. (For example: `git grep\n path … :(optional)non-existent` could pretend like\n `non-existent` was never given?)\n- The parsing is not exactly a \"clean API,\" but I wasn't sure how to\n make it cleaner :)\n\nChanges in v2:\n- Only check for missing files, not empty files\n- Move a test change to the appropriate commit\n- Document optional magic in options in gitcli(1)\n\nThis series adds support for optional filepaths in config and\nparse-options, which supports use-cases such as missing commit templates\nor blame.ignoreRevsFile values without erroring.\n\nv1: https://lore.kernel.org/git/20250501214057.371711-1-gitster@pobox.com/\n\nJunio C Hamano (3):\n t7500: make each piece more independent\n config: values of pathname type can be prefixed with :(optional)\n parseopt: values of pathname type can be prefixed with :(optional)\n\n Documentation/config.adoc | 4 ++-\n Documentation/gitcli.adoc | 14 +++++++++\n config.c | 16 +++++++++--\n parse-options.c | 31 +++++++++++++-------\n t/t7500-commit-template-squash-signoff.sh | 35 +++++++++++++++++------\n wrapper.c | 13 +++++++++\n wrapper.h | 4 ++-\n 7 files changed, 94 insertions(+), 23 deletions(-)\n\nDiff-intervalle contre v1 :\n1: 82d283c626 ! 1: 63b2b24d42 t7500: make each piece more independent\n @@ Commit message\n Signed-off-by: Taylor Blau \n \n ## t/t7500-commit-template-squash-signoff.sh ##\n +@@ t/t7500-commit-template-squash-signoff.sh: commit_msg_is ()\n + \t(\n + \t\tGIT_EDITOR=\"echo hello >\\\"\\$1\\\"\" &&\n + \t\texport GIT_EDITOR &&\n +-\t\ttest_must_fail git commit\n ++\t\ttest_must_fail git commit --allow-empty\n + \t)\n + '\n + \n @@ t/t7500-commit-template-squash-signoff.sh: commit_msg_is ()\n TEMPLATE=\"$PWD\"/template\n \n2: dbafaff13b ! 2: 5c97f580a9 config: values of pathname type can be prefixed with :(optional)\n @@ Commit message\n pathname wants to signal such an optional file, it can be marked by\n prepending \":(optional)\" in front of it. Such a setting that is\n marked optional would avoid getting the command barf for a missing\n - file, as an optional configuration setting that names a missing or\n - an empty file is not even seen.\n + file, as an optional configuration setting that names a missing\n + file is not even seen.\n \n cf. \n \n Signed-off-by: Junio C Hamano \n Signed-off-by: Taylor Blau \n \n - ## Documentation/config.txt ##\n -@@ Documentation/config.txt: compiled without runtime prefix support, the compiled-in prefix will be\n +\n + ## Notes ##\n + The 2nd paragraph in this commit is wrapped strangely\n +\n + I've kept the strange wrapping length for now, but can reflow it if\n + desired.\n +\n + ## Documentation/config.adoc ##\n +@@ Documentation/config.adoc: compiled without runtime prefix support, the compiled-in prefix will be\n substituted instead. In the unlikely event that a literal path needs to\n be specified that should _not_ be expanded, it needs to be prefixed by\n `./`, like so: `./%(prefix)/bin`.\n -\n ++\n +If prefixed with `:(optional)`, the configuration variable is treated\n -+as if it does not exist, if the named path does not exist or names an\n -+empty file.\n ++as if it does not exist, if the named path does not exist.\n \n Variables\n ~~~~~~~~~\n @@ config.c: int git_config_string(char **dest, const char *var, const char *value)\n +\tif (!path)\n \t\tdie(_(\"failed to expand user dir in: '%s'\"), value);\n +\n -+\tif (is_optional && is_empty_or_missing_file(path)) {\n ++\tif (is_optional && is_missing_file(path)) {\n +\t\tfree(path);\n +\t\treturn 0;\n +\t}\n @@ t/t7500-commit-template-squash-signoff.sh: commit_msg_is ()\n # From now on we'll use a template file that exists.\n TEMPLATE=\"$PWD\"/template\n \n +\n + ## wrapper.c ##\n +@@ wrapper.c: int xgethostname(char *buf, size_t len)\n + \treturn ret;\n + }\n + \n ++int is_missing_file(const char *filename)\n ++{\n ++\tstruct stat st;\n ++\n ++\tif (stat(filename, &st) < 0) {\n ++\t\tif (errno == ENOENT)\n ++\t\t\treturn 1;\n ++\t\tdie_errno(_(\"could not stat %s\"), filename);\n ++\t}\n ++\n ++\treturn 0;\n ++}\n ++\n + int is_empty_or_missing_file(const char *filename)\n + {\n + \tstruct stat st;\n +\n + ## wrapper.h ##\n +@@ wrapper.h: void write_file_buf(const char *path, const char *buf, size_t len);\n + __attribute__((format (printf, 2, 3)))\n + void write_file(const char *path, const char *fmt, ...);\n + \n +-/* Return 1 if the file is empty or does not exists, 0 otherwise. */\n ++/* Return 1 if the file does not exist, 0 otherwise. */\n ++int is_missing_file(const char *filename);\n ++/* Return 1 if the file is empty or does not exist, 0 otherwise. */\n + int is_empty_or_missing_file(const char *filename);\n + \n + enum fsync_action {\n3: 2da08f2c3d ! 3: 5f7057c236 parseopt: values of pathname type can be prefixed with :(optional)\n @@ Commit message\n Signed-off-by: Junio C Hamano \n Signed-off-by: Taylor Blau \n \n + ## Documentation/gitcli.adoc ##\n +@@ Documentation/gitcli.adoc: $ git describe --abbrev=10 HEAD # correct\n + $ git describe --abbrev 10 HEAD # NOT WHAT YOU MEANT\n + ----------------------------\n + \n ++\n ++Magic filename options\n ++~~~~~~~~~~~~~~~~~~~~~~\n ++Options that take a filename allow a prefix `:(optional)`. For example:\n ++\n ++----------------------------\n ++git commit -F :(optional)COMMIT_EDITMSG\n ++# if COMMIT_EDITMSG does not exist, equivalent to\n ++git commit\n ++----------------------------\n ++\n ++Like with configuration values, if the named file is missing Git behaves as if\n ++the option was not given at all. See \"Values\" in linkgit:git-config[1].\n ++\n + NOTES ON FREQUENTLY CONFUSED OPTIONS\n + ------------------------------------\n + \n +\n ## parse-options.c ##\n @@ parse-options.c: static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n {\n - \tconst char *s, *arg;\n + \tconst char *arg;\n \tconst int unset = flags & OPT_UNSET;\n -\tint err;\n \n @@ t/t7500-commit-template-squash-signoff.sh: commit_msg_is ()\n test_expect_success 'nonexistent template file in config should return error' '\n \ttest_config commit.template \"$PWD\"/notexist &&\n \t(\n - \t\tGIT_EDITOR=\"echo hello >\\\"\\$1\\\"\" &&\n - \t\texport GIT_EDITOR &&\n --\t\ttest_must_fail git commit\n -+\t\ttest_must_fail git commit --allow-empty\n - \t)\n - '\n - \n\nbase-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0\n-- \n2.48.1\n\n"},{"id":"527520","messageId":"63b2b24d42906162f2415da37ccc75c921518b7a.1759094936.git.ben.knoble+github@gmail.com","threadId":"56234","inReplyTo":"cover.1759094936.git.ben.knoble+github@gmail.com","subject":"[PATCH v2 1/3] t7500: make each piece more independent","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-09-28T21:29:14Z","receivedAt":"2025-09-28T21:30:15Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"From: Junio C Hamano \n\nThese tests prepare the working tree & index state to have something\nto be committed, and try a sequence of \"test_must_fail git commit\".\nIf an earlier one did not fail by a bug, a later one will fail for\na wrong reason (namely, \"nothing to commit\").\n\nGive them \"--allow-empty\" to make sure that they would work even\nwhen there is nothing to commit by accident.\n\nSigned-off-by: Junio C Hamano \nSigned-off-by: Taylor Blau \nSigned-off-by: D. Ben Knoble \n---\n t/t7500-commit-template-squash-signoff.sh | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\nindex 4dca8d97a7..05cda50186 100755\n--- a/t/t7500-commit-template-squash-signoff.sh\n+++ b/t/t7500-commit-template-squash-signoff.sh\n@@ -42,7 +42,7 @@ commit_msg_is ()\n \t(\n \t\tGIT_EDITOR=\"echo hello >\\\"\\$1\\\"\" &&\n \t\texport GIT_EDITOR &&\n-\t\ttest_must_fail git commit\n+\t\ttest_must_fail git commit --allow-empty\n \t)\n '\n \n@@ -50,33 +50,33 @@ commit_msg_is ()\n TEMPLATE=\"$PWD\"/template\n \n test_expect_success 'unedited template should not commit' '\n-\techo \"template line\" > \"$TEMPLATE\" &&\n-\ttest_must_fail git commit --template \"$TEMPLATE\"\n+\techo \"template line\" >\"$TEMPLATE\" &&\n+\ttest_must_fail git commit --allow-empty --template \"$TEMPLATE\"\n '\n \n test_expect_success 'unedited template with comments should not commit' '\n-\techo \"# comment in template\" >> \"$TEMPLATE\" &&\n-\ttest_must_fail git commit --template \"$TEMPLATE\"\n+\techo \"# comment in template\" >>\"$TEMPLATE\" &&\n+\ttest_must_fail git commit --allow-empty --template \"$TEMPLATE\"\n '\n \n test_expect_success 'a Signed-off-by line by itself should not commit' '\n \t(\n \t\ttest_set_editor \"$TEST_DIRECTORY\"/t7500/add-signed-off &&\n-\t\ttest_must_fail git commit --template \"$TEMPLATE\"\n+\t\ttest_must_fail git commit --allow-empty --template \"$TEMPLATE\"\n \t)\n '\n \n test_expect_success 'adding comments to a template should not commit' '\n \t(\n \t\ttest_set_editor \"$TEST_DIRECTORY\"/t7500/add-comments &&\n-\t\ttest_must_fail git commit --template \"$TEMPLATE\"\n+\t\ttest_must_fail git commit --allow-empty --template \"$TEMPLATE\"\n \t)\n '\n \n test_expect_success 'adding real content to a template should commit' '\n \t(\n \t\ttest_set_editor \"$TEST_DIRECTORY\"/t7500/add-content &&\n-\t\tgit commit --template \"$TEMPLATE\"\n+\t\tgit commit --allow-empty --template \"$TEMPLATE\"\n \t) &&\n \tcommit_msg_is \"template linecommit message\"\n '\n-- \n2.48.1\n\n"},{"id":"527521","messageId":"5c97f580a9e77c464bc6bf4ed9ea8546711c6637.1759094936.git.ben.knoble+github@gmail.com","threadId":"56234","inReplyTo":"cover.1759094936.git.ben.knoble+github@gmail.com","subject":"[PATCH v2 2/3] config: values of pathname type can be prefixed with :(optional)","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-09-28T21:29:15Z","receivedAt":"2025-09-28T21:30:17Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"From: Junio C Hamano \n\nSometimes people want to specify additional configuration data\nas \"best effort\" basis. Maybe commit.template configuration file points\nat somewhere in ~/template/ but on a particular system, the file may not\nexist and the user may be OK without using the template in such a case.\n\nWhen the value given to a configuration variable whose type is\npathname wants to signal such an optional file, it can be marked by\nprepending \":(optional)\" in front of it. Such a setting that is\nmarked optional would avoid getting the command barf for a missing\nfile, as an optional configuration setting that names a missing\nfile is not even seen.\n\ncf. \n\nSigned-off-by: Junio C Hamano \nSigned-off-by: Taylor Blau \nSigned-off-by: D. Ben Knoble \n---\n\nNotes:\n The 2nd paragraph in this commit is wrapped strangely\n \n I've kept the strange wrapping length for now, but can reflow it if\n desired.\n\n Documentation/config.adoc | 4 +++-\n config.c | 16 ++++++++++++++--\n t/t7500-commit-template-squash-signoff.sh | 9 +++++++++\n wrapper.c | 13 +++++++++++++\n wrapper.h | 4 +++-\n 5 files changed, 42 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config.adoc b/Documentation/config.adoc\nindex cc769251be..7301ced836 100644\n--- a/Documentation/config.adoc\n+++ b/Documentation/config.adoc\n@@ -358,7 +358,9 @@ compiled without runtime prefix support, the compiled-in prefix will be\n substituted instead. In the unlikely event that a literal path needs to\n be specified that should _not_ be expanded, it needs to be prefixed by\n `./`, like so: `./%(prefix)/bin`.\n-\n++\n+If prefixed with `:(optional)`, the configuration variable is treated\n+as if it does not exist, if the named path does not exist.\n \n Variables\n ~~~~~~~~~\ndiff --git a/config.c b/config.c\nindex 97ffef4270..73fc74c8fa 100644\n--- a/config.c\n+++ b/config.c\n@@ -1279,11 +1279,23 @@ int git_config_string(char **dest, const char *var, const char *value)\n \n int git_config_pathname(char **dest, const char *var, const char *value)\n {\n+\tint is_optional;\n+\tchar *path;\n+\n \tif (!value)\n \t\treturn config_error_nonbool(var);\n-\t*dest = interpolate_path(value, 0);\n-\tif (!*dest)\n+\n+\tis_optional = skip_prefix(value, \":(optional)\", &value);\n+\tpath = interpolate_path(value, 0);\n+\tif (!path)\n \t\tdie(_(\"failed to expand user dir in: '%s'\"), value);\n+\n+\tif (is_optional && is_missing_file(path)) {\n+\t\tfree(path);\n+\t\treturn 0;\n+\t}\n+\n+\t*dest = path;\n \treturn 0;\n }\n \ndiff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\nindex 05cda50186..366f7f23b3 100755\n--- a/t/t7500-commit-template-squash-signoff.sh\n+++ b/t/t7500-commit-template-squash-signoff.sh\n@@ -46,6 +46,15 @@ commit_msg_is ()\n \t)\n '\n \n+test_expect_success 'nonexistent optional template file in config' '\n+\ttest_config commit.template \":(optional)$PWD\"/notexist &&\n+\t(\n+\t\tGIT_EDITOR=\"echo hello >\\\"\\$1\\\"\" &&\n+\t\texport GIT_EDITOR &&\n+\t\tgit commit --allow-empty\n+\t)\n+'\n+\n # From now on we'll use a template file that exists.\n TEMPLATE=\"$PWD\"/template\n \ndiff --git a/wrapper.c b/wrapper.c\nindex 2f00d2ac87..3d507d4204 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -721,6 +721,19 @@ int xgethostname(char *buf, size_t len)\n \treturn ret;\n }\n \n+int is_missing_file(const char *filename)\n+{\n+\tstruct stat st;\n+\n+\tif (stat(filename, &st) < 0) {\n+\t\tif (errno == ENOENT)\n+\t\t\treturn 1;\n+\t\tdie_errno(_(\"could not stat %s\"), filename);\n+\t}\n+\n+\treturn 0;\n+}\n+\n int is_empty_or_missing_file(const char *filename)\n {\n \tstruct stat st;\ndiff --git a/wrapper.h b/wrapper.h\nindex 7df824e34a..44a8597ac3 100644\n--- a/wrapper.h\n+++ b/wrapper.h\n@@ -66,7 +66,9 @@ void write_file_buf(const char *path, const char *buf, size_t len);\n __attribute__((format (printf, 2, 3)))\n void write_file(const char *path, const char *fmt, ...);\n \n-/* Return 1 if the file is empty or does not exists, 0 otherwise. */\n+/* Return 1 if the file does not exist, 0 otherwise. */\n+int is_missing_file(const char *filename);\n+/* Return 1 if the file is empty or does not exist, 0 otherwise. */\n int is_empty_or_missing_file(const char *filename);\n \n enum fsync_action {\n-- \n2.48.1\n\n"},{"id":"527522","messageId":"5f7057c236c9af3152bd531eed2e4ad0ac35e291.1759094936.git.ben.knoble+github@gmail.com","threadId":"56234","inReplyTo":"cover.1759094936.git.ben.knoble+github@gmail.com","subject":"[PATCH v2 3/3] parseopt: values of pathname type can be prefixed with :(optional)","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-09-28T21:29:16Z","receivedAt":"2025-09-28T21:30:19Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"From: Junio C Hamano \n\nIn the previous step, we introduced an optional filename that can be\ngiven to a configuration variable, and nullify the fact that such a\nconfiguration setting even existed if the named path is missing or\nempty.\n\nLet's do the same for command line options that name a pathname.\n\nSigned-off-by: Junio C Hamano \nSigned-off-by: Taylor Blau \nSigned-off-by: D. Ben Knoble \n---\n Documentation/gitcli.adoc | 14 ++++++++++\n parse-options.c | 31 +++++++++++++++--------\n t/t7500-commit-template-squash-signoff.sh | 10 ++++++++\n 3 files changed, 44 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/gitcli.adoc b/Documentation/gitcli.adoc\nindex 1ea681b59d..ef2a0a399d 100644\n--- a/Documentation/gitcli.adoc\n+++ b/Documentation/gitcli.adoc\n@@ -216,6 +216,20 @@ $ git describe --abbrev=10 HEAD # correct\n $ git describe --abbrev 10 HEAD # NOT WHAT YOU MEANT\n ----------------------------\n \n+\n+Magic filename options\n+~~~~~~~~~~~~~~~~~~~~~~\n+Options that take a filename allow a prefix `:(optional)`. For example:\n+\n+----------------------------\n+git commit -F :(optional)COMMIT_EDITMSG\n+# if COMMIT_EDITMSG does not exist, equivalent to\n+git commit\n+----------------------------\n+\n+Like with configuration values, if the named file is missing Git behaves as if\n+the option was not given at all. See \"Values\" in linkgit:git-config[1].\n+\n NOTES ON FREQUENTLY CONFUSED OPTIONS\n ------------------------------------\n \ndiff --git a/parse-options.c b/parse-options.c\nindex 5224203ffe..4faf66023a 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -133,7 +133,6 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n {\n \tconst char *arg;\n \tconst int unset = flags & OPT_UNSET;\n-\tint err;\n \n \tif (unset && p->opt)\n \t\treturn error(_(\"%s takes no value\"), optname(opt, flags));\n@@ -209,21 +208,31 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \tcase OPTION_FILENAME:\n \t{\n \t\tconst char *value;\n-\n-\t\tFREE_AND_NULL(*(char **)opt->value);\n-\n-\t\terr = 0;\n+\t\tint is_optional;\n \n \t\tif (unset)\n \t\t\tvalue = NULL;\n \t\telse if (opt->flags & PARSE_OPT_OPTARG && !p->opt)\n-\t\t\tvalue = (const char *) opt->defval;\n-\t\telse\n-\t\t\terr = get_arg(p, opt, flags, &value);\n+\t\t\tvalue = (char *)opt->defval;\n+\t\telse {\n+\t\t\tint err = get_arg(p, opt, flags, &value);\n+\t\t\tif (err)\n+\t\t\t\treturn err;\n+\t\t}\n+\t\tif (!value)\n+\t\t\treturn 0;\n \n-\t\tif (!err)\n-\t\t\t*(char **)opt->value = fix_filename(p->prefix, value);\n-\t\treturn err;\n+\t\tis_optional = skip_prefix(value, \":(optional)\", &value);\n+\t\tif (!value)\n+\t\t\tis_optional = 0;\n+\t\tvalue = fix_filename(p->prefix, value);\n+\t\tif (is_optional && is_empty_or_missing_file(value)) {\n+\t\t\tfree((char *)value);\n+\t\t} else {\n+\t\t\tFREE_AND_NULL(*(char **)opt->value);\n+\t\t\t*(const char **)opt->value = value;\n+\t\t}\n+\t\treturn 0;\n \t}\n \tcase OPTION_CALLBACK:\n \t{\ndiff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\nindex 366f7f23b3..c065f12baf 100755\n--- a/t/t7500-commit-template-squash-signoff.sh\n+++ b/t/t7500-commit-template-squash-signoff.sh\n@@ -37,6 +37,16 @@ commit_msg_is ()\n \t)\n '\n \n+test_expect_success 'nonexistent optional template file on command line' '\n+\techo changes >> foo &&\n+\tgit add foo &&\n+\t(\n+\t\tGIT_EDITOR=\"echo hello >\\\"\\$1\\\"\" &&\n+\t\texport GIT_EDITOR &&\n+\t\tgit commit --template \":(optional)$PWD/notexist\"\n+\t)\n+'\n+\n test_expect_success 'nonexistent template file in config should return error' '\n \ttest_config commit.template \"$PWD\"/notexist &&\n \t(\n-- \n2.48.1\n\n"},{"id":"527575","messageId":"xmqqh5wm5hgu.fsf@gitster.g","threadId":"56234","inReplyTo":"cover.1759094936.git.ben.knoble+github@gmail.com","subject":"Re: [PATCH v2 0/3] Support :(optional) filepaths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-28T22:40:17Z","receivedAt":"2025-09-28T22:40:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"D. Ben Knoble\" writes:\n\nBefore \"notes\" you would want an overall description of what the\ntopic is for those who no longer remember the previous iteration,\nor for those this iteration is the first one they see.\n\n> Notes:\n> - Based on commit 2da08f2c3d (parseopt: values of pathname type can be\n> prefixed with :(optional), 2024-10-14) (broken-out/wip/optional-path)\n> - Rebased on v2.51.0\n\nThanks.\n\n> - I'm least sure of the 3rd patch and am happy to drop it in support of\n> the first 2. I think it might be better to (a) integrate :(optional)\n> support as pathspec magic and (b) use pathspec magic in parse-options\n> when getting filenames. But I'm not sure, and this has other\n> ramifications I'm not prepared to deal with. (For example: `git grep\n> path … :(optional)non-existent` could pretend like\n> `non-existent` was never given?)\n\nWhile it might not hurt, I do not see a need for such a support.\n\nPathspec _is_ a pattern. If an existing path does not match the\npattern, there is no ill effect. In other words, in this command\ninvocation:\n\n $ git grep -e needle -- Makefile no-such-file.txt\n\nneither Makefile or no-such-file.txt is required nor optional. If\nthere are paths that match these two \"patterns\" among the paths in\nthe working tree that are known to the index, the contents of these\npaths are inspected by the command. If no paths match the patterns,\nthat is fine as well.\n\nThe command line parser helpfully offers to notice a pathspec\npattern that did not match any path when you do not give \"--\", but\nthat is up to the caller of match_pathspec() API to do so. The\npathspec machinery only reports if each pathspec element matched a\npath in its seen[] array, and the caller can use that information to\nreport which pathspec elements did not contribute to finding the set\nof paths to work on.\n\n> - The parsing is not exactly a \"clean API,\" but I wasn't sure how to\n> make it cleaner :)\n\nWhat you have in [2/3], the update to git_config_pathname(), seems\nquite reasonable and something that cannot be made cleaner, to me.\n\n> Changes in v2:\n> - Only check for missing files, not empty files\n> - Move a test change to the appropriate commit\n> - Document optional magic in options in gitcli(1)\n\nI agree that it is a better design not to special case an empty file\nlike the previous round did. Looking better.\n\n> This series adds support for optional filepaths in config and\n> parse-options, which supports use-cases such as missing commit templates\n> or blame.ignoreRevsFile values without erroring.\n\nYes, this is what you wanted to have at the very beginning, before\nlisting points you want to call attention to under \"Notes\" label.\n\nWill queue. Thanks for resurrecting the topic.\n"},{"id":"527590","messageId":"6646024D-319D-47D9-805A-CEB3A620E4BC@gmail.com","threadId":"56234","inReplyTo":"xmqqh5wm5hgu.fsf@gitster.g","subject":"Re: [PATCH v2 0/3] Support :(optional) filepaths","fromName":"Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-09-29T16:42:53Z","receivedAt":"2025-09-29T16:43:05Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"\n> Le 28 sept. 2025 à 18:40, Junio C Hamano a écrit :\n> \n> \"D. Ben Knoble\" writes:\n> \n> Before \"notes\" you would want an overall description of what the\n> topic is for those who no longer remember the previous iteration,\n> or for those this iteration is the first one they see.\n\nAgreed, thanks.\n\n>> - I'm least sure of the 3rd patch and am happy to drop it in support of\n>> the first 2. I think it might be better to (a) integrate :(optional)\n>> support as pathspec magic and (b) use pathspec magic in parse-options\n>> when getting filenames. But I'm not sure, and this has other\n>> ramifications I'm not prepared to deal with. (For example: `git grep\n>> path … :(optional)non-existent` could pretend like\n>> `non-existent` was never given?)\n> \n> While it might not hurt, I do not see a need for such a support.\n> \n> Pathspec _is_ a pattern. If an existing path does not match the\n> pattern, there is no ill effect. In other words, in this command\n> invocation:\n> \n> $ git grep -e needle -- Makefile no-such-file.txt\n> \n> neither Makefile or no-such-file.txt is required nor optional. If\n> there are paths that match these two \"patterns\" among the paths in\n> the working tree that are known to the index, the contents of these\n> paths are inspected by the command. If no paths match the patterns,\n> that is fine as well.\n> \n> The command line parser helpfully offers to notice a pathspec\n> pattern that did not match any path when you do not give \"--\", but\n> that is up to the caller of match_pathspec() API to do so. The\n> pathspec machinery only reports if each pathspec element matched a\n> path in its seen[] array, and the caller can use that information to\n> report which pathspec elements did not contribute to finding the set\n> of paths to work on.\n\nI must have been thinking of the case without --, which triggers the usual ambiguity error. Either way, for now, I think a smaller feature is better :)\n\n> Will queue. Thanks for resurrecting the topic.\n\nThanks!"},{"id":"527640","messageId":"a687ec17-8ee4-428e-bae5-063716d59a08@gmail.com","threadId":"56234","inReplyTo":"5c97f580a9e77c464bc6bf4ed9ea8546711c6637.1759094936.git.ben.knoble+github@gmail.com","subject":"Re: [PATCH v2 2/3] config: values of pathname type can be prefixed with :(optional)","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-09-30T15:26:36Z","receivedAt":"2025-09-30T15:26:39Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ben\n\nOn 28/09/2025 22:29, D. Ben Knoble wrote:\n> From: Junio C Hamano \n> \n> Sometimes people want to specify additional configuration data\n> as \"best effort\" basis. Maybe commit.template configuration file points\n> at somewhere in ~/template/ but on a particular system, the file may not\n> exist and the user may be OK without using the template in such a case.\n> \n> When the value given to a configuration variable whose type is\n> pathname wants to signal such an optional file, it can be marked by\n> prepending \":(optional)\" in front of it. Such a setting that is\n> marked optional would avoid getting the command barf for a missing\n> file, as an optional configuration setting that names a missing\n> file is not even seen.\n\nI think this would be a useful addition, we've had several people \nwanting to make blame.ignoreRevsFile optional and this provides a \ngeneral way to do that.\n\n> --- a/config.c\n> +++ b/config.c\n> @@ -1279,11 +1279,23 @@ int git_config_string(char **dest, const char *var, const char *value)\n> \n> int git_config_pathname(char **dest, const char *var, const char *value)\n> {\n> +\tint is_optional;\n\nThis could be bool rather than int, the rest of the implementation looks \ngood.\n\n> --- a/t/t7500-commit-template-squash-signoff.sh\n> +++ b/t/t7500-commit-template-squash-signoff.sh\n> @@ -46,6 +46,15 @@ commit_msg_is ()\n> \t)\n> '\n> \n> +test_expect_success 'nonexistent optional template file in config' '\n> +\ttest_config commit.template \":(optional)$PWD\"/notexist &&\n> +\t(\n> +\t\tGIT_EDITOR=\"echo hello >\\\"\\$1\\\"\" &&\n\nwhen git runs the editor this will be expanded to\n\n sh -c 'echo hello >\"$1\" \"$@\"' 'echo hello >\"$1\"' path/to/file\n\nI think it should be\n\n GIT_EDITOR=\"echo hello >\"\n\ninstead\n> +\t\texport GIT_EDITOR &&\n> +\t\tgit commit --allow-empty\n\nMaybe I'm missing something but don't we want to ensure that we have a \nnon-empty message here? Also as it is a single command we can avoid the \nsubshell with\n\n GIT_EDITOR=\"echo hello >\" git commit\n\nThanks\n\nPhillip\n\n"},{"id":"527641","messageId":"e8755a04-bd44-4ead-ba44-c603bffcc75e@gmail.com","threadId":"56234","inReplyTo":"5f7057c236c9af3152bd531eed2e4ad0ac35e291.1759094936.git.ben.knoble+github@gmail.com","subject":"Re: [PATCH v2 3/3] parseopt: values of pathname type can be prefixed with :(optional)","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-09-30T15:26:45Z","receivedAt":"2025-09-30T15:26:48Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ben\n\nOn 28/09/2025 22:29, D. Ben Knoble wrote:\n> From: Junio C Hamano \n> \n> In the previous step, we introduced an optional filename that can be\n> given to a configuration variable, and nullify the fact that such a\n> configuration setting even existed if the named path is missing or\n> empty.\n> \n> Let's do the same for command line options that name a pathname.\n\nSounds sensible\n\n> +Magic filename options\n\nI assume we're calling these \"magic\" to match to pathspec \"magic\" \noptions? I wonder if that is a good idea but I don't have a better \nsuggestion.\n\n> +~~~~~~~~~~~~~~~~~~~~~~\n> +Options that take a filename allow a prefix `:(optional)`. For example:\n> +\n> +----------------------------\n> +git commit -F :(optional)COMMIT_EDITMSG\n> +# if COMMIT_EDITMSG does not exist, equivalent to\n\nThis doesn't quite scan for me, maybe s/, /, it is/ ?\n\n> +git commit\n> +----------------------------\n> +\n> +Like with configuration values, if the named file is missing Git behaves as if\n\nI'd drop \"with\" here\n\n> +the option was not given at all. See \"Values\" in linkgit:git-config[1].\n> +\n\n> @@ -209,21 +208,31 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n> \tcase OPTION_FILENAME:\n> \t{\n> \t\tconst char *value;\n> -\n> -\t\tFREE_AND_NULL(*(char **)opt->value);\n> -\n> -\t\terr = 0;\n> +\t\tint is_optional;\n\nThis can be a bool as in the last patch.\n\n> \t\tif (unset)\n> \t\t\tvalue = NULL;\n> \t\telse if (opt->flags & PARSE_OPT_OPTARG && !p->opt)\n> -\t\t\tvalue = (const char *) opt->defval;\n> -\t\telse\n> -\t\t\terr = get_arg(p, opt, flags, &value);\n> +\t\t\tvalue = (char *)opt->defval;\n\nI'm not sure why we're changing the cast here (or why we need one in the \nfirst place assuming opt->defval is \"void*\")\n\n> +\t\telse {\n> +\t\t\tint err = get_arg(p, opt, flags, &value);\n> +\t\t\tif (err)\n> +\t\t\t\treturn err;\n> +\t\t}\n> +\t\tif (!value)\n> +\t\t\treturn 0;\n> \n> -\t\tif (!err)\n> -\t\t\t*(char **)opt->value = fix_filename(p->prefix, value);\n> -\t\treturn err;\n> +\t\tis_optional = skip_prefix(value, \":(optional)\", &value);\n> +\t\tif (!value)\n> +\t\t\tis_optional = 0;\n\nI'm struggling to see how value can be NULL here as we return early if \nit NULL before calling skip_prefix()\n\n> +\t\tvalue = fix_filename(p->prefix, value);\n> +\t\tif (is_optional && is_empty_or_missing_file(value)) {\n> +\t\t\tfree((char *)value);\n\nI think we want to call is_missing_file() here. If the file is missing \nthen we do nothing which matches the documentation above - Good.\n\n> +\t\t} else {\n> +\t\t\tFREE_AND_NULL(*(char **)opt->value);\n> +\t\t\t*(const char **)opt->value = value;\n\nIf the file isn't optional or it is optional and exists then we behave \nas before - Good.\n\nThanks\n\nPhillip\n\n> +\t\t}\n> +\t\treturn 0;\n> \t}\n> \tcase OPTION_CALLBACK:\n> \t{\n> diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\n> index 366f7f23b3..c065f12baf 100755\n> --- a/t/t7500-commit-template-squash-signoff.sh\n> +++ b/t/t7500-commit-template-squash-signoff.sh\n> @@ -37,6 +37,16 @@ commit_msg_is ()\n> \t)\n> '\n> \n> +test_expect_success 'nonexistent optional template file on command line' '\n> +\techo changes >> foo &&\n> +\tgit add foo &&\n> +\t(\n> +\t\tGIT_EDITOR=\"echo hello >\\\"\\$1\\\"\" &&\n> +\t\texport GIT_EDITOR &&\n> +\t\tgit commit --template \":(optional)$PWD/notexist\"\n> +\t)\n> +'\n> +\n> test_expect_success 'nonexistent template file in config should return error' '\n> \ttest_config commit.template \"$PWD\"/notexist &&\n> \t(\n\n"},{"id":"528020","messageId":"xmqqzfa3onxx.fsf@gitster.g","threadId":"56234","inReplyTo":"a687ec17-8ee4-428e-bae5-063716d59a08@gmail.com","subject":"Re: [PATCH v2 2/3] config: values of pathname type can be prefixed with :(optional)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-06T19:00:26Z","receivedAt":"2025-10-06T19:00:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>> +\ttest_config commit.template \":(optional)$PWD\"/notexist &&\n>> +\t(\n>> +\t\tGIT_EDITOR=\"echo hello >\\\"\\$1\\\"\" &&\n>\n> when git runs the editor this will be expanded to\n>\n> sh -c 'echo hello >\"$1\" \"$@\"' 'echo hello >\"$1\"' path/to/file\n>\n> I think it should be\n>\n> GIT_EDITOR=\"echo hello >\"\n>\n> instead\n\nThat's interesting in that I find it unusual. Fine as long as it\nworks ;-)\n\n> Maybe I'm missing something but don't we want to ensure that we have a\n> non-empty message here? Also as it is a single command we can avoid\n> the subshell with\n>\n> GIT_EDITOR=\"echo hello >\" git commit\n\nYeah, that does sound better.\n"},{"id":"528035","messageId":"xmqqsefvol7s.fsf@gitster.g","threadId":"56234","inReplyTo":"xmqqzfa3onxx.fsf@gitster.g","subject":"Re: [PATCH v2 2/3] config: values of pathname type can be prefixed with :(optional)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-06T19:59:19Z","receivedAt":"2025-10-06T19:59:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Phillip Wood writes:\n>\n>>> +\ttest_config commit.template \":(optional)$PWD\"/notexist &&\n>>> +\t(\n>>> +\t\tGIT_EDITOR=\"echo hello >\\\"\\$1\\\"\" &&\n>>\n>> when git runs the editor this will be expanded to\n>>\n>> sh -c 'echo hello >\"$1\" \"$@\"' 'echo hello >\"$1\"' path/to/file\n>>\n>> I think it should be\n>>\n>> GIT_EDITOR=\"echo hello >\"\n>>\n>> instead\n\nIt seems that this was a copy-paste from a few of tests before this\nnew piece. They all _expect_ to fail, so probably nobody bothered\nto inspect the outcome ;-)\n\nI just looked at what actually goes to COMMIT_EDITMSG with this test\nthat expects to succeed.\n\n$ cat .git/COMMIT_EDITMSG\nhello /home/gitster/w/git.git/t/trash directory.t7500-commit-template-squash-signoff/.git/COMMIT_EDITMSG\n\nSo, you're right to say \"$@\" will be given in addition to \"hello\" as\narguments to \"echo\". That extra argument is to tell the editor the\npath to the edited file.\n\nWe'd probably need a preliminary clean-up patch to fix all of these\nin the vicinity.\n\nThanks.\n"},{"id":"528037","messageId":"xmqqms63ok7g.fsf@gitster.g","threadId":"56234","inReplyTo":"xmqqsefvol7s.fsf@gitster.g","subject":"Re: [PATCH v2 2/3] config: values of pathname type can be prefixed with :(optional)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-06T20:21:07Z","receivedAt":"2025-10-06T20:21:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> We'd probably need a preliminary clean-up patch to fix all of these\n> in the vicinity.\n\nSo, here is the preliminary clea-up step that should come before\n[2/3]\n\n--- >8 ---\nSubject: [PATCH] t7500: fix GIT_EDITOR shell snippet\n\n2140b140 (commit: error out for missing commit message template,\n2011-02-25) defined\n\n GIT_EDITOR=\"echo hello >\\\"\\$1\\\"\"\n\nfor thest two tests, with the intention that 'hello' would be\nwritten in the given file, but as Phillip Wood points out,\nGIT_EDITOR is invoked by shell after getting expanded to\n\n sh -c 'echo hello >\"$1\" \"$@\"' 'echo hello >\"$1\"' path/to/file\n\nwhich is not what we want.\n\nSigned-off-by: Junio C Hamano \n---\n t/t7500-commit-template-squash-signoff.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\nindex 05cda50186..4922543256 100755\n--- a/t/t7500-commit-template-squash-signoff.sh\n+++ b/t/t7500-commit-template-squash-signoff.sh\n@@ -31,7 +31,7 @@ test_expect_success 'nonexistent template file should return error' '\n \techo changes >> foo &&\n \tgit add foo &&\n \t(\n-\t\tGIT_EDITOR=\"echo hello >\\\"\\$1\\\"\" &&\n+\t\tGIT_EDITOR=\"echo hello >\" &&\n \t\texport GIT_EDITOR &&\n \t\ttest_must_fail git commit --template \"$PWD\"/notexist\n \t)\n@@ -40,7 +40,7 @@ test_expect_success 'nonexistent template file should return error' '\n test_expect_success 'nonexistent template file in config should return error' '\n \ttest_config commit.template \"$PWD\"/notexist &&\n \t(\n-\t\tGIT_EDITOR=\"echo hello >\\\"\\$1\\\"\" &&\n+\t\tGIT_EDITOR=\"echo hello >\" &&\n \t\texport GIT_EDITOR &&\n \t\ttest_must_fail git commit --allow-empty\n \t)\n-- \n2.51.0-580-g8258b70b6e\n\n"},{"id":"528038","messageId":"xmqqikgrok4u.fsf@gitster.g","threadId":"56234","inReplyTo":"xmqqms63ok7g.fsf@gitster.g","subject":"Re: [PATCH v2 2/3] config: values of pathname type can be prefixed with :(optional)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-06T20:22:41Z","receivedAt":"2025-10-06T20:22:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> So, here is the preliminary clea-up step that should come before\n> [2/3]\n>\n> --- >8 ---\n> Subject: [PATCH] t7500: fix GIT_EDITOR shell snippet\n\nAnd this is [2/3] rebased on top.\n\n--- >8 ---\nSubject: [PATCH] config: values of pathname type can be prefixed with :(optional)\n\nSometimes people want to specify additional configuration data\nas \"best effort\" basis. Maybe commit.template configuration file points\nat somewhere in ~/template/ but on a particular system, the file may not\nexist and the user may be OK without using the template in such a case.\n\nWhen the value given to a configuration variable whose type is\npathname wants to signal such an optional file, it can be marked by\nprepending \":(optional)\" in front of it. Such a setting that is\nmarked optional would avoid getting the command barf for a missing\nfile, as an optional configuration setting that names a missing\nfile is not even seen.\n\ncf. \n\nSigned-off-by: Junio C Hamano \nSigned-off-by: Taylor Blau \nSigned-off-by: D. Ben Knoble \nSigned-off-by: Junio C Hamano \n---\n Documentation/config.adoc | 4 +++-\n config.c | 16 ++++++++++++++--\n t/t7500-commit-template-squash-signoff.sh | 8 ++++++++\n wrapper.c | 13 +++++++++++++\n wrapper.h | 4 +++-\n 5 files changed, 41 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config.adoc b/Documentation/config.adoc\nindex cc769251be..7301ced836 100644\n--- a/Documentation/config.adoc\n+++ b/Documentation/config.adoc\n@@ -358,7 +358,9 @@ compiled without runtime prefix support, the compiled-in prefix will be\n substituted instead. In the unlikely event that a literal path needs to\n be specified that should _not_ be expanded, it needs to be prefixed by\n `./`, like so: `./%(prefix)/bin`.\n-\n++\n+If prefixed with `:(optional)`, the configuration variable is treated\n+as if it does not exist, if the named path does not exist.\n \n Variables\n ~~~~~~~~~\ndiff --git a/config.c b/config.c\nindex 97ffef4270..73fc74c8fa 100644\n--- a/config.c\n+++ b/config.c\n@@ -1279,11 +1279,23 @@ int git_config_string(char **dest, const char *var, const char *value)\n \n int git_config_pathname(char **dest, const char *var, const char *value)\n {\n+\tint is_optional;\n+\tchar *path;\n+\n \tif (!value)\n \t\treturn config_error_nonbool(var);\n-\t*dest = interpolate_path(value, 0);\n-\tif (!*dest)\n+\n+\tis_optional = skip_prefix(value, \":(optional)\", &value);\n+\tpath = interpolate_path(value, 0);\n+\tif (!path)\n \t\tdie(_(\"failed to expand user dir in: '%s'\"), value);\n+\n+\tif (is_optional && is_missing_file(path)) {\n+\t\tfree(path);\n+\t\treturn 0;\n+\t}\n+\n+\t*dest = path;\n \treturn 0;\n }\n \ndiff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\nindex 4922543256..a85229e556 100755\n--- a/t/t7500-commit-template-squash-signoff.sh\n+++ b/t/t7500-commit-template-squash-signoff.sh\n@@ -46,6 +46,14 @@ test_expect_success 'nonexistent template file in config should return error' '\n \t)\n '\n \n+test_expect_success 'nonexistent optional template file in config' '\n+\ttest_config commit.template \":(optional)$PWD\"/notexist &&\n+\tGIT_EDITOR=\"echo hello >\" git commit --allow-empty &&\n+\tgit cat-file commit HEAD | sed -e \"1,/^$/d\" >actual &&\n+\techo hello >expect &&\n+\ttest_cmp expect actual\n+'\n+\n # From now on we'll use a template file that exists.\n TEMPLATE=\"$PWD\"/template\n \ndiff --git a/wrapper.c b/wrapper.c\nindex 2f00d2ac87..3d507d4204 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -721,6 +721,19 @@ int xgethostname(char *buf, size_t len)\n \treturn ret;\n }\n \n+int is_missing_file(const char *filename)\n+{\n+\tstruct stat st;\n+\n+\tif (stat(filename, &st) < 0) {\n+\t\tif (errno == ENOENT)\n+\t\t\treturn 1;\n+\t\tdie_errno(_(\"could not stat %s\"), filename);\n+\t}\n+\n+\treturn 0;\n+}\n+\n int is_empty_or_missing_file(const char *filename)\n {\n \tstruct stat st;\ndiff --git a/wrapper.h b/wrapper.h\nindex 7df824e34a..44a8597ac3 100644\n--- a/wrapper.h\n+++ b/wrapper.h\n@@ -66,7 +66,9 @@ void write_file_buf(const char *path, const char *buf, size_t len);\n __attribute__((format (printf, 2, 3)))\n void write_file(const char *path, const char *fmt, ...);\n \n-/* Return 1 if the file is empty or does not exists, 0 otherwise. */\n+/* Return 1 if the file does not exist, 0 otherwise. */\n+int is_missing_file(const char *filename);\n+/* Return 1 if the file is empty or does not exist, 0 otherwise. */\n int is_empty_or_missing_file(const char *filename);\n \n enum fsync_action {\n-- \n2.51.0-580-g8258b70b6e\n\n"},{"id":"528090","messageId":"18d9eef5-a1dc-4d9d-957b-ae630f0a2b12@app.fastmail.com","threadId":"56234","inReplyTo":"xmqqms63ok7g.fsf@gitster.g","subject":"Re: [PATCH v2 2/3] config: values of pathname type can be prefixed with :(optional)","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-10-07T12:24:38Z","receivedAt":"2025-10-07T12:25:01Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Mon, Oct 6, 2025, at 22:21, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> We'd probably need a preliminary clean-up patch to fix all of these\n>> in the vicinity.\n>\n> So, here is the preliminary clea-up step that should come before\n> [2/3]\n>\n> --- >8 ---\n> Subject: [PATCH] t7500: fix GIT_EDITOR shell snippet\n>\n> 2140b140 (commit: error out for missing commit message template,\n> 2011-02-25) defined\n>\n> GIT_EDITOR=\"echo hello >\\\"\\$1\\\"\"\n>\n> for thest two tests, with the intention that 'hello' would be\n\ns/thest/these/\n\n>[snip]\n"},{"id":"528135","messageId":"xmqqzfa2k5im.fsf@gitster.g","threadId":"56234","inReplyTo":"18d9eef5-a1dc-4d9d-957b-ae630f0a2b12@app.fastmail.com","subject":"Re: [PATCH v2 2/3] config: values of pathname type can be prefixed with :(optional)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-07T17:04:17Z","receivedAt":"2025-10-07T17:04:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" writes:\n\n> On Mon, Oct 6, 2025, at 22:21, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>>\n>>> We'd probably need a preliminary clean-up patch to fix all of these\n>>> in the vicinity.\n>>\n>> So, here is the preliminary clea-up step that should come before\n>> [2/3]\n>>\n>> --- >8 ---\n>> Subject: [PATCH] t7500: fix GIT_EDITOR shell snippet\n>>\n>> 2140b140 (commit: error out for missing commit message template,\n>> 2011-02-25) defined\n>>\n>> GIT_EDITOR=\"echo hello >\\\"\\$1\\\"\"\n>>\n>> for thest two tests, with the intention that 'hello' would be\n>\n> s/thest/these/\n\nThanks. Will modify locally.\n"},{"id":"529164","messageId":"6a83c7d1-7cd4-432e-a0ab-7b18ce3af08d@kdbg.org","threadId":"56234","inReplyTo":"cover.1759094936.git.ben.knoble+github@gmail.com","subject":"[PATCH] t7500: fix tests with absolute path following \":(optional)\" on Windows","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2025-10-20T09:40:08Z","receivedAt":"2025-10-20T09:40:26Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Windows, the MSYS layer translates absolute path names generated by\na shell script from the POSIX style /c/dir/file to the Windows style\nC:/dir/file form that is understood by git.exe. This happens only when\nthe absolute path stands on its own as a program argument or a value of\nan environment variable.\n\nThe earlier commits 749d6d166d (config: values of pathname type can be\nprefixed with :(optional), 2025-09-28) and ccfcaf399f (parseopt: values\nof pathname type can be prefixed with :(optional), 2025-09-28) added\ntest cases where \":(optional)\" is inserted before an absolute path.\n$PWD is used to construct the absolute paths, which gives the POSIX\nform, and the result is \":(optional)/c/dir/template\". Such command line\narguments are no longer recognized as absolute paths and do not undergo\ntranslation.\n\nExisting test cases that expect that the specified file does not exist\nare not incorrect (after all, git.exe will not find /c/dir/template).\nYet, they are conceptually incorrect. That the use of $PWD is erroneous\nis revealed by a test case that expects that the optional file exists.\nSince no such test case is present, add one. Use \"$(pwd)\" to generate\nthe absolute paths, so that the command line arguments become\n\":(optional)C:/dir/template\".\n\nSigned-off-by: Johannes Sixt \n---\n It's pure coincidence that I had a closer look at t7500 today.\n\n t/t7500-commit-template-squash-signoff.sh | 19 ++++++++++++++-----\n 1 file changed, 14 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\nindex 1145ea783b..1072c84bf2 100755\n--- a/t/t7500-commit-template-squash-signoff.sh\n+++ b/t/t7500-commit-template-squash-signoff.sh\n@@ -33,7 +33,7 @@ commit_msg_is () {\n \t(\n \t\tGIT_EDITOR=\"echo hello >\" &&\n \t\texport GIT_EDITOR &&\n-\t\ttest_must_fail git commit --template \"$PWD\"/notexist\n+\t\ttest_must_fail git commit --template \"$(pwd)\"/notexist\n \t)\n '\n \n@@ -43,12 +43,12 @@ commit_msg_is () {\n \t(\n \t\tGIT_EDITOR=\"echo hello >\\\"\\$1\\\"\" &&\n \t\texport GIT_EDITOR &&\n-\t\tgit commit --template \":(optional)$PWD/notexist\"\n+\t\tgit commit --template \":(optional)$(pwd)/notexist\"\n \t)\n '\n \n test_expect_success 'nonexistent template file in config should return error' '\n-\ttest_config commit.template \"$PWD\"/notexist &&\n+\ttest_config commit.template \"$(pwd)\"/notexist &&\n \t(\n \t\tGIT_EDITOR=\"echo hello >\" &&\n \t\texport GIT_EDITOR &&\n@@ -57,7 +57,7 @@ commit_msg_is () {\n '\n \n test_expect_success 'nonexistent optional template file in config' '\n-\ttest_config commit.template \":(optional)$PWD\"/notexist &&\n+\ttest_config commit.template \":(optional)$(pwd)\"/notexist &&\n \tGIT_EDITOR=\"echo hello >\" git commit --allow-empty &&\n \tgit cat-file commit HEAD | sed -e \"1,/^$/d\" >actual &&\n \techo hello >expect &&\n@@ -65,7 +65,7 @@ commit_msg_is () {\n '\n \n # From now on we'll use a template file that exists.\n-TEMPLATE=\"$PWD\"/template\n+TEMPLATE=\"$(pwd)\"/template\n \n test_expect_success 'unedited template should not commit' '\n \techo \"template line\" >\"$TEMPLATE\" &&\n@@ -99,6 +99,15 @@ commit_msg_is () {\n \tcommit_msg_is \"template linecommit message\"\n '\n \n+test_expect_success 'existent template marked optional should commit' '\n+\techo \"existent template\" >\"$TEMPLATE\" &&\n+\t(\n+\t\ttest_set_editor \"$TEST_DIRECTORY\"/t7500/add-content &&\n+\t\tgit commit --allow-empty --template \":(optional)$TEMPLATE\"\n+\t) &&\n+\tcommit_msg_is \"existent templatecommit message\"\n+'\n+\n test_expect_success '-t option should be short for --template' '\n \techo \"short template\" > \"$TEMPLATE\" &&\n \techo \"new content\" >> foo &&\n-- \n2.51.0.431.g0f99086cdf\n\n\n"},{"id":"529165","messageId":"A563E028-19E7-48A0-B538-82ACE821DB67@gmail.com","threadId":"56234","inReplyTo":"6a83c7d1-7cd4-432e-a0ab-7b18ce3af08d@kdbg.org","subject":"Re: [PATCH] t7500: fix tests with absolute path following \":(optional)\" on Windows","fromName":"Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-10-20T13:43:55Z","receivedAt":"2025-10-20T13:44:07Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"\n> Le 20 oct. 2025 à 05:40, Johannes Sixt a écrit :\n> \n> On Windows, the MSYS layer translates absolute path names generated by\n> a shell script from the POSIX style /c/dir/file to the Windows style\n> C:/dir/file form that is understood by git.exe. This happens only when\n> the absolute path stands on its own as a program argument or a value of\n> an environment variable.\n> \n> The earlier commits 749d6d166d (config: values of pathname type can be\n> prefixed with :(optional), 2025-09-28) and ccfcaf399f (parseopt: values\n> of pathname type can be prefixed with :(optional), 2025-09-28) added\n> test cases where \":(optional)\" is inserted before an absolute path.\n> $PWD is used to construct the absolute paths, which gives the POSIX\n> form, and the result is \":(optional)/c/dir/template\". Such command line\n> arguments are no longer recognized as absolute paths and do not undergo\n> translation.\n> \n> Existing test cases that expect that the specified file does not exist\n> are not incorrect (after all, git.exe will not find /c/dir/template).\n> Yet, they are conceptually incorrect. That the use of $PWD is erroneous\n> is revealed by a test case that expects that the optional file exists.\n> Since no such test case is present, add one. Use \"$(pwd)\" to generate\n> the absolute paths, so that the command line arguments become\n> \":(optional)C:/dir/template\".\n\nThanks! I probably assumed there was no meaningful difference between the value of PWD and what pwd computes, so (prematurely) optimized for a lookup over executing a command.\n\nGoing forward I will probably stick with using pwd, given the difference in platform behavior.\n\nIs there a doc or test lint for that? If not, might be useful."},{"id":"529175","messageId":"xmqqh5vt1rb0.fsf@gitster.g","threadId":"56234","inReplyTo":"6a83c7d1-7cd4-432e-a0ab-7b18ce3af08d@kdbg.org","subject":"Re: [PATCH] t7500: fix tests with absolute path following \":(optional)\" on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-20T16:17:07Z","receivedAt":"2025-10-20T16:17:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Existing test cases that expect that the specified file does not exist\n> are not incorrect (after all, git.exe will not find /c/dir/template).\n> Yet, they are conceptually incorrect.\n\nWow, if I am counting correctly, the oldest one is from July 2007,\nand we have been running these tests without anybody noticing?\nThat's just ... wow.\n\n> Signed-off-by: Johannes Sixt \n> ---\n> It's pure coincidence that I had a closer look at t7500 today.\n\nThanks, will queue.\n"},{"id":"529181","messageId":"01e65d25-33de-4025-b3c1-52dc7d58fc27@kdbg.org","threadId":"56234","inReplyTo":"xmqqh5vt1rb0.fsf@gitster.g","subject":"Re: [PATCH] t7500: fix tests with absolute path following \":(optional)\" on Windows","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2025-10-20T17:24:47Z","receivedAt":"2025-10-20T17:24:57Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 20.10.25 um 18:17 schrieb Junio C Hamano:\n> Johannes Sixt writes:\n> \n>> Existing test cases that expect that the specified file does not exist\n>> are not incorrect (after all, git.exe will not find /c/dir/template).\n>> Yet, they are conceptually incorrect.\n> \n> Wow, if I am counting correctly, the oldest one is from July 2007,\n> and we have been running these tests without anybody noticing?\n> That's just ... wow.\n\nObviously, I didn't do a great job in explaining the situation. It isn't\n*that* bad.\n\nBefore the invention of the \":(optional)\" prefix, the tests are totally\nfine, because /c/dir/template or /c/dir/notexist always appear as an\nisolated command line argument. Then they are translated to\nC:/dir/template and C:/dir/notexist as expected.\n\nThe tests become wrong-in-spirit only in combination with the\n\":(optional)\" prefix, because now the MSYS layer sees\n\":(optional)/c/dir/notexist\", which is not an absolute path. Yet, all\ntests with the \":(optional)\" prefix before this patch still work as\nexpected, because all expect the path to not exist. And from git.exe's\npoint of view, /c/dir/notexist does not exist.\n\nThe new test case would fail if $PWD was used, because it expects that\nthe file exists, but MSYS does not translate\n\":(optional)/c/dir/template\" to \":(optional)C:/dir/template\". So, we\nmust do the translation in the test script itself by using $(pwd). For\nconsistency, all other test cases with the \":(optional)\" should then use\n$(pwd), too. All remaining test cases could keep using $PWD, but I\nchanged them to $(pwd) for even more consistency.\n\n-- Hannes\n\n"},{"id":"529183","messageId":"5d780103-285b-4e6c-9b26-2a87609837cf@kdbg.org","threadId":"56234","inReplyTo":"A563E028-19E7-48A0-B538-82ACE821DB67@gmail.com","subject":"Re: [PATCH] t7500: fix tests with absolute path following \":(optional)\" on Windows","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2025-10-20T17:32:29Z","receivedAt":"2025-10-20T17:32:32Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 20.10.25 um 15:43 schrieb Ben Knoble:\n> Going forward I will probably stick with using pwd, given the\n> difference in platform behavior.\n$(pwd) is usually safe, but not always. If we have to look at every\ninstance anyway, we can use $PWD for efficiency if it does not matter,\nand $(pwd) only when it is necessary.\n\n> Is there a doc or test lint for that? If not, might be useful.\n\nIf this were documented somewhere, would you have found it and obeyed\nthe recommendations?\n\n-- Hannes\n\n"},{"id":"529185","messageId":"CAPig+cTP1ARNMQmxZh9_YO0pDOsFZ1Z2HTa+Bo=58O-voL9hXA@mail.gmail.com","threadId":"56234","inReplyTo":"A563E028-19E7-48A0-B538-82ACE821DB67@gmail.com","subject":"Re: [PATCH] t7500: fix tests with absolute path following \":(optional)\" on Windows","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-10-20T17:39:59Z","receivedAt":"2025-10-20T17:40:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Oct 20, 2025 at 9:44 AM Ben Knoble wrote:\n> > Le 20 oct. 2025 à 05:40, Johannes Sixt a écrit :\n> > On Windows, the MSYS layer translates absolute path names generated by\n> > a shell script from the POSIX style /c/dir/file to the Windows style\n> > C:/dir/file form that is understood by git.exe. This happens only when\n> > the absolute path stands on its own as a program argument or a value of\n> > an environment variable.\n> > [...]\n>\n> Going forward I will probably stick with using pwd, given the difference in platform behavior.\n>\n> Is there a doc or test lint for that? If not, might be useful.\n\nThe use of $PWD versus $(pwd) is documented in t/README:\n\n When a test checks for an absolute path that a git command\n generated, construct the expected value using $(pwd) rather than\n $PWD, $TEST_DIRECTORY, or $TRASH_DIRECTORY. It makes a difference\n on Windows, where the shell (MSYS bash) mangles absolute path\n names. For details, see the commit message of 4114156ae9.\n\n(Though, it might have been nicer if it described the behavior in more\ndetail rather than referring the reader elsewhere.)\n"},{"id":"529187","messageId":"xmqqtsztzbvo.fsf@gitster.g","threadId":"56234","inReplyTo":"5d780103-285b-4e6c-9b26-2a87609837cf@kdbg.org","subject":"Re: [PATCH] t7500: fix tests with absolute path following \":(optional)\" on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-20T18:06:19Z","receivedAt":"2025-10-20T18:06:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 20.10.25 um 15:43 schrieb Ben Knoble:\n>> Going forward I will probably stick with using pwd, given the\n>> difference in platform behavior.\n> $(pwd) is usually safe, but not always. If we have to look at every\n> instance anyway, we can use $PWD for efficiency if it does not matter,\n> and $(pwd) only when it is necessary.\n>\n>> Is there a doc or test lint for that? If not, might be useful.\n>\n> If this were documented somewhere, would you have found it and obeyed\n> the recommendations?\n\nI myself forget about it every time, even after getting bitten at\nleast 3 times in the past, maybe more.\n\nt/README has this.\n\n - When a test checks for an absolute path that a git command generated,\n construct the expected value using $(pwd) rather than $PWD,\n $TEST_DIRECTORY, or $TRASH_DIRECTORY. It makes a difference on\n Windows, where the shell (MSYS bash) mangles absolute path names.\n For details, see the commit message of 4114156ae9.\n\nIt is mentioned in t/README, I know it is mentioned in t/README, and\nI did re-read the part of t/README, every time I needed to decide\nbetween $PWD and $(pwd), but I still got it wrong 50% of the time\nX-<.\n"},{"id":"529203","messageId":"CALnO6CBwn-NP-ZdoaeOD37_VM8N4D-KKopm3nnf4a9b+9OiizA@mail.gmail.com","threadId":"56234","inReplyTo":"5d780103-285b-4e6c-9b26-2a87609837cf@kdbg.org","subject":"Re: [PATCH] t7500: fix tests with absolute path following \":(optional)\" on Windows","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-10-20T20:27:08Z","receivedAt":"2025-10-20T20:27:21Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Mon, Oct 20, 2025 at 1:32 PM Johannes Sixt wrote:\n>\n> Am 20.10.25 um 15:43 schrieb Ben Knoble:\n> > Going forward I will probably stick with using pwd, given the\n> > difference in platform behavior.\n> $(pwd) is usually safe, but not always. If we have to look at every\n> instance anyway, we can use $PWD for efficiency if it does not matter,\n> and $(pwd) only when it is necessary.\n>\n> > Is there a doc or test lint for that? If not, might be useful.\n>\n> If this were documented somewhere, would you have found it and obeyed\n> the recommendations?\n\nLikely yes, but I'll admit to being the exception rather than the rule\n(I like to read). A lint is more valuable in that it can at least be\nrun rather than searched for.\n\n-- \nD. Ben Knoble\n"},{"id":"529204","messageId":"CALnO6CCfDy19J-DTT4Vjp9EYf6M8sk5DpMjsm-Mp4_kNO9=kdg@mail.gmail.com","threadId":"56234","inReplyTo":"CALnO6CBwn-NP-ZdoaeOD37_VM8N4D-KKopm3nnf4a9b+9OiizA@mail.gmail.com","subject":"Re: [PATCH] t7500: fix tests with absolute path following \":(optional)\" on Windows","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-10-20T20:27:51Z","receivedAt":"2025-10-20T20:28:04Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Mon, Oct 20, 2025 at 4:27 PM D. Ben Knoble wrote:\n>\n> On Mon, Oct 20, 2025 at 1:32 PM Johannes Sixt wrote:\n> >\n> > Am 20.10.25 um 15:43 schrieb Ben Knoble:\n> > > Going forward I will probably stick with using pwd, given the\n> > > difference in platform behavior.\n> > $(pwd) is usually safe, but not always. If we have to look at every\n> > instance anyway, we can use $PWD for efficiency if it does not matter,\n> > and $(pwd) only when it is necessary.\n> >\n> > > Is there a doc or test lint for that? If not, might be useful.\n> >\n> > If this were documented somewhere, would you have found it and obeyed\n> > the recommendations?\n>\n> Likely yes, but I'll admit to being the exception rather than the rule\n> (I like to read). A lint is more valuable in that it can at least be\n> run rather than searched for.\n\nAch, and yet… I clearly didn't ;) hence the lint\n\n-- \nD. Ben Knoble\n"},{"id":"530082","messageId":"CALnO6CC=FFuMmBfJPzunUqDOBMBtmXm3i73y9M9LgRrhxzrs9g@mail.gmail.com","threadId":"56234","inReplyTo":"e8755a04-bd44-4ead-ba44-c603bffcc75e@gmail.com","subject":"Re: [PATCH v2 3/3] parseopt: values of pathname type can be prefixed with :(optional)","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-11-02T16:20:27Z","receivedAt":"2025-11-02T16:20:41Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Hi Phillip, apologies for the long delay.\n\nOn Tue, Sep 30, 2025 at 11:26 AM Phillip Wood wrote:\n>\n> Hi Ben\n>\n> On 28/09/2025 22:29, D. Ben Knoble wrote:\n> > From: Junio C Hamano \n> >\n> > In the previous step, we introduced an optional filename that can be\n> > given to a configuration variable, and nullify the fact that such a\n> > configuration setting even existed if the named path is missing or\n> > empty.\n> >\n> > Let's do the same for command line options that name a pathname.\n>\n> Sounds sensible\n>\n> > +Magic filename options\n>\n> I assume we're calling these \"magic\" to match to pathspec \"magic\"\n> options? I wonder if that is a good idea but I don't have a better\n> suggestion.\n\nYeah, best I could come up with.\n\n> > +~~~~~~~~~~~~~~~~~~~~~~\n> > +Options that take a filename allow a prefix `:(optional)`. For example:\n> > +\n> > +----------------------------\n> > +git commit -F :(optional)COMMIT_EDITMSG\n> > +# if COMMIT_EDITMSG does not exist, equivalent to\n>\n> This doesn't quite scan for me, maybe s/, /, it is/ ?\n\nWill include in a follow-up series now this has been merged.\n\n> > +git commit\n> > +----------------------------\n> > +\n> > +Like with configuration values, if the named file is missing Git behaves as if\n>\n> I'd drop \"with\" here\n\n\"Like configuration values\" seems strange since the subject is\n\"Git\"—other ideas?\n\n> > +the option was not given at all. See \"Values\" in linkgit:git-config[1].\n> > +\n>\n> > @@ -209,21 +208,31 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n> > case OPTION_FILENAME:\n> > {\n> > const char *value;\n> > -\n> > - FREE_AND_NULL(*(char **)opt->value);\n> > -\n> > - err = 0;\n> > + int is_optional;\n>\n> This can be a bool as in the last patch.\n\nAgreed.\n\n> > if (unset)\n> > value = NULL;\n> > else if (opt->flags & PARSE_OPT_OPTARG && !p->opt)\n> > - value = (const char *) opt->defval;\n> > - else\n> > - err = get_arg(p, opt, flags, &value);\n> > + value = (char *)opt->defval;\n>\n> I'm not sure why we're changing the cast here (or why we need one in the\n> first place assuming opt->defval is \"void*\")\n\nIt looks like opt->defval is intpr_t ? At any rate, I'm not sure why\nthe const was dropped here either. Might be an artifact of carrying an\nold patch forward?\n\nA quick pickaxe search says the const qualifier is from df217ed643\n(parse-opts: add OPT_FILENAME and transition builtins, 2009-05-23),\nunmodified by cf8c4237eb (parse-options: free previous value of\n`OPTION_FILENAME`, 2024-09-26). The original patch is from\nhttps://lore.kernel.org/git/20241014204427.1712182-4-gitster@pobox.com/,\nI think, so may just be a typo. Will fix.\n\n> > + else {\n> > + int err = get_arg(p, opt, flags, &value);\n> > + if (err)\n> > + return err;\n> > + }\n> > + if (!value)\n> > + return 0;\n> >\n> > - if (!err)\n> > - *(char **)opt->value = fix_filename(p->prefix, value);\n> > - return err;\n> > + is_optional = skip_prefix(value, \":(optional)\", &value);\n> > + if (!value)\n> > + is_optional = 0;\n>\n> I'm struggling to see how value can be NULL here as we return early if\n> it NULL before calling skip_prefix()\n\nDoesn't the \"skip_prefix\" above write into value? So I think if\n\"value\" is exactly the string \":(optional)\", then after the call to\nskip_prefix it points at the null terminator.\n\n> > + value = fix_filename(p->prefix, value);\n> > + if (is_optional && is_empty_or_missing_file(value)) {\n> > + free((char *)value);\n>\n> I think we want to call is_missing_file() here. If the file is missing\n> then we do nothing which matches the documentation above - Good.\n\nAgreed! Missed this when editing the patches. Will fix.\n"},{"id":"530083","messageId":"CALnO6CC02MDThBJZg47ecZQnzdEE+tXPBcNzregduPvgkASceg@mail.gmail.com","threadId":"56234","inReplyTo":"a687ec17-8ee4-428e-bae5-063716d59a08@gmail.com","subject":"Re: [PATCH v2 2/3] config: values of pathname type can be prefixed with :(optional)","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-11-02T16:20:30Z","receivedAt":"2025-11-02T16:20:44Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Hi Phillip\n\nOn Tue, Sep 30, 2025 at 11:26 AM Phillip Wood wrote:\n>\n> Hi Ben\n>\n> On 28/09/2025 22:29, D. Ben Knoble wrote:\n> > From: Junio C Hamano \n> >\n> > Sometimes people want to specify additional configuration data\n> > as \"best effort\" basis. Maybe commit.template configuration file points\n> > at somewhere in ~/template/ but on a particular system, the file may not\n> > exist and the user may be OK without using the template in such a case.\n> >\n> > When the value given to a configuration variable whose type is\n> > pathname wants to signal such an optional file, it can be marked by\n> > prepending \":(optional)\" in front of it. Such a setting that is\n> > marked optional would avoid getting the command barf for a missing\n> > file, as an optional configuration setting that names a missing\n> > file is not even seen.\n>\n> I think this would be a useful addition, we've had several people\n> wanting to make blame.ignoreRevsFile optional and this provides a\n> general way to do that.\n>\n> > --- a/config.c\n> > +++ b/config.c\n> > @@ -1279,11 +1279,23 @@ int git_config_string(char **dest, const char *var, const char *value)\n> >\n> > int git_config_pathname(char **dest, const char *var, const char *value)\n> > {\n> > + int is_optional;\n>\n> This could be bool rather than int, the rest of the implementation looks\n> good.\n\nAgreed. For now I've split this change and the parseopt change to bool\nas separate commits, but I'm indifferent to making them a single\nchange.\n\n>\n> > --- a/t/t7500-commit-template-squash-signoff.sh\n> > +++ b/t/t7500-commit-template-squash-signoff.sh\n> > @@ -46,6 +46,15 @@ commit_msg_is ()\n> > )\n> > '\n> >\n> > +test_expect_success 'nonexistent optional template file in config' '\n> > + test_config commit.template \":(optional)$PWD\"/notexist &&\n> > + (\n> > + GIT_EDITOR=\"echo hello >\\\"\\$1\\\"\" &&\n>\n> when git runs the editor this will be expanded to\n>\n> sh -c 'echo hello >\"$1\" \"$@\"' 'echo hello >\"$1\"' path/to/file\n>\n> I think it should be\n>\n> GIT_EDITOR=\"echo hello >\"\n>\n> instead\n> > + export GIT_EDITOR &&\n> > + git commit --allow-empty\n>\n> Maybe I'm missing something but don't we want to ensure that we have a\n> non-empty message here? Also as it is a single command we can avoid the\n> subshell with\n>\n> GIT_EDITOR=\"echo hello >\" git commit\n>\n> Thanks\n>\n> Phillip\n\nGreat catch, thanks. I've certainly had some trouble with this\nexpansion before [1]. It looks like this has been fixed in the version\nthat was merged, so I'll avoid touching it further for now. And thanks\nalso to Junio for the updates here.\n\n[1]:\n"},{"id":"530084","messageId":"CAPig+cQLri3m9398R0JEf2fafKVkZBvOdxvpg=xPF2aZ6ayDvQ@mail.gmail.com","threadId":"56234","inReplyTo":"CALnO6CC=FFuMmBfJPzunUqDOBMBtmXm3i73y9M9LgRrhxzrs9g@mail.gmail.com","subject":"Re: [PATCH v2 3/3] parseopt: values of pathname type can be prefixed with :(optional)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-11-03T00:10:20Z","receivedAt":"2025-11-03T00:10:32Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Nov 2, 2025 at 11:20 AM D. Ben Knoble\n wrote:\n> On Tue, Sep 30, 2025 at 11:26 AM Phillip Wood wrote:\n> > On 28/09/2025 22:29, D. Ben Knoble wrote:\n> > > + is_optional = skip_prefix(value, \":(optional)\", &value);\n> > > + if (!value)\n> > > + is_optional = 0;\n> >\n> > I'm struggling to see how value can be NULL here as we return early if\n> > it NULL before calling skip_prefix()\n>\n> Doesn't the \"skip_prefix\" above write into value? So I think if\n> \"value\" is exactly the string \":(optional)\", then after the call to\n> skip_prefix it points at the null terminator.\n\nI haven't particularly been following this topic, but your response\nsuggests that you're reading the code as if it says:\n\n if (!*value)\n is_optional = 0;\n\nwhereas, Philip is reading the code as written, which lacks the `*` dereference.\n"},{"id":"530209","messageId":"CALnO6CCDuUNiRTKbuRtJ6nY6OsxqGKvqzzZgYDOqTPZjEJ4MjA@mail.gmail.com","threadId":"56234","inReplyTo":"CAPig+cQLri3m9398R0JEf2fafKVkZBvOdxvpg=xPF2aZ6ayDvQ@mail.gmail.com","subject":"Re: [PATCH v2 3/3] parseopt: values of pathname type can be prefixed with :(optional)","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-11-04T18:22:26Z","receivedAt":"2025-11-04T18:22:39Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Sun, Nov 2, 2025 at 7:10 PM Eric Sunshine wrote:\n>\n> On Sun, Nov 2, 2025 at 11:20 AM D. Ben Knoble\n> wrote:\n> > On Tue, Sep 30, 2025 at 11:26 AM Phillip Wood wrote:\n> > > On 28/09/2025 22:29, D. Ben Knoble wrote:\n> > > > + is_optional = skip_prefix(value, \":(optional)\", &value);\n> > > > + if (!value)\n> > > > + is_optional = 0;\n> > >\n> > > I'm struggling to see how value can be NULL here as we return early if\n> > > it NULL before calling skip_prefix()\n> >\n> > Doesn't the \"skip_prefix\" above write into value? So I think if\n> > \"value\" is exactly the string \":(optional)\", then after the call to\n> > skip_prefix it points at the null terminator.\n>\n> I haven't particularly been following this topic, but your response\n> suggests that you're reading the code as if it says:\n>\n> if (!*value)\n> is_optional = 0;\n>\n> whereas, Philip is reading the code as written, which lacks the `*` dereference.\n\nIndeed, thanks\n"}]}