Re: [PATCH 06/10] setup: refactor repo format reading and verification

[David Turner <dturner@twopensource.com>]

On Tue, 2016-03-01 at 09:42 -0500, Jeff King wrote:
> +/*
> + * Read the repository format characteristics from the config file
> "path". If
> + * the version cannot be extracted from the file for any reason, the
> version
> + * field will be set to -1, and all other fields are undefined.
> + */
> +void read_repository_format(struct repository_format *format, const
> char *path);

Generally speaking, I don't care for this interface; I would prefer to
return -1 and not change the struct.  But I see why it's maybe simpler
in this one case.

+/*
> + * Internally, we need to swap out "fn" here, but we don't want to
> expose
> + * that to the world. Hence a wrapper around this internal function.
> + */

I don't understand this comment.  fn is not being swapped out -- it's
being passed on directly.  

> +static void read_repository_format_1(struct repository_format
> *format,
> +				     config_fn_t fn, const char
> *path)

The argument order here is different from git_config_from_file -- is
that for a reason?

> +	if (format->version >= 1 && format->unknown_extensions.nr) {
> +		int i;
> +
> +		for (i = 0; i < format->unknown_extensions.nr; i++)
> +			strbuf_addf(err, "unknown repository
> extension: %s",
> +				    format
> ->unknown_extensions.items[i].string);

newline here or something?  Otherwise multiple unknown extensions will
get jammed together.