[PATCH 0/1] blame: Skip missing ignore-revs file

[PATCH 0/1] blame: Skip missing ignore-revs file

[Noah Pendleton]

Setting a global `blame.ignoreRevsFile` can be convenient, since I
usually use `.git-blame-ignore-revs` in repos. If the file is missing,
though, `git blame` exits with failure. This patch changes it to skip
over non-existent ignore-rev files instead of erroring.


Noah Pendleton (1):
  blame: skip missing ignore-revs-file's

 Documentation/blame-options.txt |  2 +-
 Documentation/config/blame.txt  |  3 ++-
 builtin/blame.c                 |  2 +-
 t/t8013-blame-ignore-revs.sh    | 10 ++++++----
 4 files changed, 10 insertions(+), 7 deletions(-)

-- 
2.32.0

[PATCH 0/1] blame: Skip missing ignore-revs file

[Noah Pendleton]

Setting a global `blame.ignoreRevsFile` can be convenient, since I
usually use `.git-blame-ignore-revs` in repos. If the file is missing,
though, `git blame` exits with failure. This patch changes it to skip
over non-existent ignore-rev files instead of erroring.


Noah Pendleton (1):
  blame: skip missing ignore-revs-file's

 Documentation/blame-options.txt |  2 +-
 Documentation/config/blame.txt  |  3 ++-
 builtin/blame.c                 |  2 +-
 t/t8013-blame-ignore-revs.sh    | 10 ++++++----
 4 files changed, 10 insertions(+), 7 deletions(-)

-- 
2.32.0

Re: [PATCH 0/1] blame: Skip missing ignore-revs file

[Junio C Hamano]

Noah Pendleton <noah.pendleton@gmail.com> writes:

> Setting a global `blame.ignoreRevsFile` can be convenient, since I
> usually use `.git-blame-ignore-revs` in repos. If the file is missing,
> though, `git blame` exits with failure. This patch changes it to skip
> over non-existent ignore-rev files instead of erroring.

That cuts both ways, though.  Failing upon missing configuration
file is a way to catch misconfiguration that is hard to diagnose.

I wonder if we can easily learn where the configuration variable
came from in the codepath that diagnoses it as a misconfiguration.

If it came from a per-repo configuration and names a non-existent
file, it clearly is a misconfiguration that we want to flag as an
error.  Even if it came from a per-user configuration, if it was
specified in a conditionally included file, it is likely to be a
misconfiguration.  If it came from a per-user configuration that
applies without any condition, it can be a good convenience feature
to silently (or with a warning) ignore missing file.

Re: [PATCH 0/1] blame: Skip missing ignore-revs file

[Noah Pendleton]

Thanks for the quick response!

Very good point about no longer catching misconfiguration. For
detecting provenance of a setting, I think we'd need to tag the config
options with it when they're loaded, possibly in 'struct
config_set_element' or similar. What do you think about instead
emitting a warning message on stderr in the case of misconfiguration,
but still continuing? Eg:

diff --git a/builtin/blame.c b/builtin/blame.c
index e5b45eddf4..6ee8f29313 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -835,7 +835,9 @@ static void build_ignorelist(struct blame_scoreboard *sb,
  for_each_string_list_item(i, ignore_revs_file_list) {
  if (!strcmp(i->string, ""))
  oidset_clear(&sb->ignore_list);
- else if (file_exists(i->string))
+ else if (!file_exists(i->string))
+ warning(_("skipping ignore-revs-file %s"), i->string);
+ else
  oidset_parse_file_carefully(&sb->ignore_list, i->string,
     peel_to_commit_oid, sb);
  }

On Sat, Aug 7, 2021, 16:58 Junio C Hamano <gitster@pobox.com> wrote:
>
> Noah Pendleton <noah.pendleton@gmail.com> writes:
>
> > Setting a global `blame.ignoreRevsFile` can be convenient, since I
> > usually use `.git-blame-ignore-revs` in repos. If the file is missing,
> > though, `git blame` exits with failure. This patch changes it to skip
> > over non-existent ignore-rev files instead of erroring.
>
> That cuts both ways, though.  Failing upon missing configuration
> file is a way to catch misconfiguration that is hard to diagnose.
>
> I wonder if we can easily learn where the configuration variable
> came from in the codepath that diagnoses it as a misconfiguration.
>
> If it came from a per-repo configuration and names a non-existent
> file, it clearly is a misconfiguration that we want to flag as an
> error.  Even if it came from a per-user configuration, if it was
> specified in a conditionally included file, it is likely to be a
> misconfiguration.  If it came from a per-user configuration that
> applies without any condition, it can be a good convenience feature
> to silently (or with a warning) ignore missing file.

Re: [PATCH 0/1] blame: Skip missing ignore-revs file

[Junio C Hamano]

Noah Pendleton <noah.pendleton@gmail.com> writes:

> Very good point about no longer catching misconfiguration. For
> detecting provenance of a setting, I think we'd need to tag the config
> options with it when they're loaded, possibly in 'struct
> config_set_element' or similar. What do you think about instead
> emitting a warning message on stderr in the case of misconfiguration,
> but still continuing? Eg:

Unconditionally continuing with just a warning would not be a good
approach for at least two reasons.  (1) the user may truly have
intended that ignoreRevsFile to be optional, in which case the
warning is a nuisance that does not add any value, and (2) it truly
may be a misconfiguration that the named file did not exist, but the
output from "git blame" will wipe the display and the warning would
very well go unnoticed (or more likely the user may notice that
there was a warning, but it will go away before the user has a
chance to really read it, which is a lot worse and frustrating
experience).

I think an easier way out is to introduce a new configuration
variable blame.ignoreRevsFileIsOptional which takes a boolean value,
and when it is set to true, silently ignore when the named file does
not exist without any warning.  When the variable is set to false
(or the variable does not exist), we can keep the current behaviour
of noticing a misconfigured blame.ignoreRevsFile and error out.

That way, the current users who rely on the typo detection feature
can keep relying on it, and those who want to make it optional can
do so without getting annoyed by a warning.

[PATCH v2] blame: add config `blame.ignoreRevsFileIsOptional`

[Noah Pendleton]

Setting the config option `blame.ignoreRevsFile` globally to eg
`.git-blame-ignore-revs` causes `git blame` to error when the file
doesn't exist in the current repository:

```
fatal: could not open object name list: .git-blame-ignore-revs
```

Add a new config option, `blame.ignoreRevsFileIsOptional`, that when set
to true, `git blame` will silently ignore any missing ignoreRevsFile's.

Signed-off-by: Noah Pendleton <noah.pendleton@gmail.com>
---
Reworked this patch to add a new config
`blame.ignoreRevsFileIsOptional`, which controls whether missing
files specified by ignoreRevsFile cause an error or are silently
ignored.

Updated tests and docs to match.

 Documentation/blame-options.txt |  3 ++-
 Documentation/config/blame.txt  |  5 +++++
 builtin/blame.c                 |  7 ++++++-
 t/t8013-blame-ignore-revs.sh    | 14 ++++++++++----
 4 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index 117f4cf806..199a28ab79 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -134,7 +134,8 @@ take effect.
 	`fsck.skipList`.  This option may be repeated, and these files will be
 	processed after any files specified with the `blame.ignoreRevsFile` config
 	option.  An empty file name, `""`, will clear the list of revs from
-	previously processed files.
+	previously processed files. If `blame.ignoreRevsFileIsOptional` is true,
+	missing files will be silently ignored.
 
 -h::
 	Show help message.
diff --git a/Documentation/config/blame.txt b/Documentation/config/blame.txt
index 4d047c1790..2aae851e4b 100644
--- a/Documentation/config/blame.txt
+++ b/Documentation/config/blame.txt
@@ -27,6 +27,11 @@ blame.ignoreRevsFile::
 	file names will reset the list of ignored revisions.  This option will
 	be handled before the command line option `--ignore-revs-file`.
 
+blame.ignoreRevsFileIsOptional::
+	Silently skip missing files specified by ignoreRevsFile or the command line
+	option `--ignore-revs-file`. If unset, or set to false, missing files will
+	cause a nonrecoverable error.
+
 blame.markUnblamableLines::
 	Mark lines that were changed by an ignored revision that we could not
 	attribute to another commit with a '*' in the output of
diff --git a/builtin/blame.c b/builtin/blame.c
index 641523ff9a..df132b34ce 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -56,6 +56,7 @@ static int coloring_mode;
 static struct string_list ignore_revs_file_list = STRING_LIST_INIT_NODUP;
 static int mark_unblamable_lines;
 static int mark_ignored_lines;
+static int ignorerevsfileisoptional;
 
 static struct date_mode blame_date_mode = { DATE_ISO8601 };
 static size_t blame_date_width;
@@ -715,6 +716,9 @@ static int git_blame_config(const char *var, const char *value, void *cb)
 		string_list_insert(&ignore_revs_file_list, str);
 		return 0;
 	}
