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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 24, 2023, 01:19 UTC
Message-ID: <xmqqttspck4a.fsf@gitster.g>
In-Reply-To: <cb92a1f2e3098bede386d04da32fcc4f27fca51f.1692827403.git.steadmon@google.com>

Josh Steadmon <steadmon@google.com> writes:

Show 5 quoted lines

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

Show 14 quoted lines

> @@ -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?

Show 11 quoted lines

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

Show 16 quoted lines

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