Re: [PATCH/RFC] alias.c: Replace git_config with git_config_get_string

From: Jeff King <peff@peff.net>
Date: Jun 17, 2014, 05:43 UTC
Message-ID: <20140617054357.GC29957@sigill.intra.peff.net>
In-Reply-To: <1402910154-417-1-git-send-email-tanayabh@gmail.com>

On Mon, Jun 16, 2014 at 02:15:54AM -0700, Tanay Abhra wrote:

Show 13 quoted lines

> **DOUBT**
> This patch builds on top of patch series[1]. The first patch in the 
> replace `git_config` series is [2], which passed all the tests.
> 
> But this patch falters at this test in t1300-repo-config.sh,
> 
> git config alias.checkconfig "-c foo.check=bar config foo.check" &&
> 		echo bar >expect &&
> 		git checkconfig >actual &&
> 		test_cmp expect actual
> 
> I hand tested this case and the outputs match. But I don't know why the tests
> are failing.

I get:

    expecting success: 
            git config alias.split-cmdline-fix 'echo "' &&
            test_must_fail git split-cmdline-fix &&
            echo foo > foo &&
            git add foo &&
            git commit -m 'initial commit' &&
            git config branch.master.mergeoptions 'echo "' &&
            test_must_fail git merge master
    
    Segmentation fault
    test_must_fail: died by signal: git split-cmdline-fix

Running with valgrind gives more details (it looks like the segfault I mentioned in the other thread).

Show 10 quoted lines

>  char *alias_lookup(const char *alias)
>  {
> -	alias_key = alias;
> -	alias_val = NULL;
> -	git_config(alias_lookup_cb, NULL);
> +	char *alias_key, *alias_val;
> +	const char *v;
> +	alias_key = xmalloc(7+strlen(alias));
> +	strcpy(alias_key, "alias.");
> +	strcat(alias_key, alias);

Please use a strbuf instead of hand-rolling the math. It's much easier to verify that it is correct, and it avoids badly designed functions like strcat. I.e.:

  struct strbuf key = STRBUF_INIT;
  strbuf_addf(&key, "alias.%s", alias);
  ...
  strbuf_release(&key);

(note also that since the key/val variables are no longer static
 globals, it's fine to use a shorter, less clunky name).

> +	v = git_config_get_string(alias_key);
> +	if (!v)
> +		config_error_nonbool(alias_key);

What does a NULL output from git_config_get_string mean? I think with the current code, it means "no such key was found". In which case, you should be returning NULL here (there is no such alias), not complaining with config_error_nonbool.

Again, this is going to depend on your strategy for storing booleans that I mentioned elsewhere.

-Peff