Jonathan Tan <jonathantanmy@xxxxxxxxxx> writes: >> Begin this process by splitting the config parsing code out of >> config.[c|h] and into config-parse.[c|h]. > > I think we need to be more careful in how we split. It would be easier > if there is a concrete use case, but some preliminary questions: > > - "respect_includes" is in the library, but callers might want to opt > out of it or provide an alternative way to resolve includes. Makes sense. Or alternatively, we could choose not to support "respect_includes" initially, and exclude it to avoid confusion. > - There is a lot of error reporting capability with respect to the > source of config, and not all sources are applicable to library > users. How should we proceed? E.g. should we expect that all library > users conform to the list here (e.g. even if the source is something > like but not exactly STDIN, they should pick it), or allow users to > customize sources? Good point. I would also prefer to have the list of sources constrained to the list of sources available via the library. Some possibilities I can see are: 1. Move the Git-program-specific error message reporting to a level above the library (i.e. config.c). 2. Proceed as-is (with the additional sources in the library) and leave a FIXME to address this when we find a Git library-idiomatic way to handle errors. This won't be the last time we'll have to untangle Git-program-specific error reporting from the library - it might be useful to try to figure out all of that in one fell swoop. 3. Figure out library-idiomatic error handling mentioned in 2. right now. I think 1. is the best option, but if that fails, 2. is also reasonable. 3. is too difficult to do with a sample size of 1. > In the absence of more information, the split I would envision is > either something that can only parse a buffer, its error messages being > very generic (the caller should turn them into something more specific > before showing them to the user) (but one problem here is that we must > postprocess includes, which might be a problem if the output of parsing > is a flat hashtable, since we wouldn't know which keys are overridden > by the includes and which are not); Hm, how does the include mechanism here this differ from what's in this patch? This also only parses a single file and ignores includes. I'm not sure why this requires us to postprocess includes - in config.c, includes are handled by 'pausing' parsing of the current source, evaluating the included files, then 'resuming' parsing. > or something that can take in a > callback that is invoked whenever something is included and maybe also > a callback for access to the object database and that has full knowledge > of all sources for error reporting (or allows the caller to customize > the sources). Ah, I like this callback idea quite a lot. This lets config-parse.c easily support unconditional includes and provides entry points for program-specific behavior (like checking the odb). I will try this.
