Hi Junio,
On Sun, Feb 24, 2013 at 09:54:43PM -0800, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@xxxxxxxxxx> writes:
>
> > 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.
Thanks for taking a look. You suggestion sounds reasonable, I will
modify my patch accordingly.
Cheers Heiko
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html