Re: [PATCH] alias.c: use git_config_string() to get alias_val
- From
Christian Couder <chriscool@tuxfamily.org>
- Date
- Apr 6, 2008, 05:49 UTC
- Message-ID
- <200804060749.34109.chriscool@tuxfamily.org>
- In-Reply-To
- <20080405173956.GD14565@leksak.fem-net>
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<----
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.