{"thread":{"id":"36922","subject":"[PATCH/RFC] branch.c: Replace `git_config` with `git_config_get_string`","startedAt":"2014-06-16T08:52:30Z","lastAt":"2014-06-16T17:49:19Z","messageCount":3,"participants":["Tanay Abhra","Matthieu Moy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"244241","messageId":"1402908750-24851-1-git-send-email-tanayabh@gmail.com","threadId":"36922","inReplyTo":null,"subject":"[PATCH/RFC] branch.c: Replace `git_config` with `git_config_get_string`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-16T08:52:30Z","receivedAt":"2014-06-16T08:52:30Z","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\n---\n\nThis patch builds on top of patch series[1]. It passes all the tests, and\nthis in first of series of patches that aim to replace all git_config\ncalls scattered arund the code base with appropriate non-callback based\ncalls.\n\nThere are total 111 calls in total in all of git codebase. How should I send\nthe patches, alphabetically or otherwise?\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/251704\n\nCheers,\nTanay Abhra.\n\n branch.c | 25 +++++++++----------------\n 1 file changed, 9 insertions(+), 16 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 660097b..257b1bf 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -140,33 +140,26 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,\n \treturn 0;\n }\n \n-struct branch_desc_cb {\n+struct branch_desc {\n \tconst char *config_name;\n \tconst char *value;\n };\n \n-static int read_branch_desc_cb(const char *var, const char *value, void *cb)\n-{\n-\tstruct branch_desc_cb *desc = cb;\n-\tif (strcmp(desc->config_name, var))\n-\t\treturn 0;\n-\tfree((char *)desc->value);\n-\treturn git_config_string(&desc->value, var, value);\n-}\n-\n int read_branch_desc(struct strbuf *buf, const char *branch_name)\n {\n-\tstruct branch_desc_cb cb;\n+\tconst char *value;\n+\tstruct branch_desc desc;\n \tstruct strbuf name = STRBUF_INIT;\n \tstrbuf_addf(&name, \"branch.%s.description\", branch_name);\n-\tcb.config_name = name.buf;\n-\tcb.value = NULL;\n-\tif (git_config(read_branch_desc_cb, &cb) < 0) {\n+\tdesc.config_name = name.buf;\n+\tdesc.value = NULL;\n+\tvalue = git_config_get_string(desc.config_name);\n+\tgit_config_string(&desc.value, desc.config_name, value);\n+\tif (!desc.value) {\n \t\tstrbuf_release(&name);\n \t\treturn -1;\n \t}\n-\tif (cb.value)\n-\t\tstrbuf_addstr(buf, cb.value);\n+\tstrbuf_addstr(buf, desc.value);\n \tstrbuf_release(&name);\n \treturn 0;\n }\n-- \n1.9.0.GIT\n"},{"id":"244270","messageId":"vpqfvj4226o.fsf@anie.imag.fr","threadId":"36922","inReplyTo":"1402908750-24851-1-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH/RFC] branch.c: Replace `git_config` with `git_config_get_string`","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-06-16T17:29:19Z","receivedAt":"2014-06-16T17:29:19Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"> Subject: Re: [PATCH/RFC] branch.c: Replace `git_config` with `git_config_get_string`\n\nHere and elsewhere: usually, no capital after :.\n\nTanay Abhra <tanayabh@gmail.com> writes:\n\n> Original implementation uses a callback based approach which has some\n> deficiencies like a convoluted control flow and redundant variables.\n\n\"deficiencies\" might be a bit strong (the code did work).\n\n> There are total 111 calls in total in all of git codebase. How should I send\n> the patches, alphabetically or otherwise?\n\nMy advice would be: try as much as possible to split according to the\ncomplexity of the patch.\n\nAs a reviewer, I find it rather easy to review a large number of trivial\nand similar changes, but I hate having to switch back to \"wow, the\nauthor did something tricky, let's try to understand this\" in the middle\nof a trivial series.\n\n(we had this discussion about `...` Vs $(...) and test -a Vs test ... &&\nseries, which were essentially very trivial changes, but with subtle\nbugs introduced and hidden by the volume of trivial changes).\n\n>  branch.c | 25 +++++++++----------------\n>  1 file changed, 9 insertions(+), 16 deletions(-)\n\nRemoving more lines than it adds. I like the patch already ;-).\n\n> diff --git a/branch.c b/branch.c\n> index 660097b..257b1bf 100644\n> --- a/branch.c\n> +++ b/branch.c\n[...]\n>  int read_branch_desc(struct strbuf *buf, const char *branch_name)\n>  {\n> -\tstruct branch_desc_cb cb;\n> +\tconst char *value;\n> +\tstruct branch_desc desc;\n>  \tstruct strbuf name = STRBUF_INIT;\n>  \tstrbuf_addf(&name, \"branch.%s.description\", branch_name);\n> -\tcb.config_name = name.buf;\n> -\tcb.value = NULL;\n> -\tif (git_config(read_branch_desc_cb, &cb) < 0) {\n> +\tdesc.config_name = name.buf;\n> +\tdesc.value = NULL;\n> +\tvalue = git_config_get_string(desc.config_name);\n> +\tgit_config_string(&desc.value, desc.config_name, value);\n\nYou're ignoring the return value of git_config_string, which is an error\ncode. It shouldn't harm, because the code is non-zero iff desc.value is\nset to non-NULL, but you may want to write the code as\n\nif (git_config_string(...)) {\n\tstrbuf_release(...);\n\treturn -1;\n}\n\nIn any case, the patch sounds good to me.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"244273","messageId":"539F2E1F.7010604@gmail.com","threadId":"36922","inReplyTo":"vpqfvj4226o.fsf@anie.imag.fr","subject":"Re: [PATCH/RFC] branch.c: Replace `git_config` with `git_config_get_string`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-16T17:49:19Z","receivedAt":"2014-06-16T17:49:19Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"On 06/16/2014 10:29 AM, Matthieu Moy wrote:\n>> Subject: Re: [PATCH/RFC] branch.c: Replace `git_config` with `git_config_get_string`\n> \n> Here and elsewhere: usually, no capital after :.\n> \n\nNoted.\n\n> Tanay Abhra <tanayabh@gmail.com> writes:\n> \n>> Original implementation uses a callback based approach which has some\n>> deficiencies like a convoluted control flow and redundant variables.\n> \n> \"deficiencies\" might be a bit strong (the code did work).\n> \n\nHehe, I did spell out what the \"deficiencies\" were, nevertheless will revise it in\nnext iteration.\n\n>> There are total 111 calls in total in all of git codebase. How should I send\n>> the patches, alphabetically or otherwise?\n> \n> My advice would be: try as much as possible to split according to the\n> complexity of the patch.\n> \n> As a reviewer, I find it rather easy to review a large number of trivial\n> and similar changes, but I hate having to switch back to \"wow, the\n> author did something tricky, let's try to understand this\" in the middle\n> of a trivial series.\n> \n> (we had this discussion about `...` Vs $(...) and test -a Vs test ... &&\n> series, which were essentially very trivial changes, but with subtle\n> bugs introduced and hidden by the volume of trivial changes).\n> \n\nNoted.\n\n>>  branch.c | 25 +++++++++----------------\n>>  1 file changed, 9 insertions(+), 16 deletions(-)\n> \n> Removing more lines than it adds. I like the patch already ;-).\n> \n>> diff --git a/branch.c b/branch.c\n>> index 660097b..257b1bf 100644\n>> --- a/branch.c\n>> +++ b/branch.c\n> [...]\n>>  int read_branch_desc(struct strbuf *buf, const char *branch_name)\n>>  {\n>> -\tstruct branch_desc_cb cb;\n>> +\tconst char *value;\n>> +\tstruct branch_desc desc;\n>>  \tstruct strbuf name = STRBUF_INIT;\n>>  \tstrbuf_addf(&name, \"branch.%s.description\", branch_name);\n>> -\tcb.config_name = name.buf;\n>> -\tcb.value = NULL;\n>> -\tif (git_config(read_branch_desc_cb, &cb) < 0) {\n>> +\tdesc.config_name = name.buf;\n>> +\tdesc.value = NULL;\n>> +\tvalue = git_config_get_string(desc.config_name);\n>> +\tgit_config_string(&desc.value, desc.config_name, value);\n> \n> You're ignoring the return value of git_config_string, which is an error\n> code. It shouldn't harm, because the code is non-zero iff desc.value is\n> set to non-NULL, but you may want to write the code as\n> \n> if (git_config_string(...)) {\n> \tstrbuf_release(...);\n> \treturn -1;\n> }\n> \n> In any case, the patch sounds good to me.\n> \n\nYes, for clarity sake, will rewrite the section like that.\nThanks for the review.\n"}]}