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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
