Re: [RFC/WIP PATCH 1/3] teach config parsing to read from strbuf

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 25, 2013, 05:54 UTC
Message-ID: <7v4nh13plo.fsf@alter.siamese.dyndns.org>
In-Reply-To: <6c69068b4e6a72a2cca5dc6eaffa9982032a7f2a.1361751905.git.hvoigt@hvoigt.net>

Heiko Voigt <hvoigt@hvoigt.net> writes:

Show 14 quoted lines

> diff --git a/config.c b/config.c
> index aefd80b..f995e98 100644
> --- a/config.c
> +++ b/config.c
> @@ -13,6 +13,9 @@
>  typedef struct config_file {
>  	struct config_file *prev;
>  	FILE *f;
> +	int is_strbuf;
> +	struct strbuf *strbuf_contents;
> +	int strbuf_pos;
>  	const char *name;
>  	int linenr;
>  	int eof;

The idea to allow more kinds of sources specified for "config_file" structure is not bad per-se, but whenever you design an enhancement to something that currently supports only on thing to allow taking another kind, please consider what needs to be done by the next person who adds the third kind. That would help catch design mistakes early. For example, will the "string-list" (I am not saying use of string-list makes sense as the third kind; just as an example off the top of my head) source patch add

	int is_string_list;
        struct string_list *string_list_contents;

fields to this structure?  Sounds insane for at least two reasons:

 * if both is_strbuf and is_string_list are true, what should
   happen?

 * is there a good reason to waste storage for the three fields your
   patch adds when sring_list strage (or FILE * storage for that
   matter) is used?

The helper functions like config_fgetc() and config_ftell() sounds like you are going in the right direction but may want to do the OO-in-C in a similar way transport.c does, keeping a pointer to a structure of methods, but I didn't read the remainder of this patch very carefully enough to comment further.