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

[Noah Pendleton <noah.pendleton@gmail.com>]

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.
>