On Tue, Jun 14, 2011 at 07:19:19PM +0100, Ramsay Jones wrote:
> > 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.
>
> 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);
> }
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.
> 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).
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
--
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