[PATCH] alias.c: use git_config_string() to get alias_val

[PATCH] alias.c: use git_config_string() to get alias_val

[Stephan Beyer]

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
Hi,

This is a `Janitor patch' to get involved ;-)
See
	http://git.or.cz/gitwiki/Janitor

It does not (at least, should not) change any functionality and
it has been tested using some aliases.

Regards,
Stephan

 alias.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/alias.c b/alias.c
index 116cac8..ac88d38 100644
--- a/alias.c
+++ b/alias.c
@@ -4,12 +4,8 @@ static const char *alias_key;
 static char *alias_val;
 static int alias_lookup_cb(const char *k, const char *v)
 {
-	if (!prefixcmp(k, "alias.") && !strcmp(k+6, alias_key)) {
-		if (!v)
-			return config_error_nonbool(k);
-		alias_val = xstrdup(v);
-		return 0;
-	}
+	if (!prefixcmp(k, "alias.") && !strcmp(k+6, alias_key))
+		return git_config_string((const char**)&alias_val, k, v);
 	return 0;
 }
 
-- 
1.5.4.4

Re: [PATCH] alias.c: use git_config_string() to get alias_val

[Christian Couder]

Le samedi 5 avril 2008, Stephan Beyer a écrit :
> Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
> ---
> Hi,
>
> This is a `Janitor patch' to get involved ;-)

Great!

> See
> 	http://git.or.cz/gitwiki/Janitor

Did you see:

"(And no, casting the "char **" into a "const char **" is not a good 
solution either.)"

in the above page ?

> It does not (at least, should not) change any functionality and
> it has been tested using some aliases.
>
> Regards,
> Stephan
>
>  alias.c |    8 ++------
>  1 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/alias.c b/alias.c
> index 116cac8..ac88d38 100644
> --- a/alias.c
> +++ b/alias.c
> @@ -4,12 +4,8 @@ static const char *alias_key;
>  static char *alias_val;
>  static int alias_lookup_cb(const char *k, const char *v)
>  {
> -	if (!prefixcmp(k, "alias.") && !strcmp(k+6, alias_key)) {
> -		if (!v)
> -			return config_error_nonbool(k);
> -		alias_val = xstrdup(v);
> -		return 0;
> -	}
> +	if (!prefixcmp(k, "alias.") && !strcmp(k+6, alias_key))
> +		return git_config_string((const char**)&alias_val, k, v);

Are you sure this ugly cast to "const char**" is needed ?
Isn't there a better way to do it ?

>  	return 0;
>  }

Thanks,
Christian.

Re: [PATCH] alias.c: use git_config_string() to get alias_val

[Stephan Beyer]

Hi,

> Did you see:
> 
> "(And no, casting the "char **" into a "const char **" is not a good 
> solution either.)"
> 
> in the above page ?

Yes.

> > +	if (!prefixcmp(k, "alias.") && !strcmp(k+6, alias_key))
> > +		return git_config_string((const char**)&alias_val, k, v);
> 
> Are you sure this ugly cast to "const char**" is needed ?
> Isn't there a better way to do it ?

Well, because alias_val is not a constant[1], changing
	static char *alias_val;
to
	static const char *alias_val;
is not an option.

The only other way I see[2] atm is a brain-damaged:
--
static int alias_lookup_cb(const char *k, const char *v)
{
	if (!prefixcmp(k, "alias.") && !strcmp(k+6, alias_key)) {
		const char *tmp;
		int ret = git_config_string(&tmp, k, v);
		alias_val = xstrdup(tmp);
		/* actually, tmp should be free()'d. */
		return ret;
	}
	return 0;
}
--
But instead of doing that, the original should be kept, because it is
better in code beauty, performance and memory usage. ;-)

So I thought the casting is ugly, but it does no harm. I hope ;)
(Yes, a cast from const char ** to char ** is, indeed, dangerous.)

But if I miss an obvious point, please tell me :)

Regards,
 Stephan

Footnotes:
 [1] It is no constant because it is returned by alias_lookup(),
     and thus could be changed by further instructions.
 [2] ...between the many ways that result in compiler warnings ;-)

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

Re: [PATCH] alias.c: use git_config_string() to get alias_val

[Christian Couder]

Hi,

Le samedi 5 avril 2008, Stephan Beyer a écrit :
> Hi,
>
> > Did you see:
> >
> > "(And no, casting the "char **" into a "const char **" is not a good
> > solution either.)"
> >
> > in the above page ?
>
> Yes.

