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

From: Jeff King <peff@peff.net>
Date: Mar 2, 2016, 02:51 UTC
Message-ID: <20160302025143.GB20625@sigill.intra.peff.net>
In-Reply-To: <1456867206.18017.94.camel@twopensource.com>

On Tue, Mar 01, 2016 at 04:20:06PM -0500, David Turner wrote:

Show 14 quoted lines

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

Yeah, I waffled on it. The main reason was simply that the existing code it's replacing generally wanted to have a stateful return, do some cleanup, and then check "did we get a version?".

It would be easy to provide both (which matches the redundancy of nongit_ok).

Show 7 quoted lines

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

I meant that internally to setup.c, we need to call with different variants of "fn", but that is an ugly interface we don't need to expose outside the file. And further on in the series, we clean that up, and can drop this internal interface. Maybe it's not even worth commenting on (or commenting on in the commit message only).

Show 7 quoted lines

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

Only that I consider this to be a more object-oriented interface (akin to strbuf, argv_array, etc), with "struct repository_format" as the "this" object. We usually put that argument first.

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

Thanks, I blindly replaced warning(), which handles the newline. I'll double-check the other conversions to sure we handle newlines there, too.

-Peff