RFC patchconfig.c: Make git_config() work correctly when called recursively

On Cygwin, this fixes a test failure in t3301-notes.sh (test 98, "git notes copy --for-rewrite (disabled)").

The test failure is caused by a recursive call to git_config() which has the effect of skipping to the end-of-file while processing the "notes.rewriteref" config variable. Thus, any config variables that appear after "notes.rewriteref" are simply ignored by git_config().

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

[The line numbers are from a somewhat recent version of git, but are just quoted as a guide; some of them have probably already changed]

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---

While rebasing my 'cygwin-tests' branch recently, this commit conflicted with e96c19c5 (config: support values longer than 1023 bytes, 10-04-2011), due to the replacement of the value char array with a strbuf.

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 had also intended to check if a similar change for the key (currently limited to MAXNAME) would be a good idea; my initial reaction was no since we have control of the key names, with only a few exceptions eg. some keys contain branch names ...]

ATB, Ramsay Jones

 config.c |   80 ++++++++++++++++++++++++++++++++++++++-----------------------
 1 files changed, 50 insertions(+), 30 deletions(-)

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

Wow, that's quite a call-chain.

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.

The patch looks sane from my quick skim, though:

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

[Sorry for the late reply - I've been away from email for several days...]

Jeff King wrote:

[...]

:-P

Indeed, that is exactly why this was my first (and so far only) solution to this problem; it is simple, safe and resilient in the face of future patches! [I'm not going to commit to auditing all of the current calls to git_config(), checking all subsequent call-chains for calls to [l]stat(), let alone all future changes to git.]

However, I anticipated some resistance to this approach, so I have intended to find another solution (for about 4 months now!), in order to present the list with some alternatives. I would be quite happy not to bother ;-)

[Actually, one of my preferred solutions is to revert commit adbc0b6 et. seq., but that is a whole different discussion!]

The first version of this patch did exactly that! However, that first patch failed the test-suite. :(

The failure was caused by a change to the die_bad_config() function, which in the current patch looks like this:

    @@ -374,8 +381,8 @@ int git_parse_ulong(const char *value, unsigned long *ret)
 
     static void die_bad_config(const char *name)
     {
    -	if (config_file_name)
    -		die("bad config value for '%s' in %s", name, config_file_name);
    +	if (cf && cf->name)
    +		die("bad config value for '%s' in %s", name, cf->name);
     	die("bad config value for '%s'", name);
     }

In order not to change the public interface (note that git_config_int() and git_config_ulong() call die_bad_config()), I had to change this function so that it only contains the final die() call. Thus, the error message no longer included the config filename. This caused the test called 'invalid unit' in t1300-repo-config.sh to fail.

I could, of course, have simply changed the expect file so that it would pass the test, but I wanted the change to be self-contained and to pass all existing tests (ie. the external interface/behaviour should *not* change).

So, I ended up with this (even simpler) patch.

Thank you for your comments on the patch.

ATB, Ramsay Jones

On Tue, Jun 14, 2011 at 07:19:19PM +0100, Ramsay Jones wrote:

Ah, right. We want to keep it as globals, because we end up in a callback which does not get the context passed to it.

No, you did the right thing here. The information on which config file we're in is valuable, and taking away the globals is not worth the pain of making all of the callers and callbacks of git_config have to deal with passing around a context struct.

So the patch you posted looks good to me.

-Peff

Jeff King wrote:

[...]

Thanks! (I will abandon the search for an alternate solution! ;-)

I think the commit message needs to be re-worded before actually submitting the patch (it's fine for an RFC, but the call-chain info has a limited shelf life and should go).

Also, I wanted to re-submit two additional patches in that branch; last time they didn't even make it to pu! Hopefully they will take at least one step further this time.

ATB, Ramsay Jones