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

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

[Tanay Abhra]

Original implementation uses a callback based approach which has some
deficiencies like a convoluted control flow and redundant variables.
Use git_config_get_string instead of git_config to take advantage of
the config hash-table.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---

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

In t1300-repo-config this test also fails,

#		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

Can anybody help me with this one?
Thanks.

[1] http://thread.gmane.org/gmane.comp.version-control.git/251704
[2] http://thread.gmane.org/gmane.comp.version-control.git/251707

Cheers,
Tanay Abhra.

 alias.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/alias.c b/alias.c
index 5efc3d6..fffad73 100644
--- a/alias.c
+++ b/alias.c
@@ -1,24 +1,17 @@
 #include "cache.h"
 
-static const char *alias_key;
-static char *alias_val;
-
-static int alias_lookup_cb(const char *k, const char *v, void *cb)
-{
-	if (starts_with(k, "alias.") && !strcmp(k + 6, alias_key)) {
-		if (!v)
-			return config_error_nonbool(k);
-		alias_val = xstrdup(v);
-		return 0;
-	}
-	return 0;
-}
-
 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);
+	v = git_config_get_string(alias_key);
+	if (!v)
+		config_error_nonbool(alias_key);
+	alias_val = xstrdup(v);
+	free(alias_key);
 	return alias_val;
 }
 
-- 
1.9.0.GIT

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

[Jeff King]

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

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

[Tanay Abhra]

On 06/16/2014 10:43 PM, Jeff King wrote:

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

Yes, you surmised correctly. I totally skipped the fact that git_config() can
return null for values indicating a boolean value. I will correct it in the next
patch.

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

I have read your other two replies related to it. I suggest the following approach
for git_config_get_string(), it will return,

1. Return null if no value was found for the entered key.

2. Empty string (""), returned for NULL values denoting boolean true in some cases.
   I think it would be much better than converting NULL to "true" or something else
   internally in the function.
   We can easily handle such cases as the above with a strcmp like,

+	v = git_config_get_string(alias_key);
+	if (!strcmp(v, ""))
+		config_error_nonbool(alias_key);

What do you think about this approach?

Thanks for the suggestion, I was pulling my hair out due this bug for last two days.

Cheers,
Tanay Abhra.

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

[Jeff King]

On Tue, Jun 17, 2014 at 11:51:35AM -0700, Tanay Abhra wrote:

> I have read your other two replies related to it. I suggest the following approach
> for git_config_get_string(), it will return,
> 
> 1. Return null if no value was found for the entered key.
> 
> 2. Empty string (""), returned for NULL values denoting boolean true in some cases.
>    I think it would be much better than converting NULL to "true" or something else
>    internally in the function.
>    We can easily handle such cases as the above with a strcmp like,
> 
> +	v = git_config_get_string(alias_key);
> +	if (!strcmp(v, ""))
> +		config_error_nonbool(alias_key);
> 
> What do you think about this approach?

Hmm. Then we can't distinguish between:

  [alias]
  foo

and

  [alias]
  foo =

I cannot think of a good reason to do the latter with aliases, but I
wonder if there are other config values for which "the empty string" is
a useful value.

I think I'd lean towards an interface which can actually express the
difference between "key not found" (1), "key present but no value" (2), and "key
present with some string" (3).

You could express (2) with a special pointer value, like:

  v = git_config_get_string(key);
  if (!v)
	return NULL; /* no key */
  else if (v == CONFIG_EMPTY_VALUE) {
	config_error_nonbool(key);
	return NULL;
  } else
	return xstrdup(v); /* actual value */

It's reasonably concise, but it's a little ugly that if you dereference
CONFIG_EMPTY_VALUE, you get random memory (on the other hand, we could
point it at 0x1, which would make it no different than NULL; you'd get a
segfault in either case).

Another option is to indicate (1) with a return value, and use a
separate parameter for the value. Like:

  if (git_config_get_string(key, &v) < 0)
	return NULL; /* no key */
  else if (!v) {
	config_error_nonbool(key); /* no value */
	return NULL;
  } else
	return xstrdup(v); /* actual value */

> Thanks for the suggestion, I was pulling my hair out due this bug for last two days.

You're welcome. This is exactly why we encourage students to get their
patches onto the list early for review. There is a lot of git expertise
available on the list. :)

-Peff