Thread (2 messages) 2 messages, 2 authors, 2021-08-08

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

From: Noah Pendleton <hidden>
Date: 2021-08-08 18:21:57

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 [off-list ref] wrote:
Junio C Hamano [off-list ref] writes:
quoted
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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help