Re: [RFC/PATCH] config.c: Make git_config() work correctly when called recursively

From: Jeff King <peff@peff.net>
Date: Jun 9, 2011, 20:39 UTC
Message-ID: <20110609203958.GA4671@sigill.intra.peff.net>
In-Reply-To: <4DF106B8.2080902@ramsay1.demon.co.uk>

On Thu, Jun 09, 2011 at 06:45:28PM +0100, Ramsay Jones wrote:

Show 23 quoted lines

> The recursive call to git_config() is due to the "schizophrenic stat"
> functions on cygwin, and is arrived at as follows:
> 
>   cmd_notes() : builtin/notes.c:1057
>     =>copy() @1084: builtin/notes.c:605
>       =>notes_copy_from_stdin() @630: builtin/notes.c:418
>         =>init_copy_notes_for_rewrite() @426: builtin/notes.c:359
>           =>git_config() @384: config.c:876
> 
>   *cb=>notes_rewite_config() : builtin/notes.c:329
>     =>string_list_add_refs_by_glob() @348 : notes.c:936
>       =>for_each_glob_ref() @939: refs.c:815
>         =>for_each_glob_ref_in() @817: refs.c:785
>           =>for_each_ref() @809: refs.c:729
>             =>do_for_each_ref() @731: refs.c:658
>               =>get_loose_refs() @663: refs.c:359
>                 =>get_ref_dir() @368: refs.c:258
>                   =>stat() @299: compat/cygwin.h:8
> 
>   stat macro => indirect call: *cygwin_stat_fn : compat/cygwin.c:141
>     =>cygwin_stat_stub() : compat/cygwin.c:131
>       =>init_stat() @133: compat/cygwin.c:115
>         =>git_config() @118: config.c:876

Wow, that's quite a call-chain.

> I have not sent this patch to the list before, since I had planned to find
> a different solution first, so this (updated patch) is more by way of an
> extended bug-report! Having said that, it works fine ...

I think it's actually a pretty sane approach. Your other option would be not lazily configuring cygwin stat, which means putting an extra very early stat call somewhere.

But look at how deep the call chain above is. And consider how the bug manifested itself: silently ignoring some config. So I wouldn't be terribly surprised if there is another such recursion hiding somewhere, or if we manage to introduce one accidentally in the future.

So making git_config safe for recursion seems like a good solution for future maintainability.

>  config.c |   80 ++++++++++++++++++++++++++++++++++++++-----------------------

The patch looks sane from my quick skim, though:

Show 15 quoted lines

> -static FILE *config_file;
> -static const char *config_file_name;
> -static int config_linenr;
> -static int config_file_eof;
> +typedef struct config_file {
> +	struct config_file *prev;
> +	FILE *f;
> +	const char *name;
> +	int linenr;
> +	int eof;
> +	struct strbuf value;
> +	char var[MAXNAME];
> +} config_file;
> +
> +static config_file *cf;

Since you've nicely encapsulated all of the global data in this struct, maybe it is worth simply passing a struct pointer down the call-chain instead of relying on a global pointer. Then you can let the language do its job and stop managing the stack yourself.

-Peff