+	if (!strcmp(var, "blame.ignorerevsfileisoptional")) {
+		ignorerevsfileisoptional = git_config_bool(var, value);
+	}
 	if (!strcmp(var, "blame.markunblamablelines")) {
 		mark_unblamable_lines = git_config_bool(var, value);
 		return 0;
@@ -835,7 +839,8 @@ static void build_ignorelist(struct blame_scoreboard *sb,
 	for_each_string_list_item(i, ignore_revs_file_list) {
 		if (!strcmp(i->string, ""))
 			oidset_clear(&sb->ignore_list);
-		else
+		/* skip non-existent files if ignorerevsfileisoptional is set */
+		else if (!ignorerevsfileisoptional || file_exists(i->string))
 			oidset_parse_file_carefully(&sb->ignore_list, i->string,
 						    peel_to_commit_oid, sb);
 	}
diff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh
index b18633dee1..f789426cbf 100755
--- a/t/t8013-blame-ignore-revs.sh
+++ b/t/t8013-blame-ignore-revs.sh
@@ -127,18 +127,24 @@ test_expect_success override_ignore_revs_file '
 	grep -E "^[0-9a-f]+ [0-9]+ 2" blame_raw | sed -e "s/ .*//" >actual &&
 	test_cmp expect actual
 	'
-test_expect_success bad_files_and_revs '
+test_expect_success bad_revs '
 	test_must_fail git blame file --ignore-rev NOREV 2>err &&
 	test_i18ngrep "cannot find revision NOREV to ignore" err &&
 
-	test_must_fail git blame file --ignore-revs-file NOFILE 2>err &&
-	test_i18ngrep "could not open.*: NOFILE" err &&
-
 	echo NOREV >ignore_norev &&
 	test_must_fail git blame file --ignore-revs-file ignore_norev 2>err &&
 	test_i18ngrep "invalid object name: NOREV" err
 '
 
+# Non-existent ignore-revs-file should fail unless
+# blame.ignoreRevsFileIsOptional is set
+test_expect_success bad_file '
+	test_must_fail git blame file --ignore-revs-file NOFILE &&
+
+	git config --add blame.ignorerevsfileisoptional true &&
+	git blame file --ignore-revs-file NOFILE
+'
+
 # For ignored revs that have added 'unblamable' lines, mark those lines with a
 # '*'
 # 	A--B--X--Y
-- 
2.32.0

Re: [PATCH 0/1] blame: Skip missing ignore-revs file

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I think an easier way out is to introduce a new configuration
> variable blame.ignoreRevsFileIsOptional which takes a boolean value,
> and when it is set to true, silently ignore when the named file does
> not exist without any warning.  When the variable is set to false
> (or the variable does not exist), we can keep the current behaviour
> of noticing a misconfigured blame.ignoreRevsFile and error out.
>
> That way, the current users who rely on the typo detection feature
> can keep relying on it, and those who want to make it optional can
> do so without getting annoyed by a warning.

A bit more ambitious might want to consider another more generally
applicable avenue, which would help the userbase a lot more, before
continuing.

We start from the realization that this is not the only
configuration variable that specifies a filename that could be
missing.  There may be other variables that name files to be used
("git config --help" would hopefully be the most comprehensive, but
"git grep -e git_config_pathname \*.c" would give us quicker
starting point to gauge how big an impact to the system we would be
talking about).

What do the codepaths that use these variables do when they find
that the named files are missing?  Do some of them die, some
others just warn, and yet some others silently ignore?  Would such
an inconsistency hurt our users?

Among the ones that die, are there ones that could reasonably
continue as if the configuration variable weren't there and no file
was specified (i.e. similar to what you want blame.ignoreRevsFile to
do)?  Among the ones that are silently ignored, are there ones that
may benefit by having a typo-detection?  Do all of them benefit if
the behaviour upon missing files can be configurable by the end-user?

Depending on the answers to the above questions, it might be that it
is not a desirable approach to add "blame.ignoreRevsFileIsOptional"
configuration variable, as all the existing configuration variables
that name files would want to add their own.  We might be better off
inventing a syntax for the value of blame.ignoreRevsFile (and other
variables that name files) to mark if the file is optional (i.e.
silently ignore if the named file does not exist) or required (i.e.
diagnose as a configuration error).  For example, we may borrow from
the "magic" syntax for pathspecs that begin with ":(", with comma
separated "magic" keywords and ends with ")" and specify optional
pathname configuration like so:

    [blame] ignoreRevsFile = :(optional).gitignorerevs

and teach the config parser to pretend as if it saw nothing when it
notices that the named file is missing.  That approach would cover
not just this single variable, but other variables that are parsed
using git_config_pathname() may benefit the same way (of course, the
callsites for git_config_pathmame() must be inspected and adjusted
for this to happen).

Thanks.

Re: [PATCH 0/1] blame: Skip missing ignore-revs file

[Noah Pendleton]

Very good point- I see about 21 call sites for `git_config_pathname`,
plus a few others (`git_config_get_pathname`) that bottom out in the
same function. I could see the utility of optional paths for some of
them: for example, `commit.template`, `core.excludesfile`. Some of the
others seem a little more ambiguous, eg `http.sslcert` probably wants
to always fail in case of missing file.

There seems to be a mix of fail-hard on invalid paths, printing a
warning message and skipping, and silently ignoring.

Hard for me to predict what the least confusing behavior is around
path configuration values, though, so maybe adding support for the
`:(optional)` (and maybe additionally a `:(required)`) tag across the
board to pathname configs is the right move.

That patch might be beyond what I'm capable of, though I'm happy to
put up a draft that applies it to the original `ignoreRevsFile` case
as a starting point.

On Sun, Aug 8, 2021 at 1:50 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > I think an easier way out is to introduce a new configuration
> > variable blame.ignoreRevsFileIsOptional which takes a boolean value,
> > and when it is set to true, silently ignore when the named file does
> > not exist without any warning.  When the variable is set to false
> > (or the variable does not exist), we can keep the current behaviour
> > of noticing a misconfigured blame.ignoreRevsFile and error out.
> >
> > That way, the current users who rely on the typo detection feature
> > can keep relying on it, and those who want to make it optional can
> > do so without getting annoyed by a warning.
>
> A bit more ambitious might want to consider another more generally
> applicable avenue, which would help the userbase a lot more, before
> continuing.
>
> We start from the realization that this is not the only
> configuration variable that specifies a filename that could be
> missing.  There may be other variables that name files to be used
> ("git config --help" would hopefully be the most comprehensive, but
> "git grep -e git_config_pathname \*.c" would give us quicker
> starting point to gauge how big an impact to the system we would be
> talking about).
>
> What do the codepaths that use these variables do when they find
> that the named files are missing?  Do some of them die, some
> others just warn, and yet some others silently ignore?  Would such
> an inconsistency hurt our users?
>
> Among the ones that die, are there ones that could reasonably
> continue as if the configuration variable weren't there and no file
> was specified (i.e. similar to what you want blame.ignoreRevsFile to
> do)?  Among the ones that are silently ignored, are there ones that
> may benefit by having a typo-detection?  Do all of them benefit if
> the behaviour upon missing files can be configurable by the end-user?
>
> Depending on the answers to the above questions, it might be that it
> is not a desirable approach to add "blame.ignoreRevsFileIsOptional"
> configuration variable, as all the existing configuration variables
> that name files would want to add their own.  We might be better off
> inventing a syntax for the value of blame.ignoreRevsFile (and other
> variables that name files) to mark if the file is optional (i.e.
> silently ignore if the named file does not exist) or required (i.e.
> diagnose as a configuration error).  For example, we may borrow from
> the "magic" syntax for pathspecs that begin with ":(", with comma
> separated "magic" keywords and ends with ")" and specify optional
> pathname configuration like so:
>
>     [blame] ignoreRevsFile = :(optional).gitignorerevs
>
> and teach the config parser to pretend as if it saw nothing when it
> notices that the named file is missing.  That approach would cover
> not just this single variable, but other variables that are parsed
> using git_config_pathname() may benefit the same way (of course, the
> callsites for git_config_pathmame() must be inspected and adjusted
> for this to happen).
>
> Thanks.
>

Re: [PATCH 0/1] blame: Skip missing ignore-revs file

[Junio C Hamano]

Noah Pendleton <noah.pendleton@gmail.com> writes:

> Very good point- I see about 21 call sites for `git_config_pathname`,
> plus a few others (`git_config_get_pathname`) that bottom out in the
> same function. I could see the utility of optional paths for some of
> them: for example, `commit.template`, `core.excludesfile`. Some of the
> others seem a little more ambiguous, eg `http.sslcert` probably wants
> to always fail in case of missing file.

Thanks for already doing initial surveillance.  Very useful.

> There seems to be a mix of fail-hard on invalid paths, printing a
> warning message and skipping, and silently ignoring.
>
> Hard for me to predict what the least confusing behavior is around
> path configuration values, though, so maybe adding support for the
> `:(optional)` (and maybe additionally a `:(required)`) tag across the
> board to pathname configs is the right move.

I originally hoped only ":(optional)" would be necessary, but to
keep the continuity in behaviour for those currently that do not die
upon seeing a missing file, we probably should treat an unadorned
value as asking for the "traditional" behaviour, at least in the
shorter term, and allow those users who want to detect typos to
tighten the rule using ":(required)".  I dunno.

> That patch might be beyond what I'm capable of, though I'm happy to
> put up a draft that applies it to the original `ignoreRevsFile` case
> as a starting point.

Thanks for an offer.  We are not in a hurry (especially during the
pre-release feature freeze), and hopefully this discussion would
pique other developers' interest to nudge them to help ;-)

Re: [PATCH 0/1] blame: Skip missing ignore-revs file

[Thranur Andul]

On 07/08/2021 22:58, Junio C Hamano wrote:
> Noah Pendleton <noah.pendleton@gmail.com> writes:
> 
> 
> That cuts both ways, though.  Failing upon missing configuration
> file is a way to catch misconfiguration that is hard to diagnose.
> 
> I wonder if we can easily learn where the configuration variable
> came from in the codepath that diagnoses it as a misconfiguration.
> 
> If it came from a per-repo configuration and names a non-existent
> file, it clearly is a misconfiguration that we want to flag as an
> error.  Even if it came from a per-user configuration, if it was
> specified in a conditionally included file, it is likely to be a
> misconfiguration.  If it came from a per-user configuration that
> applies without any condition, it can be a good convenience feature
> to silently (or with a warning) ignore missing file.
>
I am very interested in this feature, but I'd like to add another point 
to the discussion: in the case of ignoreRevsFile in particular, no one 
creates a repository with such a file; it is always added later. 
However, when bisecting (a typical usage scenario for git-blame), we may 
end up returning back to a point _before_ the file had been added, and 
then, git-blame fails. This often happens to me, and I am then forced to 
`touch` the file to create it again, only to ensure git-blame keeps 
working. And then, when I want to return to the HEAD commit, the file 
must be erased again otherwise there is a conflict. So, for me, the 
"ignore if absent" behavior seems to me like it should be the default.

[PATCH 0/3] specifying a file that can optionally exist

[Junio C Hamano]

In a discussion a few years ago (cf. <xmqq5ywehb69.fsf@gitster.g>),
we wondered if it is a good idea to allow a configuration variable
(or a command line option for that matter) to name an "optional"
file, and pretend as if such a configuration setting or a command
line option was not even given when the named file did not exist or
empty.

Here are a few patches I did while passing time without anything
better to do.

Even though I updated the documentation for the configuration
variables, I didn't find a good central place to do the same for
parse-options.  I'll leave it as an exercise for the readers ;-).

The first patch is a preliminary clean-up for test script that is
used to house tests added by the later patches.

The second patch is for configuration variables, and the last one is
for command line options.

Junio C Hamano (3):
  t7500: make each piece more independent
  config: values of pathname type can be prefixed with :(optional)
  parseopt: values of pathname type can be prefixed with :(optional)

 Documentation/config.txt                  |  5 +++-
 config.c                                  | 16 +++++++++--
 parse-options.c                           | 31 +++++++++++++-------
 t/t7500-commit-template-squash-signoff.sh | 35 +++++++++++++++++------
 4 files changed, 65 insertions(+), 22 deletions(-)

-- 
2.47.0-148-g19c85929c5

[PATCH 1/3] t7500: make each piece more independent

[Junio C Hamano]

These tests prepare the working tree & index state to have something
to be committed, and try a sequence of "test_must_fail git commit".
If an earlier one did not fail by a bug, a later one will fail for
a wrong reason (namely, "nothing to commit").

Give them "--allow-empty" to make sure that they would work even
when there is nothing to commit by accident.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t7500-commit-template-squash-signoff.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index 4dca8d97a7..4927b7260d 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -50,33 +50,33 @@ test_expect_success 'nonexistent template file in config should return error' '
 TEMPLATE="$PWD"/template
 
 test_expect_success 'unedited template should not commit' '
-	echo "template line" > "$TEMPLATE" &&
-	test_must_fail git commit --template "$TEMPLATE"
+	echo "template line" >"$TEMPLATE" &&
+	test_must_fail git commit --allow-empty --template "$TEMPLATE"
 '
 
 test_expect_success 'unedited template with comments should not commit' '
-	echo "# comment in template" >> "$TEMPLATE" &&
-	test_must_fail git commit --template "$TEMPLATE"
+	echo "# comment in template" >>"$TEMPLATE" &&
+	test_must_fail git commit --allow-empty --template "$TEMPLATE"
 '
 
 test_expect_success 'a Signed-off-by line by itself should not commit' '
 	(
 		test_set_editor "$TEST_DIRECTORY"/t7500/add-signed-off &&
-		test_must_fail git commit --template "$TEMPLATE"
+		test_must_fail git commit --allow-empty --template "$TEMPLATE"
 	)
 '
 
 test_expect_success 'adding comments to a template should not commit' '
 	(
 		test_set_editor "$TEST_DIRECTORY"/t7500/add-comments &&
-		test_must_fail git commit --template "$TEMPLATE"
+		test_must_fail git commit --allow-empty --template "$TEMPLATE"
 	)
 '
 
 test_expect_success 'adding real content to a template should commit' '
 	(
 		test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
-		git commit --template "$TEMPLATE"
+		git commit --allow-empty --template "$TEMPLATE"
 	) &&
 	commit_msg_is "template linecommit message"
 '
-- 
2.47.0-148-g19c85929c5

[PATCH 2/3] config: values of pathname type can be prefixed with :(optional)

[Junio C Hamano]

Sometimes people want to specify additional configuration data
as "best effort" basis.  Maybe commit.template configuration file points
at somewhere in ~/template/ but on a particular system, the file may not
exist and the user may be OK without using the template in such a case.

When the value given to a configuration variable whose type is
pathname wants to signal such an optional file, it can be marked by
prepending ":(optional)" in front of it.  Such a setting that is
marked optional would avoid getting the command barf for a missing
file, as an optional configuration setting that names a missing or
an empty file is not even seen.

cf. <xmqq5ywehb69.fsf@gitster.g>

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config.txt                  |  5 ++++-
 config.c                                  | 16 ++++++++++++++--
 t/t7500-commit-template-squash-signoff.sh |  9 +++++++++
 3 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8c0b3ed807..199e29ccea 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -358,7 +358,10 @@ compiled without runtime prefix support, the compiled-in prefix will be
 substituted instead. In the unlikely event that a literal path needs to
 be specified that should _not_ be expanded, it needs to be prefixed by
 `./`, like so: `./%(prefix)/bin`.
-
++
+If prefixed with `:(optional)`, the configuration variable is treated
+as if it does not exist, if the named path does not exist or names an
+empty file.
 
 Variables
 ~~~~~~~~~
diff --git a/config.c b/config.c
index a11bb85da3..4a060f1d82 100644
--- a/config.c
+++ b/config.c
@@ -1364,11 +1364,23 @@ int git_config_string(char **dest, const char *var, const char *value)
 
 int git_config_pathname(char **dest, const char *var, const char *value)
 {
+	int is_optional;
+	char *path;
+
 	if (!value)
 		return config_error_nonbool(var);
-	*dest = interpolate_path(value, 0);
-	if (!*dest)
+
+	is_optional = skip_prefix(value, ":(optional)", &value);
+	path = interpolate_path(value, 0);
+	if (!path)
 		die(_("failed to expand user dir in: '%s'"), value);
+
+	if (is_optional && is_empty_or_missing_file(path)) {
+		free(path);
+		return 0;
+	}
+
+	*dest = path;
 	return 0;
 }
 
diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index 4927b7260d..e28a79987d 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -46,6 +46,15 @@ test_expect_success 'nonexistent template file in config should return error' '
 	)
 '
 
+test_expect_success 'nonexistent optional template file in config' '
+	test_config commit.template ":(optional)$PWD"/notexist &&
+	(
+		GIT_EDITOR="echo hello >\"\$1\"" &&
+		export GIT_EDITOR &&
+		git commit --allow-empty
+	)
+'
+
 # From now on we'll use a template file that exists.
 TEMPLATE="$PWD"/template
 
-- 
2.47.0-148-g19c85929c5

[PATCH 3/3] parseopt: values of pathname type can be prefixed with :(optional)

[Junio C Hamano]

In the previous step, we introduced an optional filename that can be
given to a configuration variable, and nullify the fact that such a
configuration setting even existed if the named path is missing or
empty.

Let's do the same for command line options that name a pathname.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 parse-options.c                           | 31 +++++++++++++++--------
 t/t7500-commit-template-squash-signoff.sh | 12 ++++++++-
 2 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 33bfba0ed4..7a2a3b1f08 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -75,7 +75,6 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 {
 	const char *s, *arg;
 	const int unset = flags & OPT_UNSET;
-	int err;
 
 	if (unset && p->opt)
 		return error(_("%s takes no value"), optname(opt, flags));
@@ -131,21 +130,31 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 	case OPTION_FILENAME:
 	{
 		const char *value;
-
-		FREE_AND_NULL(*(char **)opt->value);
-
-		err = 0;
+		int is_optional;
 
 		if (unset)
 			value = NULL;
 		else if (opt->flags & PARSE_OPT_OPTARG && !p->opt)
-			value = (const char *) opt->defval;
-		else
-			err = get_arg(p, opt, flags, &value);
+			value = (char *)opt->defval;
+		else {
+			int err = get_arg(p, opt, flags, &value);
+			if (err)
+				return err;
+		}
+		if (!value)
+			return 0;
 
-		if (!err)
-			*(char **)opt->value = fix_filename(p->prefix, value);
-		return err;
+		is_optional = skip_prefix(value, ":(optional)", &value);
+		if (!value)
+			is_optional = 0;
+		value = fix_filename(p->prefix, value);
+		if (is_optional && is_empty_or_missing_file(value)) {
+			free((char *)value);
+		} else {
+			FREE_AND_NULL(*(char **)opt->value);
+			*(const char **)opt->value = value;
+		}
+		return 0;
 	}
 	case OPTION_CALLBACK:
 	{
diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index e28a79987d..c065f12baf 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -37,12 +37,22 @@ test_expect_success 'nonexistent template file should return error' '
 	)
 '
 
+test_expect_success 'nonexistent optional template file on command line' '
+	echo changes >> foo &&
+	git add foo &&
+	(
+		GIT_EDITOR="echo hello >\"\$1\"" &&
+		export GIT_EDITOR &&
+		git commit --template ":(optional)$PWD/notexist"
+	)
+'
+
 test_expect_success 'nonexistent template file in config should return error' '
 	test_config commit.template "$PWD"/notexist &&
 	(
 		GIT_EDITOR="echo hello >\"\$1\"" &&
 		export GIT_EDITOR &&
-		test_must_fail git commit
+		test_must_fail git commit --allow-empty
 	)
 '
 
-- 
2.47.0-148-g19c85929c5

Feature request: automatically read .git-blame-ignore-revs or allow global optional config

[Michael Grosser]

I have many repos where I use .git-blame-ignore-revs,
but I cannot set it globally because then I get
```
fatal: could not open object name list: .git-blame-ignore-revs
```
so please make it either the default for `git blame` to check that file,
or add a "blame: ignoreMissingFile: true" option so I an set
```
[blame]
  ignoreRevsFile = .git-blame-ignore-revs
  ignoreMissingFile: true
```
and can use this feature without constantly having to think about it

Re: Feature request: automatically read .git-blame-ignore-revs or allow global optional config

[Eric Sunshine]

On Fri, Apr 25, 2025 at 2:42 PM Michael Grosser
<grosser.michael@gmail.com> wrote:>
> I have many repos where I use .git-blame-ignore-revs,
> but I cannot set it globally because then I get
> ```
> fatal: could not open object name list: .git-blame-ignore-revs
> ```
> so please make it either the default for `git blame` to check that file,
> or add a "blame: ignoreMissingFile: true" option so I an set
> ```
> [blame]
>   ignoreRevsFile = .git-blame-ignore-revs
>   ignoreMissingFile: true
> ```
> and can use this feature without constantly having to think about it

Relevant threads:
https://lore.kernel.org/git/pull.1947.git.git.1745088194384.gitgitgadget@gmail.com/T/#u
https://lore.kernel.org/git/20241014204427.1712182-1-gitster@pobox.com/T/#u

Re: Feature request: automatically read .git-blame-ignore-revs or allow global optional config

[D. Ben Knoble]

On Fri, Apr 25, 2025 at 3:55 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Fri, Apr 25, 2025 at 2:42 PM Michael Grosser
> <grosser.michael@gmail.com> wrote:>
> > I have many repos where I use .git-blame-ignore-revs,
> > but I cannot set it globally because then I get
> > ```
> > fatal: could not open object name list: .git-blame-ignore-revs
> > ```
> > so please make it either the default for `git blame` to check that file,
> > or add a "blame: ignoreMissingFile: true" option so I an set
> > ```
> > [blame]
> >   ignoreRevsFile = .git-blame-ignore-revs
> >   ignoreMissingFile: true
> > ```
> > and can use this feature without constantly having to think about it
>
> Relevant threads:
> https://lore.kernel.org/git/pull.1947.git.git.1745088194384.gitgitgadget@gmail.com/T/#u
> https://lore.kernel.org/git/20241014204427.1712182-1-gitster@pobox.com/T/#u

Did that second set of patches ever go anywhere? It seems similar to
what Junio proposed in the first linked thread and possibly worth
resurrecting. (A log --grep :(optional) didn't find anything.)

-- 
D. Ben Knoble

Re: Feature request: automatically read .git-blame-ignore-revs or allow global optional config

[Eric Sunshine]

On Thu, May 1, 2025 at 2:00 PM D. Ben Knoble <ben.knoble@gmail.com> wrote:
> On Fri, Apr 25, 2025 at 3:55 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Fri, Apr 25, 2025 at 2:42 PM Michael Grosser
> > <grosser.michael@gmail.com> wrote:>
> > > so please make it either the default for `git blame` to check that file,
> > > or add a "blame: ignoreMissingFile: true" option so I an set
> >
> > Relevant threads:
> > https://lore.kernel.org/git/pull.1947.git.git.1745088194384.gitgitgadget@gmail.com/T/#u
> > https://lore.kernel.org/git/20241014204427.1712182-1-gitster@pobox.com/T/#u
>
> Did that second set of patches ever go anywhere? It seems similar to
> what Junio proposed in the first linked thread and possibly worth
> resurrecting. (A log --grep :(optional) didn't find anything.)

The patches Junio submitted[1] did not go anywhere. The topic sat in
his tree for several "What's Cooking" reports and then he dropped it
because it received no response from reviewers[2]. I agree that it
could be a useful enhancement if resurrected.

[1]: https://lore.kernel.org/git/20241014204427.1712182-1-gitster@pobox.com/T/#u
[2]: https://lore.kernel.org/git/CAPig+cT1BNXRotrz=rnVgvhQjZZwYgsAOQMonHFFTPfK-C0LOQ@mail.gmail.com/

[PATCH 0/3] specifying a file that can optionally exist

[Junio C Hamano]

In a discussion some years ago (cf. <xmqq5ywehb69.fsf@gitster.g>),
we wondered if it is a good idea to allow a configuration variable
(or a command line option for that matter) to name an "optional"
file, and pretend as if such a configuration setting or a command
line option was not even given when the named file did not exist or
empty.  Then I floated a set of patches to implement the feature,
but the topic did not get any traction and was dropped.

I am resurrecting the patches after seeing some interest in it in
recent discussion threads; it would be easier for people to
comment on, if they are in more recent parts of their mailbox.
I didn't change anything in the patch; they are verbatim copies
that I happened to have found lying somewhere in my filesystem.

Even though I updated the documentation for the configuration
variables, I didn't find a good central place to do the same for
parse-options.  I'll leave it as an exercise for the readers ;-).

The first patch is a preliminary clean-up for test script that is
used to house tests added by the later patches.

The second patch is for configuration variables, and the last one is
for command line options.

Junio C Hamano (3):
  t7500: make each piece more independent
  config: values of pathname type can be prefixed with :(optional)
  parseopt: values of pathname type can be prefixed with :(optional)

 Documentation/config.txt                  |  5 +++-
 config.c                                  | 16 +++++++++--
 parse-options.c                           | 31 +++++++++++++-------
 t/t7500-commit-template-squash-signoff.sh | 35 +++++++++++++++++------
 4 files changed, 65 insertions(+), 22 deletions(-)

-- 
2.47.0-148-g19c85929c5

[PATCH 1/3] t7500: make each piece more independent

[Junio C Hamano]

These tests prepare the working tree & index state to have something
to be committed, and try a sequence of "test_must_fail git commit".
If an earlier one did not fail by a bug, a later one will fail for
a wrong reason (namely, "nothing to commit").

Give them "--allow-empty" to make sure that they would work even
when there is nothing to commit by accident.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t7500-commit-template-squash-signoff.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index 4dca8d97a7..4927b7260d 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -50,33 +50,33 @@ test_expect_success 'nonexistent template file in config should return error' '
 TEMPLATE="$PWD"/template
 
 test_expect_success 'unedited template should not commit' '
-	echo "template line" > "$TEMPLATE" &&
-	test_must_fail git commit --template "$TEMPLATE"
+	echo "template line" >"$TEMPLATE" &&
+	test_must_fail git commit --allow-empty --template "$TEMPLATE"
 '
 
 test_expect_success 'unedited template with comments should not commit' '
-	echo "# comment in template" >> "$TEMPLATE" &&
-	test_must_fail git commit --template "$TEMPLATE"
+	echo "# comment in template" >>"$TEMPLATE" &&
+	test_must_fail git commit --allow-empty --template "$TEMPLATE"
 '
 
 test_expect_success 'a Signed-off-by line by itself should not commit' '
 	(
 		test_set_editor "$TEST_DIRECTORY"/t7500/add-signed-off &&
-		test_must_fail git commit --template "$TEMPLATE"
+		test_must_fail git commit --allow-empty --template "$TEMPLATE"
 	)
 '
 
 test_expect_success 'adding comments to a template should not commit' '
 	(
 		test_set_editor "$TEST_DIRECTORY"/t7500/add-comments &&
-		test_must_fail git commit --template "$TEMPLATE"
+		test_must_fail git commit --allow-empty --template "$TEMPLATE"
 	)
 '
 
 test_expect_success 'adding real content to a template should commit' '
 	(
 		test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
-		git commit --template "$TEMPLATE"
+		git commit --allow-empty --template "$TEMPLATE"
 	) &&
 	commit_msg_is "template linecommit message"
 '
-- 
2.47.0-148-g19c85929c5

[PATCH 2/3] config: values of pathname type can be prefixed with :(optional)

[Junio C Hamano]

Sometimes people want to specify additional configuration data
as "best effort" basis.  Maybe commit.template configuration file points
at somewhere in ~/template/ but on a particular system, the file may not
exist and the user may be OK without using the template in such a case.

When the value given to a configuration variable whose type is
pathname wants to signal such an optional file, it can be marked by
prepending ":(optional)" in front of it.  Such a setting that is
marked optional would avoid getting the command barf for a missing
file, as an optional configuration setting that names a missing or
an empty file is not even seen.

cf. <xmqq5ywehb69.fsf@gitster.g>

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config.txt                  |  5 ++++-
 config.c                                  | 16 ++++++++++++++--
 t/t7500-commit-template-squash-signoff.sh |  9 +++++++++
 3 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8c0b3ed807..199e29ccea 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -358,7 +358,10 @@ compiled without runtime prefix support, the compiled-in prefix will be
 substituted instead. In the unlikely event that a literal path needs to
 be specified that should _not_ be expanded, it needs to be prefixed by
 `./`, like so: `./%(prefix)/bin`.
-
++
+If prefixed with `:(optional)`, the configuration variable is treated
+as if it does not exist, if the named path does not exist or names an
+empty file.
 
 Variables
 ~~~~~~~~~
diff --git a/config.c b/config.c
index a11bb85da3..4a060f1d82 100644
--- a/config.c
+++ b/config.c
@@ -1364,11 +1364,23 @@ int git_config_string(char **dest, const char *var, const char *value)
 
 int git_config_pathname(char **dest, const char *var, const char *value)
 {
+	int is_optional;
+	char *path;
+
 	if (!value)
 		return config_error_nonbool(var);
-	*dest = interpolate_path(value, 0);
-	if (!*dest)
+
+	is_optional = skip_prefix(value, ":(optional)", &value);
+	path = interpolate_path(value, 0);
+	if (!path)
 		die(_("failed to expand user dir in: '%s'"), value);
+
+	if (is_optional && is_empty_or_missing_file(path)) {
+		free(path);
+		return 0;
+	}
+
+	*dest = path;
 	return 0;
 }
 
diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index 4927b7260d..e28a79987d 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -46,6 +46,15 @@ test_expect_success 'nonexistent template file in config should return error' '
 	)
 '
 
+test_expect_success 'nonexistent optional template file in config' '
+	test_config commit.template ":(optional)$PWD"/notexist &&
+	(
+		GIT_EDITOR="echo hello >\"\$1\"" &&
+		export GIT_EDITOR &&
+		git commit --allow-empty
+	)
+'
+
 # From now on we'll use a template file that exists.
 TEMPLATE="$PWD"/template
 
-- 
2.47.0-148-g19c85929c5

[PATCH 3/3] parseopt: values of pathname type can be prefixed with :(optional)

[Junio C Hamano]

In the previous step, we introduced an optional filename that can be
given to a configuration variable, and nullify the fact that such a
configuration setting even existed if the named path is missing or
empty.

Let's do the same for command line options that name a pathname.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 parse-options.c                           | 31 +++++++++++++++--------
 t/t7500-commit-template-squash-signoff.sh | 12 ++++++++-
 2 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 33bfba0ed4..7a2a3b1f08 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -75,7 +75,6 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 {
 	const char *s, *arg;
 	const int unset = flags & OPT_UNSET;
-	int err;
 
 	if (unset && p->opt)
 		return error(_("%s takes no value"), optname(opt, flags));
@@ -131,21 +130,31 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 	case OPTION_FILENAME:
 	{
 		const char *value;
-
-		FREE_AND_NULL(*(char **)opt->value);
-
-		err = 0;
+		int is_optional;
 
 		if (unset)
 			value = NULL;
 		else if (opt->flags & PARSE_OPT_OPTARG && !p->opt)
-			value = (const char *) opt->defval;
-		else
-			err = get_arg(p, opt, flags, &value);
+			value = (char *)opt->defval;
+		else {
+			int err = get_arg(p, opt, flags, &value);
+			if (err)
+				return err;
+		}
+		if (!value)
+			return 0;
 
-		if (!err)
-			*(char **)opt->value = fix_filename(p->prefix, value);
-		return err;
+		is_optional = skip_prefix(value, ":(optional)", &value);
+		if (!value)
+			is_optional = 0;
+		value = fix_filename(p->prefix, value);
+		if (is_optional && is_empty_or_missing_file(value)) {
+			free((char *)value);
+		} else {
+			FREE_AND_NULL(*(char **)opt->value);
+			*(const char **)opt->value = value;
+		}
+		return 0;
 	}
 	case OPTION_CALLBACK:
 	{
diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index e28a79987d..c065f12baf 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -37,12 +37,22 @@ test_expect_success 'nonexistent template file should return error' '
 	)
 '
 
+test_expect_success 'nonexistent optional template file on command line' '
+	echo changes >> foo &&
+	git add foo &&
+	(
+		GIT_EDITOR="echo hello >\"\$1\"" &&
+		export GIT_EDITOR &&
+		git commit --template ":(optional)$PWD/notexist"
+	)
+'
+
 test_expect_success 'nonexistent template file in config should return error' '
 	test_config commit.template "$PWD"/notexist &&
 	(
 		GIT_EDITOR="echo hello >\"\$1\"" &&
 		export GIT_EDITOR &&
-		test_must_fail git commit
+		test_must_fail git commit --allow-empty
 	)
 '
 
-- 
2.47.0-148-g19c85929c5

Re: [PATCH 2/3] config: values of pathname type can be prefixed with :(optional)

[Patrick Steinhardt]

On Thu, May 01, 2025 at 02:40:56PM -0700, Junio C Hamano wrote:
> Sometimes people want to specify additional configuration data
> as "best effort" basis.  Maybe commit.template configuration file points
> at somewhere in ~/template/ but on a particular system, the file may not
> exist and the user may be OK without using the template in such a case.
> 
> When the value given to a configuration variable whose type is
> pathname wants to signal such an optional file, it can be marked by
> prepending ":(optional)" in front of it.  Such a setting that is
> marked optional would avoid getting the command barf for a missing
> file, as an optional configuration setting that names a missing or
> an empty file is not even seen.
> 
> cf. <xmqq5ywehb69.fsf@gitster.g>
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/config.txt                  |  5 ++++-
>  config.c                                  | 16 ++++++++++++++--
>  t/t7500-commit-template-squash-signoff.sh |  9 +++++++++
>  3 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 8c0b3ed807..199e29ccea 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -358,7 +358,10 @@ compiled without runtime prefix support, the compiled-in prefix will be
>  substituted instead. In the unlikely event that a literal path needs to
>  be specified that should _not_ be expanded, it needs to be prefixed by
>  `./`, like so: `./%(prefix)/bin`.
> -
> ++
> +If prefixed with `:(optional)`, the configuration variable is treated
> +as if it does not exist, if the named path does not exist or names an
> +empty file.

I can see why it may be useful to allow for non-existent paths. But I
wonder whether we really should be skipping over empty files, as well,
as it may be assuming too much about the semantics of a given config
key. In other words, are we reasonably sure that there won't ever be a
usecase where you may want to specify an optional and empty file? And
are there any use cases where an empty file should be ignored?

Patrick

Re: [PATCH 2/3] config: values of pathname type can be prefixed with :(optional)

[Phillip Wood]

On 02/05/2025 09:52, Patrick Steinhardt wrote:
> On Thu, May 01, 2025 at 02:40:56PM -0700, Junio C Hamano wrote:
> 
>> ++
>> +If prefixed with `:(optional)`, the configuration variable is treated
>> +as if it does not exist, if the named path does not exist or names an
>> +empty file.
> 
> I can see why it may be useful to allow for non-existent paths. But I
> wonder whether we really should be skipping over empty files, as well,
> as it may be assuming too much about the semantics of a given config
> key. In other words, are we reasonably sure that there won't ever be a
> usecase where you may want to specify an optional and empty file? And
> are there any use cases where an empty file should be ignored?

That's my thought too - ignoring a missing file sounds like a good idea 
but why an empty file too?

Best Wishes

Phillip

Re: [PATCH 2/3] config: values of pathname type can be prefixed with :(optional)

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> I can see why it may be useful to allow for non-existent paths. But I
> wonder whether we really should be skipping over empty files, as well,
> as it may be assuming too much about the semantics of a given config
> key. In other words, are we reasonably sure that there won't ever be a
> usecase where you may want to specify an optional and empty file? And
> are there any use cases where an empty file should be ignored?

If somebody goes back to the original discussion that happened a few
years before the patches were originally written, they might find a
use case where it is more convenient to ignore an empty file, but it
is an old patch series, so I do not remember the details.

I would not be surprised if the design decision for an empty blob
was done without any deep thought or motivationg use case.  After
all, this was "I had nothing better to do, so wrote these out of
boredom" patchset, as its cover letter said.

If somebody wants to carry these patches forward (which I am hoping
because there were a few people who expressed interest recently, and
because I am not all that interested, certainly not more than those
who wanted to have this feature), I think that the right approach is
to extend the system by taking advantage of the syntax that was
designed to be extensible.  In addition to ":(optional)", we could
add different variants like ":(optional,ignore-empty)" with desired
semantics, for example.

Thanks.

[PATCH v2 0/3] Support :(optional) filepaths

[D. Ben Knoble]

Notes:
- Based on commit 2da08f2c3d (parseopt: values of pathname type can be
  prefixed with :(optional), 2024-10-14) (broken-out/wip/optional-path)
- Rebased on v2.51.0
- I'm least sure of the 3rd patch and am happy to drop it in support of
  the first 2. I think it might be better to (a) integrate :(optional)
  support as pathspec magic and (b) use pathspec magic in parse-options
  when getting filenames. But I'm not sure, and this has other
  ramifications I'm not prepared to deal with. (For example: `git grep
  path <file>… :(optional)non-existent` could pretend like
  `non-existent` was never given?)
- The parsing is not exactly a "clean API," but I wasn't sure how to
  make it cleaner :)

Changes in v2:
- Only check for missing files, not empty files
- Move a test change to the appropriate commit
- Document optional magic in options in gitcli(1)

This series adds support for optional filepaths in config and
parse-options, which supports use-cases such as missing commit templates
or blame.ignoreRevsFile values without erroring.

v1: https://lore.kernel.org/git/20250501214057.371711-1-gitster@pobox.com/

Junio C Hamano (3):
  t7500: make each piece more independent
  config: values of pathname type can be prefixed with :(optional)
  parseopt: values of pathname type can be prefixed with :(optional)

 Documentation/config.adoc                 |  4 ++-
 Documentation/gitcli.adoc                 | 14 +++++++++
 config.c                                  | 16 +++++++++--
 parse-options.c                           | 31 +++++++++++++-------
 t/t7500-commit-template-squash-signoff.sh | 35 +++++++++++++++++------
 wrapper.c                                 | 13 +++++++++
 wrapper.h                                 |  4 ++-
 7 files changed, 94 insertions(+), 23 deletions(-)

Diff-intervalle contre v1 :
1:  82d283c626 ! 1:  63b2b24d42 t7500: make each piece more independent
    @@ Commit message
         Signed-off-by: Taylor Blau <me@ttaylorr.com>
     
      ## t/t7500-commit-template-squash-signoff.sh ##
    +@@ t/t7500-commit-template-squash-signoff.sh: commit_msg_is ()
    + 	(
    + 		GIT_EDITOR="echo hello >\"\$1\"" &&
    + 		export GIT_EDITOR &&
    +-		test_must_fail git commit
    ++		test_must_fail git commit --allow-empty
    + 	)
    + '
    + 
     @@ t/t7500-commit-template-squash-signoff.sh: commit_msg_is ()
      TEMPLATE="$PWD"/template
      
2:  dbafaff13b ! 2:  5c97f580a9 config: values of pathname type can be prefixed with :(optional)
    @@ Commit message
         pathname wants to signal such an optional file, it can be marked by
         prepending ":(optional)" in front of it.  Such a setting that is
         marked optional would avoid getting the command barf for a missing
    -    file, as an optional configuration setting that names a missing or
    -    an empty file is not even seen.
    +    file, as an optional configuration setting that names a missing
    +    file is not even seen.
     
         cf. <xmqq5ywehb69.fsf@gitster.g>
     
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
         Signed-off-by: Taylor Blau <me@ttaylorr.com>
     
    - ## Documentation/config.txt ##
    -@@ Documentation/config.txt: compiled without runtime prefix support, the compiled-in prefix will be
    +
    + ## Notes ##
    +    The 2nd paragraph in this commit is wrapped strangely
    +
    +    I've kept the strange wrapping length for now, but can reflow it if
    +    desired.
    +
    + ## Documentation/config.adoc ##
    +@@ Documentation/config.adoc: compiled without runtime prefix support, the compiled-in prefix will be
      substituted instead. In the unlikely event that a literal path needs to
      be specified that should _not_ be expanded, it needs to be prefixed by
      `./`, like so: `./%(prefix)/bin`.
     -
     ++
     +If prefixed with `:(optional)`, the configuration variable is treated
    -+as if it does not exist, if the named path does not exist or names an
    -+empty file.
    ++as if it does not exist, if the named path does not exist.
      
      Variables
      ~~~~~~~~~
    @@ config.c: int git_config_string(char **dest, const char *var, const char *value)
     +	if (!path)
      		die(_("failed to expand user dir in: '%s'"), value);
     +
    -+	if (is_optional && is_empty_or_missing_file(path)) {
    ++	if (is_optional && is_missing_file(path)) {
     +		free(path);
     +		return 0;
     +	}
    @@ t/t7500-commit-template-squash-signoff.sh: commit_msg_is ()
      # From now on we'll use a template file that exists.
      TEMPLATE="$PWD"/template
      
    +
    + ## wrapper.c ##
    +@@ wrapper.c: int xgethostname(char *buf, size_t len)
    + 	return ret;
    + }
    + 
    ++int is_missing_file(const char *filename)
    ++{
    ++	struct stat st;
    ++
    ++	if (stat(filename, &st) < 0) {
    ++		if (errno == ENOENT)
    ++			return 1;
    ++		die_errno(_("could not stat %s"), filename);
    ++	}
    ++
    ++	return 0;
    ++}
    ++
    + int is_empty_or_missing_file(const char *filename)
    + {
    + 	struct stat st;
    +
    + ## wrapper.h ##
    +@@ wrapper.h: void write_file_buf(const char *path, const char *buf, size_t len);
    + __attribute__((format (printf, 2, 3)))
    + void write_file(const char *path, const char *fmt, ...);
    + 
    +-/* Return 1 if the file is empty or does not exists, 0 otherwise. */
    ++/* Return 1 if the file does not exist, 0 otherwise. */
    ++int is_missing_file(const char *filename);
    ++/* Return 1 if the file is empty or does not exist, 0 otherwise. */
    + int is_empty_or_missing_file(const char *filename);
    + 
    + enum fsync_action {
3:  2da08f2c3d ! 3:  5f7057c236 parseopt: values of pathname type can be prefixed with :(optional)
    @@ Commit message
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
         Signed-off-by: Taylor Blau <me@ttaylorr.com>
     
    + ## Documentation/gitcli.adoc ##
    +@@ Documentation/gitcli.adoc: $ git describe --abbrev=10 HEAD  # correct
    + $ git describe --abbrev 10 HEAD  # NOT WHAT YOU MEANT
    + ----------------------------
    + 
    ++
    ++Magic filename options
    ++~~~~~~~~~~~~~~~~~~~~~~
    ++Options that take a filename allow a prefix `:(optional)`. For example:
    ++
    ++----------------------------
    ++git commit -F :(optional)COMMIT_EDITMSG
    ++# if COMMIT_EDITMSG does not exist, equivalent to
    ++git commit
    ++----------------------------
    ++
    ++Like with configuration values, if the named file is missing Git behaves as if
    ++the option was not given at all. See "Values" in linkgit:git-config[1].
    ++
    + NOTES ON FREQUENTLY CONFUSED OPTIONS
    + ------------------------------------
    + 
    +
      ## parse-options.c ##
     @@ parse-options.c: static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
      {
    - 	const char *s, *arg;
    + 	const char *arg;
      	const int unset = flags & OPT_UNSET;
     -	int err;
      
    @@ t/t7500-commit-template-squash-signoff.sh: commit_msg_is ()
      test_expect_success 'nonexistent template file in config should return error' '
      	test_config commit.template "$PWD"/notexist &&
      	(
    - 		GIT_EDITOR="echo hello >\"\$1\"" &&
    - 		export GIT_EDITOR &&
    --		test_must_fail git commit
    -+		test_must_fail git commit --allow-empty
    - 	)
    - '
    - 

base-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0
-- 
2.48.1

[PATCH v2 1/3] t7500: make each piece more independent

[D. Ben Knoble]

From: Junio C Hamano <gitster@pobox.com>

These tests prepare the working tree & index state to have something
to be committed, and try a sequence of "test_must_fail git commit".
If an earlier one did not fail by a bug, a later one will fail for
a wrong reason (namely, "nothing to commit").

Give them "--allow-empty" to make sure that they would work even
when there is nothing to commit by accident.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---
 t/t7500-commit-template-squash-signoff.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index 4dca8d97a7..05cda50186 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -42,7 +42,7 @@ commit_msg_is ()
 	(
 		GIT_EDITOR="echo hello >\"\$1\"" &&
 		export GIT_EDITOR &&
-		test_must_fail git commit
+		test_must_fail git commit --allow-empty
 	)
 '
 
@@ -50,33 +50,33 @@ commit_msg_is ()
 TEMPLATE="$PWD"/template
 
 test_expect_success 'unedited template should not commit' '
-	echo "template line" > "$TEMPLATE" &&
-	test_must_fail git commit --template "$TEMPLATE"
+	echo "template line" >"$TEMPLATE" &&
+	test_must_fail git commit --allow-empty --template "$TEMPLATE"
 '
 
 test_expect_success 'unedited template with comments should not commit' '
-	echo "# comment in template" >> "$TEMPLATE" &&
-	test_must_fail git commit --template "$TEMPLATE"
+	echo "# comment in template" >>"$TEMPLATE" &&
+	test_must_fail git commit --allow-empty --template "$TEMPLATE"
 '
 
 test_expect_success 'a Signed-off-by line by itself should not commit' '
 	(
 		test_set_editor "$TEST_DIRECTORY"/t7500/add-signed-off &&
-		test_must_fail git commit --template "$TEMPLATE"
+		test_must_fail git commit --allow-empty --template "$TEMPLATE"
 	)
 '
 
 test_expect_success 'adding comments to a template should not commit' '
 	(
 		test_set_editor "$TEST_DIRECTORY"/t7500/add-comments &&
-		test_must_fail git commit --template "$TEMPLATE"
+		test_must_fail git commit --allow-empty --template "$TEMPLATE"
 	)
 '
 
 test_expect_success 'adding real content to a template should commit' '
 	(
 		test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
-		git commit --template "$TEMPLATE"
+		git commit --allow-empty --template "$TEMPLATE"
 	) &&
 	commit_msg_is "template linecommit message"
 '
-- 
2.48.1

[PATCH v2 2/3] config: values of pathname type can be prefixed with :(optional)

[D. Ben Knoble]

From: Junio C Hamano <gitster@pobox.com>

Sometimes people want to specify additional configuration data
as "best effort" basis.  Maybe commit.template configuration file points
at somewhere in ~/template/ but on a particular system, the file may not
exist and the user may be OK without using the template in such a case.

When the value given to a configuration variable whose type is
pathname wants to signal such an optional file, it can be marked by
prepending ":(optional)" in front of it.  Such a setting that is
marked optional would avoid getting the command barf for a missing
file, as an optional configuration setting that names a missing
file is not even seen.

cf. <xmqq5ywehb69.fsf@gitster.g>

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---

Notes:
    The 2nd paragraph in this commit is wrapped strangely
    
    I've kept the strange wrapping length for now, but can reflow it if
    desired.

 Documentation/config.adoc                 |  4 +++-
 config.c                                  | 16 ++++++++++++++--
 t/t7500-commit-template-squash-signoff.sh |  9 +++++++++
 wrapper.c                                 | 13 +++++++++++++
 wrapper.h                                 |  4 +++-
 5 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.adoc b/Documentation/config.adoc
index cc769251be..7301ced836 100644
--- a/Documentation/config.adoc
+++ b/Documentation/config.adoc
@@ -358,7 +358,9 @@ compiled without runtime prefix support, the compiled-in prefix will be
 substituted instead. In the unlikely event that a literal path needs to
 be specified that should _not_ be expanded, it needs to be prefixed by
 `./`, like so: `./%(prefix)/bin`.
-
++
+If prefixed with `:(optional)`, the configuration variable is treated
+as if it does not exist, if the named path does not exist.
 
 Variables
 ~~~~~~~~~
diff --git a/config.c b/config.c
index 97ffef4270..73fc74c8fa 100644
--- a/config.c
+++ b/config.c
@@ -1279,11 +1279,23 @@ int git_config_string(char **dest, const char *var, const char *value)
 
 int git_config_pathname(char **dest, const char *var, const char *value)
 {
+	int is_optional;
+	char *path;
+
 	if (!value)
 		return config_error_nonbool(var);
-	*dest = interpolate_path(value, 0);
-	if (!*dest)
+
+	is_optional = skip_prefix(value, ":(optional)", &value);
+	path = interpolate_path(value, 0);
+	if (!path)
 		die(_("failed to expand user dir in: '%s'"), value);
+
+	if (is_optional && is_missing_file(path)) {
+		free(path);
+		return 0;
+	}
+
+	*dest = path;
 	return 0;
 }
 
diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index 05cda50186..366f7f23b3 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -46,6 +46,15 @@ commit_msg_is ()
 	)
 '
 
+test_expect_success 'nonexistent optional template file in config' '
+	test_config commit.template ":(optional)$PWD"/notexist &&
+	(
+		GIT_EDITOR="echo hello >\"\$1\"" &&
+		export GIT_EDITOR &&
+		git commit --allow-empty
+	)
+'
+
 # From now on we'll use a template file that exists.
 TEMPLATE="$PWD"/template
 
diff --git a/wrapper.c b/wrapper.c
index 2f00d2ac87..3d507d4204 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -721,6 +721,19 @@ int xgethostname(char *buf, size_t len)
 	return ret;
 }
 
+int is_missing_file(const char *filename)
+{
+	struct stat st;
+
+	if (stat(filename, &st) < 0) {
+		if (errno == ENOENT)
+			return 1;
+		die_errno(_("could not stat %s"), filename);
+	}
+
+	return 0;
+}
+
 int is_empty_or_missing_file(const char *filename)
 {
 	struct stat st;
diff --git a/wrapper.h b/wrapper.h
index 7df824e34a..44a8597ac3 100644
--- a/wrapper.h
+++ b/wrapper.h
@@ -66,7 +66,9 @@ void write_file_buf(const char *path, const char *buf, size_t len);
 __attribute__((format (printf, 2, 3)))
 void write_file(const char *path, const char *fmt, ...);
 
-/* Return 1 if the file is empty or does not exists, 0 otherwise. */
+/* Return 1 if the file does not exist, 0 otherwise. */
+int is_missing_file(const char *filename);
+/* Return 1 if the file is empty or does not exist, 0 otherwise. */
 int is_empty_or_missing_file(const char *filename);
 
 enum fsync_action {
-- 
2.48.1

[PATCH v2 3/3] parseopt: values of pathname type can be prefixed with :(optional)

[D. Ben Knoble]

From: Junio C Hamano <gitster@pobox.com>

In the previous step, we introduced an optional filename that can be
given to a configuration variable, and nullify the fact that such a
configuration setting even existed if the named path is missing or
empty.

Let's do the same for command line options that name a pathname.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---
 Documentation/gitcli.adoc                 | 14 ++++++++++
 parse-options.c                           | 31 +++++++++++++++--------
 t/t7500-commit-template-squash-signoff.sh | 10 ++++++++
 3 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/Documentation/gitcli.adoc b/Documentation/gitcli.adoc
index 1ea681b59d..ef2a0a399d 100644
--- a/Documentation/gitcli.adoc
+++ b/Documentation/gitcli.adoc
@@ -216,6 +216,20 @@ $ git describe --abbrev=10 HEAD  # correct
 $ git describe --abbrev 10 HEAD  # NOT WHAT YOU MEANT
 ----------------------------
 
+
+Magic filename options
+~~~~~~~~~~~~~~~~~~~~~~
+Options that take a filename allow a prefix `:(optional)`. For example:
+
+----------------------------
+git commit -F :(optional)COMMIT_EDITMSG
+# if COMMIT_EDITMSG does not exist, equivalent to
+git commit
+----------------------------
+
+Like with configuration values, if the named file is missing Git behaves as if
+the option was not given at all. See "Values" in linkgit:git-config[1].
+
 NOTES ON FREQUENTLY CONFUSED OPTIONS
 ------------------------------------
 
diff --git a/parse-options.c b/parse-options.c
index 5224203ffe..4faf66023a 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -133,7 +133,6 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 {
 	const char *arg;
 	const int unset = flags & OPT_UNSET;
-	int err;
 
 	if (unset && p->opt)
 		return error(_("%s takes no value"), optname(opt, flags));
@@ -209,21 +208,31 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 	case OPTION_FILENAME:
 	{
 		const char *value;
-
-		FREE_AND_NULL(*(char **)opt->value);
-
-		err = 0;
+		int is_optional;
 
 		if (unset)
 			value = NULL;
 		else if (opt->flags & PARSE_OPT_OPTARG && !p->opt)
-			value = (const char *) opt->defval;
-		else
-			err = get_arg(p, opt, flags, &value);
+			value = (char *)opt->defval;
+		else {
+			int err = get_arg(p, opt, flags, &value);
+			if (err)
+				return err;
+		}
+		if (!value)
+			return 0;
 
-		if (!err)
-			*(char **)opt->value = fix_filename(p->prefix, value);
-		return err;
+		is_optional = skip_prefix(value, ":(optional)", &value);
+		if (!value)
+			is_optional = 0;
+		value = fix_filename(p->prefix, value);
+		if (is_optional && is_empty_or_missing_file(value)) {
+			free((char *)value);
+		} else {
+			FREE_AND_NULL(*(char **)opt->value);
+			*(const char **)opt->value = value;
+		}
+		return 0;
 	}
 	case OPTION_CALLBACK:
 	{
diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index 366f7f23b3..c065f12baf 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -37,6 +37,16 @@ commit_msg_is ()
 	)
 '
 
+test_expect_success 'nonexistent optional template file on command line' '
+	echo changes >> foo &&
+	git add foo &&
+	(
+		GIT_EDITOR="echo hello >\"\$1\"" &&
+		export GIT_EDITOR &&
+		git commit --template ":(optional)$PWD/notexist"
+	)
+'
+
 test_expect_success 'nonexistent template file in config should return error' '
 	test_config commit.template "$PWD"/notexist &&
 	(
-- 
2.48.1

Re: [PATCH v2 0/3] Support :(optional) filepaths

[Junio C Hamano]

"D. Ben Knoble" <ben.knoble+github@gmail.com> writes:

Before "notes" you would want an overall description of what the
topic is for those who no longer remember the previous iteration,
or for those this iteration is the first one they see.

> Notes:
> - Based on commit 2da08f2c3d (parseopt: values of pathname type can be
>   prefixed with :(optional), 2024-10-14) (broken-out/wip/optional-path)
> - Rebased on v2.51.0

Thanks.

> - I'm least sure of the 3rd patch and am happy to drop it in support of
>   the first 2. I think it might be better to (a) integrate :(optional)
>   support as pathspec magic and (b) use pathspec magic in parse-options
>   when getting filenames. But I'm not sure, and this has other
>   ramifications I'm not prepared to deal with. (For example: `git grep
>   path <file>… :(optional)non-existent` could pretend like
>   `non-existent` was never given?)

While it might not hurt, I do not see a need for such a support.

Pathspec _is_ a pattern.  If an existing path does not match the
pattern, there is no ill effect.  In other words, in this command
invocation:

    $ git grep -e needle -- Makefile no-such-file.txt

neither Makefile or no-such-file.txt is required nor optional.  If
there are paths that match these two "patterns" among the paths in
the working tree that are known to the index, the contents of these
paths are inspected by the command.  If no paths match the patterns,
that is fine as well.

The command line parser helpfully offers to notice a pathspec
pattern that did not match any path when you do not give "--", but
that is up to the caller of match_pathspec() API to do so.  The
pathspec machinery only reports if each pathspec element matched a
path in its seen[] array, and the caller can use that information to
report which pathspec elements did not contribute to finding the set
of paths to work on.

> - The parsing is not exactly a "clean API," but I wasn't sure how to
>   make it cleaner :)

What you have in [2/3], the update to git_config_pathname(), seems
quite reasonable and something that cannot be made cleaner, to me.

> Changes in v2:
> - Only check for missing files, not empty files
> - Move a test change to the appropriate commit
> - Document optional magic in options in gitcli(1)

I agree that it is a better design not to special case an empty file
like the previous round did.  Looking better.

> This series adds support for optional filepaths in config and
> parse-options, which supports use-cases such as missing commit templates
> or blame.ignoreRevsFile values without erroring.

Yes, this is what you wanted to have at the very beginning, before
listing points you want to call attention to under "Notes" label.

Will queue.  Thanks for resurrecting the topic.

Re: [PATCH v2 0/3] Support :(optional) filepaths

[Ben Knoble]

> Le 28 sept. 2025 à 18:40, Junio C Hamano <gitster@pobox.com> a écrit :
> 
> "D. Ben Knoble" <ben.knoble+github@gmail.com> writes:
> 
> Before "notes" you would want an overall description of what the
> topic is for those who no longer remember the previous iteration,
> or for those this iteration is the first one they see.

Agreed, thanks.

>> - I'm least sure of the 3rd patch and am happy to drop it in support of
>>  the first 2. I think it might be better to (a) integrate :(optional)
>>  support as pathspec magic and (b) use pathspec magic in parse-options
>>  when getting filenames. But I'm not sure, and this has other
>>  ramifications I'm not prepared to deal with. (For example: `git grep
>>  path <file>… :(optional)non-existent` could pretend like
>>  `non-existent` was never given?)
> 
> While it might not hurt, I do not see a need for such a support.
> 
> Pathspec _is_ a pattern.  If an existing path does not match the
> pattern, there is no ill effect.  In other words, in this command
> invocation:
> 
>    $ git grep -e needle -- Makefile no-such-file.txt
> 
> neither Makefile or no-such-file.txt is required nor optional.  If
> there are paths that match these two "patterns" among the paths in
> the working tree that are known to the index, the contents of these
> paths are inspected by the command.  If no paths match the patterns,
> that is fine as well.
> 
> The command line parser helpfully offers to notice a pathspec
> pattern that did not match any path when you do not give "--", but
> that is up to the caller of match_pathspec() API to do so.  The
> pathspec machinery only reports if each pathspec element matched a
> path in its seen[] array, and the caller can use that information to
> report which pathspec elements did not contribute to finding the set
> of paths to work on.

I 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 :)

> Will queue.  Thanks for resurrecting the topic.

Thanks!

Re: [PATCH v2 2/3] config: values of pathname type can be prefixed with :(optional)

[Phillip Wood]

Hi Ben

On 28/09/2025 22:29, D. Ben Knoble wrote:
> From: Junio C Hamano <gitster@pobox.com>
> 
> Sometimes people want to specify additional configuration data
> as "best effort" basis.  Maybe commit.template configuration file points
> at somewhere in ~/template/ but on a particular system, the file may not
> exist and the user may be OK without using the template in such a case.
> 
> When the value given to a configuration variable whose type is
> pathname wants to signal such an optional file, it can be marked by
> prepending ":(optional)" in front of it.  Such a setting that is
> marked optional would avoid getting the command barf for a missing
> file, as an optional configuration setting that names a missing
> file is not even seen.

I think this would be a useful addition, we've had several people 
wanting to make blame.ignoreRevsFile optional and this provides a 
general way to do that.

> --- a/config.c
> +++ b/config.c
> @@ -1279,11 +1279,23 @@ int git_config_string(char **dest, const char *var, const char *value)
>   
>   int git_config_pathname(char **dest, const char *var, const char *value)
>   {
> +	int is_optional;

This could be bool rather than int, the rest of the implementation looks 
good.

> --- a/t/t7500-commit-template-squash-signoff.sh
> +++ b/t/t7500-commit-template-squash-signoff.sh
> @@ -46,6 +46,15 @@ commit_msg_is ()
>   	)
>   '
>   
> +test_expect_success 'nonexistent optional template file in config' '
> +	test_config commit.template ":(optional)$PWD"/notexist &&
> +	(
> +		GIT_EDITOR="echo hello >\"\$1\"" &&

when git runs the editor this will be expanded to

     sh -c 'echo hello >"$1" "$@"' 'echo hello >"$1"' path/to/file

I think it should be

     GIT_EDITOR="echo hello >"

instead
> +		export GIT_EDITOR &&
> +		git commit --allow-empty

Maybe I'm missing something but don't we want to ensure that we have a 
non-empty message here? Also as it is a single command we can avoid the 
subshell with

     GIT_EDITOR="echo hello >" git commit

Thanks

Phillip

Re: [PATCH v2 3/3] parseopt: values of pathname type can be prefixed with :(optional)

[Phillip Wood]

Hi Ben

On 28/09/2025 22:29, D. Ben Knoble wrote:
> From: Junio C Hamano <gitster@pobox.com>
> 
> In the previous step, we introduced an optional filename that can be
> given to a configuration variable, and nullify the fact that such a
> configuration setting even existed if the named path is missing or
> empty.
> 
> Let's do the same for command line options that name a pathname.

Sounds sensible

> +Magic filename options

I assume we're calling these "magic" to match to pathspec "magic" 
options? I wonder if that is a good idea but I don't have a better 
suggestion.

> +~~~~~~~~~~~~~~~~~~~~~~
> +Options that take a filename allow a prefix `:(optional)`. For example:
> +
> +----------------------------
> +git commit -F :(optional)COMMIT_EDITMSG
> +# if COMMIT_EDITMSG does not exist, equivalent to

This doesn't quite scan for me, maybe s/, /, it is/ ?

> +git commit
> +----------------------------
> +
> +Like with configuration values, if the named file is missing Git behaves as if

I'd drop "with" here

> +the option was not given at all. See "Values" in linkgit:git-config[1].
> +

> @@ -209,21 +208,31 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
>   	case OPTION_FILENAME:
>   	{
>   		const char *value;
> -
> -		FREE_AND_NULL(*(char **)opt->value);
> -
> -		err = 0;
> +		int is_optional;

This can be a bool as in the last patch.

>   		if (unset)
>   			value = NULL;
>   		else if (opt->flags & PARSE_OPT_OPTARG && !p->opt)
> -			value = (const char *) opt->defval;
> -		else
> -			err = get_arg(p, opt, flags, &value);
> +			value = (char *)opt->defval;

I'm not sure why we're changing the cast here (or why we need one in the 
first place assuming opt->defval is "void*")

> +		else {
> +			int err = get_arg(p, opt, flags, &value);
> +			if (err)
> +				return err;
> +		}
> +		if (!value)
> +			return 0;
>   
> -		if (!err)
> -			*(char **)opt->value = fix_filename(p->prefix, value);
> -		return err;
> +		is_optional = skip_prefix(value, ":(optional)", &value);
> +		if (!value)
> +			is_optional = 0;

I'm struggling to see how value can be NULL here as we return early if 
it NULL before calling skip_prefix()

> +		value = fix_filename(p->prefix, value);
> +		if (is_optional && is_empty_or_missing_file(value)) {
> +			free((char *)value);

I think we want to call is_missing_file() here. If the file is missing 
then we do nothing which matches the documentation above - Good.

> +		} else {
> +			FREE_AND_NULL(*(char **)opt->value);
> +			*(const char **)opt->value = value;

If the file isn't optional or it is optional and exists then we behave 
as before - Good.

Thanks

Phillip

> +		}
> +		return 0;
>   	}
>   	case OPTION_CALLBACK:
>   	{
> diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
> index 366f7f23b3..c065f12baf 100755
> --- a/t/t7500-commit-template-squash-signoff.sh
> +++ b/t/t7500-commit-template-squash-signoff.sh
> @@ -37,6 +37,16 @@ commit_msg_is ()
>   	)
>   '
>   
> +test_expect_success 'nonexistent optional template file on command line' '
> +	echo changes >> foo &&
> +	git add foo &&
> +	(
> +		GIT_EDITOR="echo hello >\"\$1\"" &&
> +		export GIT_EDITOR &&
> +		git commit --template ":(optional)$PWD/notexist"
> +	)
> +'
> +
>   test_expect_success 'nonexistent template file in config should return error' '
>   	test_config commit.template "$PWD"/notexist &&
>   	(

Re: [PATCH v2 2/3] config: values of pathname type can be prefixed with :(optional)

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

>> +	test_config commit.template ":(optional)$PWD"/notexist &&
>> +	(
>> +		GIT_EDITOR="echo hello >\"\$1\"" &&
>
> when git runs the editor this will be expanded to
>
>     sh -c 'echo hello >"$1" "$@"' 'echo hello >"$1"' path/to/file
>
> I think it should be
>
>     GIT_EDITOR="echo hello >"
>
> instead

That's interesting in that I find it unusual.  Fine as long as it
works ;-)

> Maybe I'm missing something but don't we want to ensure that we have a
> non-empty message here? Also as it is a single command we can avoid
> the subshell with
>
>     GIT_EDITOR="echo hello >" git commit

Yeah, that does sound better.

Re: [PATCH v2 2/3] config: values of pathname type can be prefixed with :(optional)

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>>> +	test_config commit.template ":(optional)$PWD"/notexist &&
>>> +	(
>>> +		GIT_EDITOR="echo hello >\"\$1\"" &&
>>
>> when git runs the editor this will be expanded to
>>
>>     sh -c 'echo hello >"$1" "$@"' 'echo hello >"$1"' path/to/file
>>
>> I think it should be
>>
>>     GIT_EDITOR="echo hello >"
>>
>> instead

It seems that this was a copy-paste from a few of tests before this
new piece.  They all _expect_ to fail, so probably nobody bothered
to inspect the outcome ;-)

I just looked at what actually goes to COMMIT_EDITMSG with this test
that expects to succeed.

$ cat .git/COMMIT_EDITMSG
hello /home/gitster/w/git.git/t/trash directory.t7500-commit-template-squash-signoff/.git/COMMIT_EDITMSG

So, you're right to say "$@" will be given in addition to "hello" as
arguments to "echo".  That extra argument is to tell the editor the
path to the edited file.

We'd probably need a preliminary clean-up patch to fix all of these
in the vicinity.

Thanks.

Re: [PATCH v2 2/3] config: values of pathname type can be prefixed with :(optional)

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> We'd probably need a preliminary clean-up patch to fix all of these
> in the vicinity.

So, here is the preliminary clea-up step that should come before
[2/3]

--- >8 ---
Subject: [PATCH] t7500: fix GIT_EDITOR shell snippet

2140b140 (commit: error out for missing commit message template,
2011-02-25) defined

    GIT_EDITOR="echo hello >\"\$1\""

for thest two tests, with the intention that 'hello' would be
written in the given file, but as Phillip Wood points out,
GIT_EDITOR is invoked by shell after getting expanded to

    sh -c 'echo hello >"$1" "$@"' 'echo hello >"$1"' path/to/file

which is not what we want.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t7500-commit-template-squash-signoff.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index 05cda50186..4922543256 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -31,7 +31,7 @@ test_expect_success 'nonexistent template file should return error' '
 	echo changes >> foo &&
 	git add foo &&
 	(
-		GIT_EDITOR="echo hello >\"\$1\"" &&
+		GIT_EDITOR="echo hello >" &&
 		export GIT_EDITOR &&
 		test_must_fail git commit --template "$PWD"/notexist
 	)
@@ -40,7 +40,7 @@ test_expect_success 'nonexistent template file should return error' '
 test_expect_success 'nonexistent template file in config should return error' '
 	test_config commit.template "$PWD"/notexist &&
 	(
-		GIT_EDITOR="echo hello >\"\$1\"" &&
+		GIT_EDITOR="echo hello >" &&
 		export GIT_EDITOR &&
 		test_must_fail git commit --allow-empty
 	)
-- 
2.51.0-580-g8258b70b6e

Re: [PATCH v2 2/3] config: values of pathname type can be prefixed with :(optional)

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> So, here is the preliminary clea-up step that should come before
> [2/3]
>
> --- >8 ---
> Subject: [PATCH] t7500: fix GIT_EDITOR shell snippet

And this is [2/3] rebased on top.

--- >8 ---
Subject: [PATCH] config: values of pathname type can be prefixed with :(optional)

Sometimes people want to specify additional configuration data
as "best effort" basis.  Maybe commit.template configuration file points
at somewhere in ~/template/ but on a particular system, the file may not
exist and the user may be OK without using the template in such a case.

When the value given to a configuration variable whose type is
pathname wants to signal such an optional file, it can be marked by
prepending ":(optional)" in front of it.  Such a setting that is
marked optional would avoid getting the command barf for a missing
file, as an optional configuration setting that names a missing
file is not even seen.

cf. <xmqq5ywehb69.fsf@gitster.g>

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config.adoc                 |  4 +++-
 config.c                                  | 16 ++++++++++++++--
 t/t7500-commit-template-squash-signoff.sh |  8 ++++++++
 wrapper.c                                 | 13 +++++++++++++
 wrapper.h                                 |  4 +++-
 5 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.adoc b/Documentation/config.adoc
index cc769251be..7301ced836 100644
--- a/Documentation/config.adoc
+++ b/Documentation/config.adoc
@@ -358,7 +358,9 @@ compiled without runtime prefix support, the compiled-in prefix will be
 substituted instead. In the unlikely event that a literal path needs to
 be specified that should _not_ be expanded, it needs to be prefixed by
 `./`, like so: `./%(prefix)/bin`.
-
++
+If prefixed with `:(optional)`, the configuration variable is treated
+as if it does not exist, if the named path does not exist.
 
 Variables
 ~~~~~~~~~
diff --git a/config.c b/config.c
index 97ffef4270..73fc74c8fa 100644
--- a/config.c
+++ b/config.c
@@ -1279,11 +1279,23 @@ int git_config_string(char **dest, const char *var, const char *value)
 
 int git_config_pathname(char **dest, const char *var, const char *value)
 {
+	int is_optional;
+	char *path;
+
 	if (!value)
 		return config_error_nonbool(var);
-	*dest = interpolate_path(value, 0);
-	if (!*dest)
+
+	is_optional = skip_prefix(value, ":(optional)", &value);
+	path = interpolate_path(value, 0);
+	if (!path)
 		die(_("failed to expand user dir in: '%s'"), value);
+
+	if (is_optional && is_missing_file(path)) {
+		free(path);
+		return 0;
+	}
+
+	*dest = path;
 	return 0;
 }
 
diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index 4922543256..a85229e556 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -46,6 +46,14 @@ test_expect_success 'nonexistent template file in config should return error' '
 	)
 '
 
+test_expect_success 'nonexistent optional template file in config' '
+	test_config commit.template ":(optional)$PWD"/notexist &&
+	GIT_EDITOR="echo hello >" git commit --allow-empty &&
+	git cat-file commit HEAD | sed -e "1,/^$/d" >actual &&
+	echo hello >expect &&
+	test_cmp expect actual
+'
+
 # From now on we'll use a template file that exists.
 TEMPLATE="$PWD"/template
 
diff --git a/wrapper.c b/wrapper.c
index 2f00d2ac87..3d507d4204 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -721,6 +721,19 @@ int xgethostname(char *buf, size_t len)
 	return ret;
 }
 
+int is_missing_file(const char *filename)
+{
+	struct stat st;
+
+	if (stat(filename, &st) < 0) {
+		if (errno == ENOENT)
+			return 1;
+		die_errno(_("could not stat %s"), filename);
+	}
+
+	return 0;
+}
+
 int is_empty_or_missing_file(const char *filename)
 {
 	struct stat st;
diff --git a/wrapper.h b/wrapper.h
index 7df824e34a..44a8597ac3 100644
--- a/wrapper.h
+++ b/wrapper.h
@@ -66,7 +66,9 @@ void write_file_buf(const char *path, const char *buf, size_t len);
 __attribute__((format (printf, 2, 3)))
 void write_file(const char *path, const char *fmt, ...);
 
-/* Return 1 if the file is empty or does not exists, 0 otherwise. */
+/* Return 1 if the file does not exist, 0 otherwise. */
+int is_missing_file(const char *filename);
+/* Return 1 if the file is empty or does not exist, 0 otherwise. */
 int is_empty_or_missing_file(const char *filename);
 
 enum fsync_action {
-- 
2.51.0-580-g8258b70b6e

Re: [PATCH v2 2/3] config: values of pathname type can be prefixed with :(optional)

[Kristoffer Haugsbakk]

On Mon, Oct 6, 2025, at 22:21, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> We'd probably need a preliminary clean-up patch to fix all of these
>> in the vicinity.
>
> So, here is the preliminary clea-up step that should come before
> [2/3]
>
> --- >8 ---
> Subject: [PATCH] t7500: fix GIT_EDITOR shell snippet
>
> 2140b140 (commit: error out for missing commit message template,
> 2011-02-25) defined
>
>     GIT_EDITOR="echo hello >\"\$1\""
>
> for thest two tests, with the intention that 'hello' would be

s/thest/these/

>[snip]

Re: [PATCH v2 2/3] config: values of pathname type can be prefixed with :(optional)

[Junio C Hamano]

"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:

> On Mon, Oct 6, 2025, at 22:21, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> We'd probably need a preliminary clean-up patch to fix all of these
>>> in the vicinity.
>>
>> So, here is the preliminary clea-up step that should come before
>> [2/3]
>>
>> --- >8 ---
>> Subject: [PATCH] t7500: fix GIT_EDITOR shell snippet
>>
>> 2140b140 (commit: error out for missing commit message template,
>> 2011-02-25) defined
>>
>>     GIT_EDITOR="echo hello >\"\$1\""
>>
>> for thest two tests, with the intention that 'hello' would be
>
> s/thest/these/

Thanks.  Will modify locally.