Re: [PATCH 1/4] config: factor out config file stack management

2013-02-26 Thread Jeff King 
On Tue, Feb 26, 2013 at 08:38:50PM +0100, Heiko Voigt wrote:

 Because a config callback may start parsing a new file, the
 global context regarding the current config file is stored
 as a stack. Currently we only need to manage that stack from
 git_config_from_file. Let's factor it out to allow new
 sources of config data.
 
 Signed-off-by: Heiko Voigt hvo...@hvoigt.net
 ---
 
 Peff, I hope you do not mind that I totally copied your commit message
 here.

I don't mind at all.

 The patch takes a different approach though. If you like we can add a
 
   Commit-Message-by: Jeff King p...@peff.net
 
 here :-)

I think my name is plastered all over git log enough as it is.

 +static int do_config_from(struct config_file *top, config_fn_t fn, void 
 *data)
 +{
 + int ret;
 +
 + /* push config-file parsing state stack */
 + top-prev = cf;
 + top-linenr = 1;
 + top-eof = 0;
 + strbuf_init(top-value, 1024);
 + strbuf_init(top-var, 1024);
 + cf = top;
 +
 + ret = git_parse_file(fn, data);
 +
 + /* pop config-file parsing state stack */
 + strbuf_release(top-value);
 + strbuf_release(top-var);
 + cf = top-prev;
 +
 + return ret;
 +}

This function name is a bit weird. I would have thought the from here
was going to be a file, or a string, or whatever. But the filename setup
happens outside this function (and yet this function depends on it being
set up, as it calls git_parse_file). But maybe it will get less
confusing with the other patches on top...

-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

Re: [PATCH 1/4] config: factor out config file stack management

2013-02-26 Thread Heiko Voigt 
On Tue, Feb 26, 2013 at 02:54:49PM -0500, Jeff King wrote:
 On Tue, Feb 26, 2013 at 08:38:50PM +0100, Heiko Voigt wrote:
  +static int do_config_from(struct config_file *top, config_fn_t fn, void 
  *data)
  +{
  +   int ret;
  +
  +   /* push config-file parsing state stack */
  +   top-prev = cf;
  +   top-linenr = 1;
  +   top-eof = 0;
  +   strbuf_init(top-value, 1024);
  +   strbuf_init(top-var, 1024);
  +   cf = top;
  +
  +   ret = git_parse_file(fn, data);
  +
  +   /* pop config-file parsing state stack */
  +   strbuf_release(top-value);
  +   strbuf_release(top-var);
  +   cf = top-prev;
  +
  +   return ret;
  +}
 
 This function name is a bit weird. I would have thought the from here
 was going to be a file, or a string, or whatever. But the filename setup
 happens outside this function (and yet this function depends on it being
 set up, as it calls git_parse_file). But maybe it will get less
 confusing with the other patches on top...

The do_config_from means parse from whatever is in 'top'. Later in
the series its type changes from config_file to struct config.

The name 'git_parse_file' becomes definitely wrong after this series.
Maybe I should rename it?

Cheers Heiko
--
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

Re: [PATCH 1/4] config: factor out config file stack management

2013-02-26 Thread Jeff King 
On Tue, Feb 26, 2013 at 09:09:41PM +0100, Heiko Voigt wrote:

  This function name is a bit weird. I would have thought the from here
  was going to be a file, or a string, or whatever. But the filename setup
  happens outside this function (and yet this function depends on it being
  set up, as it calls git_parse_file). But maybe it will get less
  confusing with the other patches on top...
 
 The do_config_from means parse from whatever is in 'top'. Later in
 the series its type changes from config_file to struct config.

Ah, I see. The from is the struct config.

I wonder if it would be more obvious with the more usual OO-struct
functions, like:

  struct config_source {
  ...
  };
  void config_source_init_file(struct config_source *, const char *fn);
  void config_source_init_strbuf(struct config_source *,
 const struct strbuf *buf);
  void config_source_clear(struct config_source *);

  int config_source_parse(struct config_source *);

and then the use would be something like:

  struct config_source top;
  int ret;

  config_source_init_file(top, foo);
  ret = config_source_parse(top);
  config_source_clear(top);

  return ret;

