threads / patch / 12992

patchalias.c: use git_config_string() to get alias_val

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

## tl;dr

6 messages between Apr 5, 2008 and Apr 6, 2008. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Stephan Beyer· Apr 5, 2008, 12:18 UTC · lore
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(-)
Show changes to alias.c +2 −6
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
Christian Couder· Apr 5, 2008, 16:19 UTC · re: Stephan Beyer · lore

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

Le samedi 5 avril 2008, Stephan Beyer a écrit :
Show 5 quoted lines
> 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 ?
Show 25 quoted lines
> 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.

Stephan Beyer· Apr 5, 2008, 17:39 UTC · re: Christian Couder · lore

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

Hi,
Show 6 quoted lines
> Did you see:
> 
> "(And no, casting the "char **" into a "const char **" is not a good 
> solution either.)"
> 
> in the above page ?
Yes.
Show 5 quoted lines
> > +	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
Christian Couder· Apr 6, 2008, 05:49 UTC · re: Stephan Beyer · lore

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

Hi,
Le samedi 5 avril 2008, Stephan Beyer a écrit :
Show 10 quoted lines
> 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.

Show 28 quoted lines
> > > +	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.

Show 11 quoted lines
> 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<----
Show changes to git.c +12 −17
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.
Stephan Beyer· Apr 6, 2008, 11:02 UTC · re: Christian Couder · lore

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

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.

Show 5 quoted lines
> > 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: --

Show changes to builtin-apply.c +2 −7
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


>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
Christian Couder· Apr 6, 2008, 13:29 UTC · re: Stephan Beyer · lore

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

Le dimanche 6 avril 2008, Stephan Beyer a écrit :
Show 26 quoted lines
> 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.

← back to recent threads