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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 8, 2021, 17:50 UTC
Message-ID: <xmqqim0fhlm1.fsf@gitster.g>
In-Reply-To: <xmqqtuk0h4ph.fsf@gitster.g>

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

Show 10 quoted lines

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