I.e., init constructors, a clear destructor, and any methods like
parse that you need.  I haven't though too hard about it, though, so
maybe there is some reason it does not fit that model (it is a little
uncommon that the init would push itself onto a stack, but I think
that's OK).

-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

Re: [PATCH 1/4] config: factor out config file stack management

2013-02-26 Thread Junio C Hamano 
Jeff King p...@peff.net writes:

 I wonder if it would be more obvious with the more usual OO-struct
 functions, like:

   struct config_source {
   ...
   };
   void config_source_init_file(struct config_source *, const char *fn);
   void config_source_init_strbuf(struct config_source *,
  const struct strbuf *buf);
   void config_source_clear(struct config_source *);

   int config_source_parse(struct config_source *);

 and then the use would be something like:

   struct config_source top;
   int ret;

   config_source_init_file(top, foo);
   ret = config_source_parse(top);
   config_source_clear(top);

   return ret;

 I.e., init constructors, a clear destructor, and any methods like
 parse that you need.

Yup, that cocincides with my first impression I sent out for the
previous RFC/PATCH round.
--
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

Re: [PATCH 1/4] config: factor out config file stack management

2013-02-26 Thread Junio C Hamano 
Heiko Voigt hvo...@hvoigt.net writes:

 The do_config_from means parse from whatever is in 'top'. Later in
 the series its type changes from config_file to struct config.

Yuck.  It would be nice to have it as struct config_src or
something. struct config sounds as if it represents the entire
configuration state and you can ask it to add new ones or enumerate
all known configuration variables, etc.
--
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

Re: [PATCH 1/4] config: factor out config file stack management

2013-02-26 Thread Heiko Voigt 
On Tue, Feb 26, 2013 at 02:10:03PM -0800, Junio C Hamano wrote:
 Jeff King p...@peff.net writes:
 
  I wonder if it would be more obvious with the more usual OO-struct
  functions, like:
 
struct config_source {
...
};
void config_source_init_file(struct config_source *, const char *fn);
void config_source_init_strbuf(struct config_source *,
   const struct strbuf *buf);
void config_source_clear(struct config_source *);
 
int config_source_parse(struct config_source *);
 
  and then the use would be something like:
 
struct config_source top;
int ret;
 
config_source_init_file(top, foo);
ret = config_source_parse(top);
config_source_clear(top);
 
return ret;
 
  I.e., init constructors, a clear destructor, and any methods like
  parse that you need.
 
 Yup, that cocincides with my first impression I sent out for the
 previous RFC/PATCH round.

I agree that your suggestions would make the design more OO and provide
us with more flexibility. However at the following costs:

Currently the push and pop are combined into one function. This design
makes it impossible to forget the cleanup if someone else uses this
function. The same applies to the private data handling around
do_config_from().

All memory is currently handled on the stack. If we have an init/clear
design at least the private data for strbuf needs to be put on the heap.
We could pass in the config_strbuf but IMO that would violate the
encapsulation.

Thus we also require a specialized clear which frees that private data
(we need that to close the file anyway). Because of that I would probably
call it destroy or free.

That means there will be six additional functions instead of one: We
need init and clear for both, a common init and a common clear for the
push/pop part. IMO that will make the code harder to read for the first
time.

Then we would have a hybrid stack/heap solution still involving side
effects (setting the global cf).

Do not get me wrong. I am not against changing it but I would like to
spell out the consequences first before doing so.

Do you still think that is nicer approach?

Cheers Heiko
--
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

Re: [PATCH 1/4] config: factor out config file stack management

2013-02-26 Thread Heiko Voigt 
On Tue, Feb 26, 2013 at 02:12:23PM -0800, Junio C Hamano wrote:
 Heiko Voigt hvo...@hvoigt.net writes:
 
  The do_config_from means parse from whatever is in 'top'. Later in
  the series its type changes from config_file to struct config.
 
 Yuck.  It would be nice to have it as struct config_src or
 something. struct config sounds as if it represents the entire
 configuration state and you can ask it to add new ones or enumerate
 all known configuration variables, etc.

Will change it to struct config_source. I choose that name in lack of a
better one. Since it can be considered the base for both sources I just
removed the _file postfix.

Cheers Heiko
--
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