Re: [PATCH 0/1] blame: Skip missing ignore-revs file
- From
Noah Pendleton <noah.pendleton@gmail.com>
- Date
- Aug 8, 2021, 18:21 UTC
- Message-ID
- <CADm0i39LV91kochHSGVHovaTbDOd0COrQPXHD3x8rEj-1Y+eMA@mail.gmail.com>
- In-Reply-To
- <xmqqim0fhlm1.fsf@gitster.g>
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:
Show 61 quoted lines
>
> 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.
>