{"thread":{"id":"12992","subject":"[PATCH] alias.c: use git_config_string() to get alias_val","startedAt":"2008-04-05T12:18:35Z","lastAt":"2008-04-06T13:29:23Z","messageCount":6,"participants":["Stephan Beyer","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"73694","messageId":"20080405121834.GB14565@leksak.fem-net","threadId":"12992","inReplyTo":null,"subject":"[PATCH] alias.c: use git_config_string() to get alias_val","fromName":"Stephan Beyer","fromEmail":"s-beyer@gmx.net","sentAt":"2008-04-05T12:18:35Z","receivedAt":"2008-04-05T12:18:35Z","isPatch":true,"sender":{"key":"s-beyer@gmx.net","avatar":"https://avatars.githubusercontent.com/u/143889?v=4"},"body":"Signed-off-by: Stephan Beyer <s-beyer@gmx.net>\n---\nHi,\n\nThis is a `Janitor patch' to get involved ;-)\nSee\n\thttp://git.or.cz/gitwiki/Janitor\n\nIt does not (at least, should not) change any functionality and\nit has been tested using some aliases.\n\nRegards,\nStephan\n\n alias.c |    8 ++------\n 1 files changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/alias.c b/alias.c\nindex 116cac8..ac88d38 100644\n--- a/alias.c\n+++ b/alias.c\n@@ -4,12 +4,8 @@ static const char *alias_key;\n static char *alias_val;\n static int alias_lookup_cb(const char *k, const char *v)\n {\n-\tif (!prefixcmp(k, \"alias.\") && !strcmp(k+6, alias_key)) {\n-\t\tif (!v)\n-\t\t\treturn config_error_nonbool(k);\n-\t\talias_val = xstrdup(v);\n-\t\treturn 0;\n-\t}\n+\tif (!prefixcmp(k, \"alias.\") && !strcmp(k+6, alias_key))\n+\t\treturn git_config_string((const char**)&alias_val, k, v);\n \treturn 0;\n }\n \n-- \n1.5.4.4\n"},{"id":"73698","messageId":"200804051819.46904.chriscool@tuxfamily.org","threadId":"12992","inReplyTo":"20080405121834.GB14565@leksak.fem-net","subject":"Re: [PATCH] alias.c: use git_config_string() to get alias_val","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-04-05T16:19:46Z","receivedAt":"2008-04-05T16:19:46Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le samedi 5 avril 2008, Stephan Beyer a écrit :\n> Signed-off-by: Stephan Beyer <s-beyer@gmx.net>\n> ---\n> Hi,\n>\n> This is a `Janitor patch' to get involved ;-)\n\nGreat!\n\n> See\n> \thttp://git.or.cz/gitwiki/Janitor\n\nDid you see:\n\n\"(And no, casting the \"char **\" into a \"const char **\" is not a good \nsolution either.)\"\n\nin the above page ?\n\n> It does not (at least, should not) change any functionality and\n> it has been tested using some aliases.\n>\n> Regards,\n> Stephan\n>\n>  alias.c |    8 ++------\n>  1 files changed, 2 insertions(+), 6 deletions(-)\n>\n> diff --git a/alias.c b/alias.c\n> index 116cac8..ac88d38 100644\n> --- a/alias.c\n> +++ b/alias.c\n> @@ -4,12 +4,8 @@ static const char *alias_key;\n>  static char *alias_val;\n>  static int alias_lookup_cb(const char *k, const char *v)\n>  {\n> -\tif (!prefixcmp(k, \"alias.\") && !strcmp(k+6, alias_key)) {\n> -\t\tif (!v)\n> -\t\t\treturn config_error_nonbool(k);\n> -\t\talias_val = xstrdup(v);\n> -\t\treturn 0;\n> -\t}\n> +\tif (!prefixcmp(k, \"alias.\") && !strcmp(k+6, alias_key))\n> +\t\treturn git_config_string((const char**)&alias_val, k, v);\n\nAre you sure this ugly cast to \"const char**\" is needed ?\nIsn't there a better way to do it ?\n\n>  \treturn 0;\n>  }\n\nThanks,\nChristian.\n"},{"id":"73705","messageId":"20080405173956.GD14565@leksak.fem-net","threadId":"12992","inReplyTo":"200804051819.46904.chriscool@tuxfamily.org","subject":"Re: [PATCH] alias.c: use git_config_string() to get alias_val","fromName":"Stephan Beyer","fromEmail":"s-beyer@gmx.net","sentAt":"2008-04-05T17:39:56Z","receivedAt":"2008-04-05T17:39:56Z","isPatch":true,"sender":{"key":"s-beyer@gmx.net","avatar":"https://avatars.githubusercontent.com/u/143889?v=4"},"body":"Hi,\n\n> Did you see:\n> \n> \"(And no, casting the \"char **\" into a \"const char **\" is not a good \n> solution either.)\"\n> \n> in the above page ?\n\nYes.\n\n> > +\tif (!prefixcmp(k, \"alias.\") && !strcmp(k+6, alias_key))\n> > +\t\treturn git_config_string((const char**)&alias_val, k, v);\n> \n> Are you sure this ugly cast to \"const char**\" is needed ?\n> Isn't there a better way to do it ?\n\nWell, because alias_val is not a constant[1], changing\n\tstatic char *alias_val;\nto\n\tstatic const char *alias_val;\nis not an option.\n\nThe only other way I see[2] atm is a brain-damaged:\n--\nstatic int alias_lookup_cb(const char *k, const char *v)\n{\n\tif (!prefixcmp(k, \"alias.\") && !strcmp(k+6, alias_key)) {\n\t\tconst char *tmp;\n\t\tint ret = git_config_string(&tmp, k, v);\n\t\talias_val = xstrdup(tmp);\n\t\t/* actually, tmp should be free()'d. */\n\t\treturn ret;\n\t}\n\treturn 0;\n}\n--\nBut instead of doing that, the original should be kept, because it is\nbetter in code beauty, performance and memory usage. ;-)\n\nSo I thought the casting is ugly, but it does no harm. I hope ;)\n(Yes, a cast from const char ** to char ** is, indeed, dangerous.)\n\nBut if I miss an obvious point, please tell me :)\n\nRegards,\n Stephan\n\nFootnotes:\n [1] It is no constant because it is returned by alias_lookup(),\n     and thus could be changed by further instructions.\n [2] ...between the many ways that result in compiler warnings ;-)\n\n-- \nStephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F\n"},{"id":"73735","messageId":"200804060749.34109.chriscool@tuxfamily.org","threadId":"12992","inReplyTo":"20080405173956.GD14565@leksak.fem-net","subject":"Re: [PATCH] alias.c: use git_config_string() to get alias_val","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-04-06T05:49:33Z","receivedAt":"2008-04-06T05:49:33Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nLe samedi 5 avril 2008, Stephan Beyer a écrit :\n> Hi,\n>\n> > Did you see:\n> >\n> > \"(And no, casting the \"char **\" into a \"const char **\" is not a good\n> > solution either.)\"\n> >\n> > in the above page ?\n>\n> Yes.\n\nSo you should say in the commit message that you decided to cast to \"const \nchar **\" despite what is on the Janitor page, and most importantly explain \nwhy in the commit message.\n\n> > > +\tif (!prefixcmp(k, \"alias.\") && !strcmp(k+6, alias_key))\n> > > +\t\treturn git_config_string((const char**)&alias_val, k, v);\n> >\n> > Are you sure this ugly cast to \"const char**\" is needed ?\n> > Isn't there a better way to do it ?\n>\n> Well, because alias_val is not a constant[1], changing\n> \tstatic char *alias_val;\n> to\n> \tstatic const char *alias_val;\n> is not an option.\n>\n> The only other way I see[2] atm is a brain-damaged:\n> --\n> static int alias_lookup_cb(const char *k, const char *v)\n> {\n> \tif (!prefixcmp(k, \"alias.\") && !strcmp(k+6, alias_key)) {\n> \t\tconst char *tmp;\n> \t\tint ret = git_config_string(&tmp, k, v);\n> \t\talias_val = xstrdup(tmp);\n> \t\t/* actually, tmp should be free()'d. */\n> \t\treturn ret;\n> \t}\n> \treturn 0;\n> }\n> --\n> But instead of doing that, the original should be kept, because it is\n> better in code beauty, performance and memory usage. ;-)\n\nYes, so perhaps it's not a good idea to convert the original file to \ngit_config_string.\n\n> So I thought the casting is ugly, but it does no harm. I hope ;)\n> (Yes, a cast from const char ** to char ** is, indeed, dangerous.)\n>\n> But if I miss an obvious point, please tell me :)\n>\n> Regards,\n>  Stephan\n>\n> Footnotes:\n>  [1] It is no constant because it is returned by alias_lookup(),\n>      and thus could be changed by further instructions.\n\nYes, but there are only 2 callers and only one in git.c changes the buffer.\n\nA patch like this (not tested) one makes use of a strbuf to copy the buffer \nreturned by alias_lookup in git.c, so that it is now possible (if we really \nwant it) to change alias_lookup to return a \"const char *\" instead of \na \"char *\":\n\n----8<----\n\ndiff --git a/git.c b/git.c\nindex c4e4644..ba5593f 100644\n--- a/git.c\n+++ b/git.c\n@@ -147,34 +147,29 @@ static int handle_alias(int *argcp, const char \n***argv)\n \tint count, option_count;\n \tconst char** new_argv;\n \tconst char *alias_command;\n-\tchar *alias_string;\n+\tstruct strbuf alias_buf = STRBUF_INIT;\n+\tchar *cmdline;\n \tint unused_nongit;\n \n \tsubdir = setup_git_directory_gently(&unused_nongit);\n \n \talias_command = (*argv)[0];\n-\talias_string = alias_lookup(alias_command);\n-\tif (alias_string) {\n-\t\tif (alias_string[0] == '!') {\n-\t\t\tif (*argcp > 1) {\n-\t\t\t\tstruct strbuf buf;\n-\n-\t\t\t\tstrbuf_init(&buf, PATH_MAX);\n-\t\t\t\tstrbuf_addstr(&buf, alias_string);\n-\t\t\t\tsq_quote_argv(&buf, (*argv) + 1, PATH_MAX);\n-\t\t\t\tfree(alias_string);\n-\t\t\t\talias_string = buf.buf;\n-\t\t\t}\n+\tstrbuf_addstr(&alias_buf, alias_lookup(alias_command));\n+\tif (alias_buf.len) {\n+\t\tif (alias_buf.buf[0] == '!') {\n+\t\t\tif (*argcp > 1)\n+\t\t\t\tsq_quote_argv(&alias_buf, (*argv) + 1, PATH_MAX);\n \t\t\ttrace_printf(\"trace: alias to shell cmd: %s => %s\\n\",\n-\t\t\t\t     alias_command, alias_string + 1);\n-\t\t\tret = system(alias_string + 1);\n+\t\t\t\t     alias_command, alias_buf.buf + 1);\n+\t\t\tret = system(alias_buf.buf + 1);\n \t\t\tif (ret >= 0 && WIFEXITED(ret) &&\n \t\t\t    WEXITSTATUS(ret) != 127)\n \t\t\t\texit(WEXITSTATUS(ret));\n \t\t\tdie(\"Failed to run '%s' when expanding alias '%s'\\n\",\n-\t\t\t    alias_string + 1, alias_command);\n+\t\t\t    alias_buf.buf + 1, alias_command);\n \t\t}\n-\t\tcount = split_cmdline(alias_string, &new_argv);\n+\t\tcmdline = strbuf_detach(&alias_buf, NULL);\n+\t\tcount = split_cmdline(cmdline, &new_argv);\n \t\toption_count = handle_options(&new_argv, &count, &envchanged);\n \t\tif (envchanged)\n \t\t\tdie(\"alias '%s' changes environment variables\\n\"\n\n----8<----\n\nBut I don't think it's worth the trouble.\n\nThanks,\nChristian.\n"},{"id":"73738","messageId":"20080406110232.GA6965@leksak.fem-net","threadId":"12992","inReplyTo":"200804060749.34109.chriscool@tuxfamily.org","subject":"Re: [PATCH] alias.c: use git_config_string() to get alias_val","fromName":"Stephan Beyer","fromEmail":"s-beyer@gmx.net","sentAt":"2008-04-06T11:02:32Z","receivedAt":"2008-04-06T11:02:32Z","isPatch":true,"sender":{"key":"s-beyer@gmx.net","avatar":"https://avatars.githubusercontent.com/u/143889?v=4"},"body":"Hi,\n\n> So you should say in the commit message that you decided to cast to \"const \n> char **\" despite what is on the Janitor page, and most importantly explain \n> why in the commit message.\n\nAh, ok.\nDidn't knew that such reasoning goes into the commit message,\nbut, yes, it makes sense.\n\n> > But instead of doing that, the original should be kept, because it is\n> > better in code beauty, performance and memory usage. ;-)\n> \n> Yes, so perhaps it's not a good idea to convert the original file to \n> git_config_string.\n\nOr to use the cast. ;-)\n\n[...]\n> But I don't think it's worth the trouble.\n\nI agree ;-)\n\nSo what to do?\nKeep it?\n\n\n\nAnd there are still some easy `Janitor tasks', like in builtin-apply.c:\n--\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex b5f78ac..ce0a0c3 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -2978,12 +2978,8 @@ static int apply_patch(int fd, const char *filename, int inaccurate_eof)\n \n static int git_apply_config(const char *var, const char *value)\n {\n-\tif (!strcmp(var, \"apply.whitespace\")) {\n-\t\tif (!value)\n-\t\t\treturn config_error_nonbool(var);\n-\t\tapply_default_whitespace = xstrdup(value);\n-\t\treturn 0;\n-\t}\n+\tif (!strcmp(var, \"apply.whitespace\"))\n+\t\treturn git_config_string(&apply_default_whitespace, var, value);\n \treturn git_default_config(var, value);\n }\n \n--\n(Test case: git-apply a patch with trailing whitespaces and check with\napply.whitespace = nowarn / fix / error)\n\nI have attached the patch because I do not want to drop another \nmail for it.\nAnd now there is nothing to argue about, I hope, because it is as simple as\nhttp://git.kernel.org/?p=git/git.git;a=commitdiff;h=9886ea417b7da9722c95630b5980ac174e04c71c\n\n\nRegards,\n Stephan\n\n-- \nStephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F\n\n\n>From 78d1ac0fd19b274853d3c8439d45922c1a0fe82d Mon Sep 17 00:00:00 2001\nFrom: Stephan Beyer <s-beyer@gmx.net>\nDate: Sun, 6 Apr 2008 12:37:31 +0200\nSubject: [PATCH] builtin-apply.c: use git_config_string() to get apply_default_whitespace\n\nSigned-off-by: Stephan Beyer <s-beyer@gmx.net>\n---\n builtin-apply.c |    8 ++------\n 1 files changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex b5f78ac..ce0a0c3 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -2978,12 +2978,8 @@ static int apply_patch(int fd, const char *filename, int inaccurate_eof)\n \n static int git_apply_config(const char *var, const char *value)\n {\n-\tif (!strcmp(var, \"apply.whitespace\")) {\n-\t\tif (!value)\n-\t\t\treturn config_error_nonbool(var);\n-\t\tapply_default_whitespace = xstrdup(value);\n-\t\treturn 0;\n-\t}\n+\tif (!strcmp(var, \"apply.whitespace\"))\n+\t\treturn git_config_string(&apply_default_whitespace, var, value);\n \treturn git_default_config(var, value);\n }\n \n-- \n1.5.5.rc3.8.g78d1a\n\n"},{"id":"73739","messageId":"200804061529.24083.chriscool@tuxfamily.org","threadId":"12992","inReplyTo":"20080406110232.GA6965@leksak.fem-net","subject":"Re: [PATCH] alias.c: use git_config_string() to get alias_val","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-04-06T13:29:23Z","receivedAt":"2008-04-06T13:29:23Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le dimanche 6 avril 2008, Stephan Beyer a écrit :\n> Hi,\n>\n> > So you should say in the commit message that you decided to cast to\n> > \"const char **\" despite what is on the Janitor page, and most\n> > importantly explain why in the commit message.\n>\n> Ah, ok.\n> Didn't knew that such reasoning goes into the commit message,\n> but, yes, it makes sense.\n>\n> > > But instead of doing that, the original should be kept, because it is\n> > > better in code beauty, performance and memory usage. ;-)\n> >\n> > Yes, so perhaps it's not a good idea to convert the original file to\n> > git_config_string.\n>\n> Or to use the cast. ;-)\n>\n> [...]\n>\n> > But I don't think it's worth the trouble.\n>\n> I agree ;-)\n>\n> So what to do?\n> Keep it?\n\nNo, I think its better to do nothing on \"alias.c\".\n\n> And there are still some easy `Janitor tasks', like in builtin-apply.c:\n\nYes. And thanks, your patch looks good, but please send a proper patch for \nit latter if you really care about it.\n\n(By proper I mean in its own email and not attached. And I know my patch \nincluded in my previous email was bad (corrupted tab, sorry about that,  \nand not in its own email), but it was only for discussion, yours too \ngranted.)\n\nNow I think that you should focus on your GSoC application as you don't have \nmuch time left to improve it and I just asked you for more information.\n\nThanks,\nChristian.\n"}]}