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

[Christian Couder <chriscool@tuxfamily.org>]

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.