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.