{"thread":{"id":"36923","subject":"[PATCH/RFC] alias.c: Replace git_config with git_config_get_string","startedAt":"2014-06-16T09:15:54Z","lastAt":"2014-06-17T21:14:23Z","messageCount":4,"participants":["Tanay Abhra","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"244242","messageId":"1402910154-417-1-git-send-email-tanayabh@gmail.com","threadId":"36923","inReplyTo":null,"subject":"[PATCH/RFC] alias.c: Replace git_config with git_config_get_string","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-16T09:15:54Z","receivedAt":"2014-06-16T09:15:54Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Original implementation uses a callback based approach which has some\ndeficiencies like a convoluted control flow and redundant variables.\nUse git_config_get_string instead of git_config to take advantage of\nthe config hash-table.\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n\n**DOUBT**\nThis patch builds on top of patch series[1]. The first patch in the \nreplace `git_config` series is [2], which passed all the tests.\n\nBut this patch falters at this test in t1300-repo-config.sh,\n\ngit config alias.checkconfig \"-c foo.check=bar config foo.check\" &&\n\t\techo bar >expect &&\n\t\tgit checkconfig >actual &&\n\t\ttest_cmp expect actual\n\nI hand tested this case and the outputs match. But I don't know why the tests\nare failing.\n\nIn t1300-repo-config this test also fails,\n\n#\t\tgit config alias.split-cmdline-fix 'echo \"' &&\n#\t\ttest_must_fail git split-cmdline-fix &&\n#\t\techo foo > foo &&\n#\t\tgit add foo &&\n#\t\tgit commit -m 'initial commit' &&\n#\t\tgit config branch.master.mergeoptions 'echo \"' &&\n#\t\ttest_must_fail git merge master\n\nCan anybody help me with this one?\nThanks.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/251704\n[2] http://thread.gmane.org/gmane.comp.version-control.git/251707\n\nCheers,\nTanay Abhra.\n\n alias.c | 27 ++++++++++-----------------\n 1 file changed, 10 insertions(+), 17 deletions(-)\n\ndiff --git a/alias.c b/alias.c\nindex 5efc3d6..fffad73 100644\n--- a/alias.c\n+++ b/alias.c\n@@ -1,24 +1,17 @@\n #include \"cache.h\"\n \n-static const char *alias_key;\n-static char *alias_val;\n-\n-static int alias_lookup_cb(const char *k, const char *v, void *cb)\n-{\n-\tif (starts_with(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-\treturn 0;\n-}\n-\n char *alias_lookup(const char *alias)\n {\n-\talias_key = alias;\n-\talias_val = NULL;\n-\tgit_config(alias_lookup_cb, NULL);\n+\tchar *alias_key, *alias_val;\n+\tconst char *v;\n+\talias_key = xmalloc(7+strlen(alias));\n+\tstrcpy(alias_key, \"alias.\");\n+\tstrcat(alias_key, alias);\n+\tv = git_config_get_string(alias_key);\n+\tif (!v)\n+\t\tconfig_error_nonbool(alias_key);\n+\talias_val = xstrdup(v);\n+\tfree(alias_key);\n \treturn alias_val;\n }\n \n-- \n1.9.0.GIT\n"},{"id":"244377","messageId":"20140617054357.GC29957@sigill.intra.peff.net","threadId":"36923","inReplyTo":"1402910154-417-1-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH/RFC] alias.c: Replace git_config with git_config_get_string","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-17T05:43:57Z","receivedAt":"2014-06-17T05:43:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 16, 2014 at 02:15:54AM -0700, Tanay Abhra wrote:\n\n> **DOUBT**\n> This patch builds on top of patch series[1]. The first patch in the \n> replace `git_config` series is [2], which passed all the tests.\n> \n> But this patch falters at this test in t1300-repo-config.sh,\n> \n> git config alias.checkconfig \"-c foo.check=bar config foo.check\" &&\n> \t\techo bar >expect &&\n> \t\tgit checkconfig >actual &&\n> \t\ttest_cmp expect actual\n> \n> I hand tested this case and the outputs match. But I don't know why the tests\n> are failing.\n\nI get:\n\n    expecting success: \n            git config alias.split-cmdline-fix 'echo \"' &&\n            test_must_fail git split-cmdline-fix &&\n            echo foo > foo &&\n            git add foo &&\n            git commit -m 'initial commit' &&\n            git config branch.master.mergeoptions 'echo \"' &&\n            test_must_fail git merge master\n    \n    Segmentation fault\n    test_must_fail: died by signal: git split-cmdline-fix\n\nRunning with valgrind gives more details (it looks like the segfault I\nmentioned in the other thread).\n\n>  char *alias_lookup(const char *alias)\n>  {\n> -\talias_key = alias;\n> -\talias_val = NULL;\n> -\tgit_config(alias_lookup_cb, NULL);\n> +\tchar *alias_key, *alias_val;\n> +\tconst char *v;\n> +\talias_key = xmalloc(7+strlen(alias));\n> +\tstrcpy(alias_key, \"alias.\");\n> +\tstrcat(alias_key, alias);\n\nPlease use a strbuf instead of hand-rolling the math. It's much easier\nto verify that it is correct, and it avoids badly designed functions\nlike strcat. I.e.:\n\n  struct strbuf key = STRBUF_INIT;\n  strbuf_addf(&key, \"alias.%s\", alias);\n  ...\n  strbuf_release(&key);\n\n(note also that since the key/val variables are no longer static\n globals, it's fine to use a shorter, less clunky name).\n\n> +\tv = git_config_get_string(alias_key);\n> +\tif (!v)\n> +\t\tconfig_error_nonbool(alias_key);\n\nWhat does a NULL output from git_config_get_string mean? I think with\nthe current code, it means \"no such key was found\".  In which case, you\nshould be returning NULL here (there is no such alias), not complaining\nwith config_error_nonbool.\n\nAgain, this is going to depend on your strategy for storing booleans\nthat I mentioned elsewhere.\n\n-Peff\n"},{"id":"244467","messageId":"53A08E37.1040104@gmail.com","threadId":"36923","inReplyTo":"20140617054357.GC29957@sigill.intra.peff.net","subject":"Re: [PATCH/RFC] alias.c: Replace git_config with git_config_get_string","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-17T18:51:35Z","receivedAt":"2014-06-17T18:51:35Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"On 06/16/2014 10:43 PM, Jeff King wrote:\n\n> \n>> +\tv = git_config_get_string(alias_key);\n>> +\tif (!v)\n>> +\t\tconfig_error_nonbool(alias_key);\n> \n> What does a NULL output from git_config_get_string mean? I think with\n> the current code, it means \"no such key was found\".  In which case, you\n> should be returning NULL here (there is no such alias), not complaining\n> with config_error_nonbool.\n> \n\nYes, you surmised correctly. I totally skipped the fact that git_config() can\nreturn null for values indicating a boolean value. I will correct it in the next\npatch.\n\n> Again, this is going to depend on your strategy for storing booleans\n> that I mentioned elsewhere.\n\nI have read your other two replies related to it. I suggest the following approach\nfor git_config_get_string(), it will return,\n\n1. Return null if no value was found for the entered key.\n\n2. Empty string (\"\"), returned for NULL values denoting boolean true in some cases.\n   I think it would be much better than converting NULL to \"true\" or something else\n   internally in the function.\n   We can easily handle such cases as the above with a strcmp like,\n\n+\tv = git_config_get_string(alias_key);\n+\tif (!strcmp(v, \"\"))\n+\t\tconfig_error_nonbool(alias_key);\n\nWhat do you think about this approach?\n\nThanks for the suggestion, I was pulling my hair out due this bug for last two days.\n\nCheers,\nTanay Abhra.\n"},{"id":"244484","messageId":"20140617211423.GB13848@sigill.intra.peff.net","threadId":"36923","inReplyTo":"53A08E37.1040104@gmail.com","subject":"Re: [PATCH/RFC] alias.c: Replace git_config with git_config_get_string","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-17T21:14:23Z","receivedAt":"2014-06-17T21:14:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 17, 2014 at 11:51:35AM -0700, Tanay Abhra wrote:\n\n> I have read your other two replies related to it. I suggest the following approach\n> for git_config_get_string(), it will return,\n> \n> 1. Return null if no value was found for the entered key.\n> \n> 2. Empty string (\"\"), returned for NULL values denoting boolean true in some cases.\n>    I think it would be much better than converting NULL to \"true\" or something else\n>    internally in the function.\n>    We can easily handle such cases as the above with a strcmp like,\n> \n> +\tv = git_config_get_string(alias_key);\n> +\tif (!strcmp(v, \"\"))\n> +\t\tconfig_error_nonbool(alias_key);\n> \n> What do you think about this approach?\n\nHmm. Then we can't distinguish between:\n\n  [alias]\n  foo\n\nand\n\n  [alias]\n  foo =\n\nI cannot think of a good reason to do the latter with aliases, but I\nwonder if there are other config values for which \"the empty string\" is\na useful value.\n\nI think I'd lean towards an interface which can actually express the\ndifference between \"key not found\" (1), \"key present but no value\" (2), and \"key\npresent with some string\" (3).\n\nYou could express (2) with a special pointer value, like:\n\n  v = git_config_get_string(key);\n  if (!v)\n\treturn NULL; /* no key */\n  else if (v == CONFIG_EMPTY_VALUE) {\n\tconfig_error_nonbool(key);\n\treturn NULL;\n  } else\n\treturn xstrdup(v); /* actual value */\n\nIt's reasonably concise, but it's a little ugly that if you dereference\nCONFIG_EMPTY_VALUE, you get random memory (on the other hand, we could\npoint it at 0x1, which would make it no different than NULL; you'd get a\nsegfault in either case).\n\nAnother option is to indicate (1) with a return value, and use a\nseparate parameter for the value. Like:\n\n  if (git_config_get_string(key, &v) < 0)\n\treturn NULL; /* no key */\n  else if (!v) {\n\tconfig_error_nonbool(key); /* no value */\n\treturn NULL;\n  } else\n\treturn xstrdup(v); /* actual value */\n\n> Thanks for the suggestion, I was pulling my hair out due this bug for last two days.\n\nYou're welcome. This is exactly why we encourage students to get their\npatches onto the list early for review. There is a lot of git expertise\navailable on the list. :)\n\n-Peff\n"}]}