So you should say in the commit message that you decided to cast to "const 
char **" despite what is on the Janitor page, and most importantly explain 
why in the commit message.

> > > +	if (!prefixcmp(k, "alias.") && !strcmp(k+6, alias_key))
> > > +		return git_config_string((const char**)&alias_val, k, v);
> >
> > Are you sure this ugly cast to "const char**" is needed ?
> > Isn't there a better way to do it ?
>
> Well, because alias_val is not a constant[1], changing
> 	static char *alias_val;
> to
> 	static const char *alias_val;
> is not an option.
>
> The only other way I see[2] atm is a brain-damaged:
> --
> static int alias_lookup_cb(const char *k, const char *v)
> {
> 	if (!prefixcmp(k, "alias.") && !strcmp(k+6, alias_key)) {
> 		const char *tmp;
> 		int ret = git_config_string(&tmp, k, v);
> 		alias_val = xstrdup(tmp);
> 		/* actually, tmp should be free()'d. */
> 		return ret;
> 	}
> 	return 0;
> }
> --
> But instead of doing that, the original should be kept, because it is
> better in code beauty, performance and memory usage. ;-)

Yes, so perhaps it's not a good idea to convert the original file to 
git_config_string.

> So I thought the casting is ugly, but it does no harm. I hope ;)
> (Yes, a cast from const char ** to char ** is, indeed, dangerous.)
>
> But if I miss an obvious point, please tell me :)
>
> Regards,
>  Stephan
>
> Footnotes:
>  [1] It is no constant because it is returned by alias_lookup(),
>      and thus could be changed by further instructions.

Yes, but there are only 2 callers and only one in git.c changes the buffer.

A patch like this (not tested) one makes use of a strbuf to copy the buffer 
returned by alias_lookup in git.c, so that it is now possible (if we really 
want it) to change alias_lookup to return a "const char *" instead of 
a "char *":

----8<----

diff --git a/git.c b/git.c
index c4e4644..ba5593f 100644
--- a/git.c
+++ b/git.c
@@ -147,34 +147,29 @@ static int handle_alias(int *argcp, const char 
***argv)
 	int count, option_count;
 	const char** new_argv;
 	const char *alias_command;
-	char *alias_string;
+	struct strbuf alias_buf = STRBUF_INIT;
+	char *cmdline;
 	int unused_nongit;
 
 	subdir = setup_git_directory_gently(&unused_nongit);
 
 	alias_command = (*argv)[0];
-	alias_string = alias_lookup(alias_command);
-	if (alias_string) {
-		if (alias_string[0] == '!') {
-			if (*argcp > 1) {
-				struct strbuf buf;
-
-				strbuf_init(&buf, PATH_MAX);
-				strbuf_addstr(&buf, alias_string);
-				sq_quote_argv(&buf, (*argv) + 1, PATH_MAX);
-				free(alias_string);
-				alias_string = buf.buf;
-			}
+	strbuf_addstr(&alias_buf, alias_lookup(alias_command));
+	if (alias_buf.len) {
+		if (alias_buf.buf[0] == '!') {
+			if (*argcp > 1)
+				sq_quote_argv(&alias_buf, (*argv) + 1, PATH_MAX);
 			trace_printf("trace: alias to shell cmd: %s => %s\n",
-				     alias_command, alias_string + 1);
-			ret = system(alias_string + 1);
+				     alias_command, alias_buf.buf + 1);
+			ret = system(alias_buf.buf + 1);
 			if (ret >= 0 && WIFEXITED(ret) &&
 			    WEXITSTATUS(ret) != 127)
 				exit(WEXITSTATUS(ret));
 			die("Failed to run '%s' when expanding alias '%s'\n",
-			    alias_string + 1, alias_command);
+			    alias_buf.buf + 1, alias_command);
 		}
-		count = split_cmdline(alias_string, &new_argv);
+		cmdline = strbuf_detach(&alias_buf, NULL);
+		count = split_cmdline(cmdline, &new_argv);
 		option_count = handle_options(&new_argv, &count, &envchanged);
 		if (envchanged)
 			die("alias '%s' changes environment variables\n"

----8<----

But I don't think it's worth the trouble.

Thanks,
Christian.

Re: [PATCH] alias.c: use git_config_string() to get alias_val

[Stephan Beyer]

[-- Attachment #1: Type: text/plain, Size: 1754 bytes --]

Hi,

> So you should say in the commit message that you decided to cast to "const 
> char **" despite what is on the Janitor page, and most importantly explain 
> why in the commit message.

Ah, ok.
Didn't knew that such reasoning goes into the commit message,
but, yes, it makes sense.

> > But instead of doing that, the original should be kept, because it is
> > better in code beauty, performance and memory usage. ;-)
> 
> Yes, so perhaps it's not a good idea to convert the original file to 
> git_config_string.

Or to use the cast. ;-)

[...]
> But I don't think it's worth the trouble.

I agree ;-)

