Re: [PATCH v2 2/4] config: report config parse errors using cb

[Josh Steadmon <steadmon@google.com>]

On 2023.08.23 18:19, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> > From: Glen Choo <chooglen@google.com>
> >
> > In a subsequent commit, config parsing will become its own library, and
> > it's likely that the caller will want flexibility in handling errors
> > (instead of being limited to the error handling we have in-tree).
> 
> And the in-tree error handling is abstracted out as the
> git_config_err_fn() function; in other words, we become the first
> client of the library interface, which makes sense.
> 
> > @@ -1035,8 +1088,6 @@ static int git_parse_source(struct config_source *cs, config_fn_t fn,
> >  	int comment = 0;
> >  	size_t baselen = 0;
> >  	struct strbuf *var = &cs->var;
> > ...
> > +	/*
> > +	 * FIXME for whatever reason, do_event passes the _previous_ event, so
> > +	 * in order for our callback to receive the error event, we have to call
> > +	 * do_event twice
> > +	 */
> > +	do_event(cs, CONFIG_EVENT_ERROR, &event_data);
> > +	do_event(cs, CONFIG_EVENT_ERROR, &event_data);
> > +	return -1;
> >  }
> 
> This indeed is very curious and needs to be looked into before we
> proceed further.  How does the current control flow cope with the
> behaviour?

As Jonathan Tan mentioned in [1], on calling do_event() we set the start
offset of the new event, and execute the callback for the previous event
whose end offset we now know.

I refactored this into "start_event()" and "flush_event()" functions as
suggested, and added a new "do_event_and_flush()" function for the case
where we want to immediately execute a callback for an event.

[1]: https://lore.kernel.org/git/20230804213457.1174493-1-jonathantanmy@google.com/

> > @@ -2322,7 +2342,9 @@ void read_early_config(config_fn_t cb, void *data)
> >   */
> >  void read_very_early_config(config_fn_t cb, void *data)
> >  {
> > -	struct config_options opts = { 0 };
> > +	struct config_options opts = {
> > +		.parse_options = CP_OPTS_INIT(CONFIG_ERROR_DIE),
> > +	};
> >  
> >  	opts.respect_includes = 1;
> >  	opts.ignore_repo = 1;
> 
> This uses a bit more assignments to various members of opts. to
> initialize it, which could have been done with designated
> initializer, like the one in read_protected_config() used to do.
> 
> > @@ -2760,12 +2784,14 @@ int repo_config_get_pathname(struct repository *repo,
> >  static void read_protected_config(void)
> >  {
> >  	struct config_options opts = {
> > -		.respect_includes = 1,
> > -		.ignore_repo = 1,
> > -		.ignore_worktree = 1,
> > -		.system_gently = 1,
> > +		.parse_options = CP_OPTS_INIT(CONFIG_ERROR_DIE),
> >  	};
> >  
> > +	opts.respect_includes = 1;
> > +	opts.ignore_repo = 1;
> > +	opts.ignore_worktree = 1;
> > +	opts.system_gently = 1;
> > +
> 
> It is curious why you want to switch to manual assignment, instead
> of keeping the designated initializer for this one.  I would have
> expected the initialization in read_very_early_config() to start
> using designated initializer to be consistent, instead.
> 
> Thanks.

Agreed, fixed here and above.