On 2023.08.23 18:19, Junio C Hamano wrote: > Josh Steadmon <steadmon@xxxxxxxxxx> writes: > > > From: Glen Choo <chooglen@xxxxxxxxxx> > > > > 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@xxxxxxxxxx/ > > @@ -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.