So what to do?
Keep it?



And there are still some easy `Janitor tasks', like in builtin-apply.c:
--
diff --git a/builtin-apply.c b/builtin-apply.c
index b5f78ac..ce0a0c3 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2978,12 +2978,8 @@ static int apply_patch(int fd, const char *filename, int inaccurate_eof)
 
 static int git_apply_config(const char *var, const char *value)
 {
-	if (!strcmp(var, "apply.whitespace")) {
-		if (!value)
-			return config_error_nonbool(var);
-		apply_default_whitespace = xstrdup(value);
-		return 0;
-	}
+	if (!strcmp(var, "apply.whitespace"))
+		return git_config_string(&apply_default_whitespace, var, value);
 	return git_default_config(var, value);
 }
 
--
(Test case: git-apply a patch with trailing whitespaces and check with
apply.whitespace = nowarn / fix / error)

I have attached the patch because I do not want to drop another 
mail for it.
And now there is nothing to argue about, I hope, because it is as simple as
http://git.kernel.org/?p=git/git.git;a=commitdiff;h=9886ea417b7da9722c95630b5980ac174e04c71c


Regards,
 Stephan

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

[-- Attachment #2: 0001-builtin-apply.c-use-git_config_string-to-get-appl.patch --]
[-- Type: text/x-diff, Size: 996 bytes --]

>From 78d1ac0fd19b274853d3c8439d45922c1a0fe82d Mon Sep 17 00:00:00 2001
From: Stephan Beyer <s-beyer@gmx.net>
Date: Sun, 6 Apr 2008 12:37:31 +0200
Subject: [PATCH] builtin-apply.c: use git_config_string() to get apply_default_whitespace

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
 builtin-apply.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index b5f78ac..ce0a0c3 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2978,12 +2978,8 @@ static int apply_patch(int fd, const char *filename, int inaccurate_eof)
 
 static int git_apply_config(const char *var, const char *value)
 {
-	if (!strcmp(var, "apply.whitespace")) {
-		if (!value)
-			return config_error_nonbool(var);
-		apply_default_whitespace = xstrdup(value);
-		return 0;
-	}
+	if (!strcmp(var, "apply.whitespace"))
+		return git_config_string(&apply_default_whitespace, var, value);
 	return git_default_config(var, value);
 }
 
-- 
1.5.5.rc3.8.g78d1a

Re: [PATCH] alias.c: use git_config_string() to get alias_val

[Christian Couder]

Le dimanche 6 avril 2008, Stephan Beyer a écrit :
> Hi,
>
> > So you should say in the commit message that you decided to cast to
> > "const char **" despite what is on the Janitor page, and most
> > importantly explain why in the commit message.
>
> Ah, ok.
> Didn't knew that such reasoning goes into the commit message,
> but, yes, it makes sense.
>
> > > But instead of doing that, the original should be kept, because it is
> > > better in code beauty, performance and memory usage. ;-)
> >
> > Yes, so perhaps it's not a good idea to convert the original file to
> > git_config_string.
>
> Or to use the cast. ;-)
>
> [...]
>
> > But I don't think it's worth the trouble.
>
> I agree ;-)
>
> So what to do?
> Keep it?

No, I think its better to do nothing on "alias.c".

> And there are still some easy `Janitor tasks', like in builtin-apply.c:

Yes. And thanks, your patch looks good, but please send a proper patch for 
it latter if you really care about it.

(By proper I mean in its own email and not attached. And I know my patch 
included in my previous email was bad (corrupted tab, sorry about that,  
and not in its own email), but it was only for discussion, yours too 
granted.)

Now I think that you should focus on your GSoC application as you don't have 
much time left to improve it and I just asked you for more information.

Thanks,
Christian.