On Tue, Mar 01, 2016 at 04:20:06PM -0500, David Turner wrote:
> 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).
> > + * 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).
> > +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.
> > + 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
