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

From: David Turner <dturner@twopensource.com>
Date: Mar 1, 2016, 21:20 UTC
Message-ID: <1456867206.18017.94.camel@twopensource.com>
In-Reply-To: <20160301144206.GF12887@sigill.intra.peff.net>

On Tue, 2016-03-01 at 09:42 -0500, Jeff King wrote:

Show 9 quoted lines

> +/*
> + * 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?

Show 8 quoted lines

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