Re: [PATCH 2/2] config-parse: split library out of config.[c|h]

From: Glen Choo <chooglen@google.com>
Date: Jul 21, 2023, 15:55 UTC
Message-ID: <kl6lwmytz2cf.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <20230721003107.3095493-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 8 quoted lines

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

Show 6 quoted lines

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

Show 7 quoted lines

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

Show 5 quoted lines

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