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

> **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).

>  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);

(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.

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

Reply via email to