{"thread":{"id":"36189","subject":"[PATCH] branch.c: simplify chain of if statements","startedAt":"2014-03-16T21:22:42Z","lastAt":"2014-03-18T21:36:33Z","messageCount":7,"participants":["Dragos Foianu","Matthieu Moy","Eric Sunshine","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"236842","messageId":"1395004962-18200-1-git-send-email-dragos.foianu@gmail.com","threadId":"36189","inReplyTo":null,"subject":"[PATCH] branch.c: simplify chain of if statements","fromName":"Dragos Foianu","fromEmail":"dragos.foianu@gmail.com","sentAt":"2014-03-16T21:22:42Z","receivedAt":"2014-03-16T21:22:42Z","isPatch":true,"sender":{"key":"dragos.foianu@gmail.com","avatar":null},"body":"This patch uses a table-driven approach in order to make the code\ncleaner. Although not necessary, it helps code reability by not\nforcing the user to read the print message when trying to\nunderstand what the code does. The rebase check has been moved to\nthe verbose if statement to avoid making the same check in each of\nthe four if statements.\n\nSigned-off-by: Dragos Foianu \n---\n branch.c | 32 ++++++++++++++++----------------\n 1 file changed, 16 insertions(+), 16 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 723a36b..e2fe455 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -54,6 +54,14 @@ void install_branch_config(int flag, const char *local, const char *origin, cons\n \tstruct strbuf key = STRBUF_INIT;\n \tint rebasing = should_setup_rebase(origin);\n \n+\tconst char *verbose_prints[4] = {\n+\t\t\"Branch %s set up to track remote branch %s from %s%s\",\n+\t\t\"Branch %s set up to track local branch %s%s\",\n+\t\t\"Branch %s set up to track remote ref %s%s\",\n+\t\t\"Branch %s set up to track local ref %s%s\"\n+\t};\n+\tchar *verbose_rebasing = rebasing ? \" by rebasing.\" : \".\";\n+\n \tif (remote_is_branch\n \t && !strcmp(local, shortname)\n \t && !origin) {\n@@ -78,25 +86,17 @@ void install_branch_config(int flag, const char *local, const char *origin, cons\n \n \tif (flag & BRANCH_CONFIG_VERBOSE) {\n \t\tif (remote_is_branch && origin)\n-\t\t\tprintf_ln(rebasing ?\n-\t\t\t\t _(\"Branch %s set up to track remote branch %s from %s by rebasing.\") :\n-\t\t\t\t _(\"Branch %s set up to track remote branch %s from %s.\"),\n-\t\t\t\t local, shortname, origin);\n+\t\t\tprintf_ln(_(verbose_prints[0]),\n+\t\t\t\tlocal, shortname, origin, verbose_rebasing);\n \t\telse if (remote_is_branch && !origin)\n-\t\t\tprintf_ln(rebasing ?\n-\t\t\t\t _(\"Branch %s set up to track local branch %s by rebasing.\") :\n-\t\t\t\t _(\"Branch %s set up to track local branch %s.\"),\n-\t\t\t\t local, shortname);\n+\t\t\tprintf_ln(_(verbose_prints[1]),\n+\t\t\t\tlocal, shortname, verbose_rebasing);\n \t\telse if (!remote_is_branch && origin)\n-\t\t\tprintf_ln(rebasing ?\n-\t\t\t\t _(\"Branch %s set up to track remote ref %s by rebasing.\") :\n-\t\t\t\t _(\"Branch %s set up to track remote ref %s.\"),\n-\t\t\t\t local, remote);\n+\t\t\tprintf_ln(_(verbose_prints[2]),\n+\t\t\t\tlocal, remote, verbose_rebasing);\n \t\telse if (!remote_is_branch && !origin)\n-\t\t\tprintf_ln(rebasing ?\n-\t\t\t\t _(\"Branch %s set up to track local ref %s by rebasing.\") :\n-\t\t\t\t _(\"Branch %s set up to track local ref %s.\"),\n-\t\t\t\t local, remote);\n+\t\t\tprintf_ln(_(verbose_prints[3]),\n+\t\t\t\tlocal, remote, verbose_rebasing);\n \t\telse\n \t\t\tdie(\"BUG: impossible combination of %d and %p\",\n \t\t\t remote_is_branch, origin);\n-- \n1.8.3.2\n"},{"id":"236857","messageId":"vpqsiqhz3sz.fsf@anie.imag.fr","threadId":"36189","inReplyTo":"1395004962-18200-1-git-send-email-dragos.foianu@gmail.com","subject":"Re: [PATCH] branch.c: simplify chain of if statements","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-03-17T07:23:40Z","receivedAt":"2014-03-17T07:23:40Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Dragos Foianu writes:\n\n> +\tconst char *verbose_prints[4] = {\n> +\t\t\"Branch %s set up to track remote branch %s from %s%s\",\n> +\t\t\"Branch %s set up to track local branch %s%s\",\n> +\t\t\"Branch %s set up to track remote ref %s%s\",\n> +\t\t\"Branch %s set up to track local ref %s%s\"\n> +\t};\n> +\tchar *verbose_rebasing = rebasing ? \" by rebasing.\" : \".\";\n> +\n\nThis seems to be a \"lego construct\" that makes translation harder: are\nyou sure that the \"by rebasing\" will be at the end of the sentence in\nany languages?\n\nAlso, this lacks the _() on verbose_rebasing, which isn't translatable\nanymore after your patch.\n\nI personnally think that the table-driven approach is wrong here, it\nmakes the code shorter but much harder to read.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"236862","messageId":"CAPig+cTff4gDuUieqDkOxbFtk2vLX+fMgCFA_7ZHTMYK2rAuQA@mail.gmail.com","threadId":"36189","inReplyTo":"vpqsiqhz3sz.fsf@anie.imag.fr","subject":"Re: [PATCH] branch.c: simplify chain of if statements","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-17T08:08:41Z","receivedAt":"2014-03-17T08:08:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 17, 2014 at 3:23 AM, Matthieu Moy\n wrote:\n> Dragos Foianu writes:\n>\n>> + const char *verbose_prints[4] = {\n>> + \"Branch %s set up to track remote branch %s from %s%s\",\n>> + \"Branch %s set up to track local branch %s%s\",\n>> + \"Branch %s set up to track remote ref %s%s\",\n>> + \"Branch %s set up to track local ref %s%s\"\n>> + };\n>> + char *verbose_rebasing = rebasing ? \" by rebasing.\" : \".\";\n>> +\n>\n> This seems to be a \"lego construct\" that makes translation harder: are\n> you sure that the \"by rebasing\" will be at the end of the sentence in\n> any languages?\n\nRead this thread [1] for more details about why this approach is problematic.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/243793/focus=243824\n\n> Also, this lacks the _() on verbose_rebasing, which isn't translatable\n> anymore after your patch.\n>\n> I personnally think that the table-driven approach is wrong here, it\n> makes the code shorter but much harder to read.\n>\n> --\n> Matthieu Moy\n> http://www-verimag.imag.fr/~moy/\n"},{"id":"236863","messageId":"CAPig+cTodcfSVmHZeHuAj2kuE_CxuZqZuaNHv33hrhDmQuSmuA@mail.gmail.com","threadId":"36189","inReplyTo":"1395004962-18200-1-git-send-email-dragos.foianu@gmail.com","subject":"Re: [PATCH] branch.c: simplify chain of if statements","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-17T08:46:55Z","receivedAt":"2014-03-17T08:46:55Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Thanks for the submission. Comments below to give you a taste of the\nGit review process...\n\nOn Sun, Mar 16, 2014 at 5:22 PM, Dragos Foianu wrote:\n> This patch uses a table-driven approach in order to make the code\n> cleaner.\n\nIn fact, this change is not table-driven (emphasis on *driven*). It\nmerely moves the strings into a table, but all the logic is still in\nthe code. To be table-driven, the logic would be encoded in the table\nas well, and that logic would *drive* the code.\n\nThis is not to say that the code must be table-driven. The GSoC\nmicroproject merely asks if doing so would make sense. Whether it\nwould is for you to decide, and then to explain to reviewers the\nreason(s) why you did or did not make it so.\n\n> Although not necessary, it helps code reability by not\n> forcing the user to read the print message when trying to\n> understand what the code does.\n\nThe if-chain is just sufficiently complex that the print messages may\nactually help the reader understand the logic of the code, so this\nargument seems specious.\n\n> The rebase check has been moved to\n> the verbose if statement to avoid making the same check in each of\n> the four if statements.\n>\n> Signed-off-by: Dragos Foianu \n> ---\n\nOverall, the patch appears to be properly constructed and you seem to\nhave digested Documentation/SubmittingPatches. Good.\n\n> branch.c | 32 ++++++++++++++++----------------\n> 1 file changed, 16 insertions(+), 16 deletions(-)\n>\n> diff --git a/branch.c b/branch.c\n> index 723a36b..e2fe455 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -54,6 +54,14 @@ void install_branch_config(int flag, const char *local, const char *origin, cons\n> struct strbuf key = STRBUF_INIT;\n> int rebasing = should_setup_rebase(origin);\n>\n> + const char *verbose_prints[4] = {\n\nNo need to hard-code 4. 'const char *verbose_prints[]' is sufficient.\n\nOn this project, it is preferred (though not consistent) to name\narrays using singular form, such as verbose_print[], so that accessing\na single element, such as verbose_print[42], reads more grammatically\ncorrect.\n\nverbose_prints[] is not as descriptive as it could be. Perhaps\nsomething like verbose_messages[] would be more informative (though\nawfully long; maybe just messages[]).\n\n> + \"Branch %s set up to track remote branch %s from %s%s\",\n\nEven though you are correctly accessing these strings via _() in the\nprintf_ln() invocation, you still need to mark them translatable here\nwith N_(). See section 4.7 [1] of the gettext manual.\n\n[1]: http://www.gnu.org/software/gettext/manual/gettext.html#Special-cases\n\n> + \"Branch %s set up to track local branch %s%s\",\n> + \"Branch %s set up to track remote ref %s%s\",\n> + \"Branch %s set up to track local ref %s%s\"\n> + };\n> + char *verbose_rebasing = rebasing ? \" by rebasing.\" : \".\";\n\nThis should be 'const char *'.\n\nMatthieu already mentioned [2] that this sort of \"lego\" string\nconstruction is not internationalization-friendly. See section 4.3 [3]\nof the gettext manual for details.\n\n[2]: http://thread.gmane.org/gmane.comp.version-control.git/244210/focus=244226\n[3]: http://www.gnu.org/software/gettext/manual/gettext.html#Preparing-Strings\n\n> if (remote_is_branch\n> && !strcmp(local, shortname)\n> && !origin) {\n> @@ -78,25 +86,17 @@ void install_branch_config(int flag, const char *local, const char *origin, cons\n>\n> if (flag & BRANCH_CONFIG_VERBOSE) {\n> if (remote_is_branch && origin)\n> - printf_ln(rebasing ?\n> - _(\"Branch %s set up to track remote branch %s from %s by rebasing.\") :\n> - _(\"Branch %s set up to track remote branch %s from %s.\"),\n> - local, shortname, origin);\n> + printf_ln(_(verbose_prints[0]),\n> + local, shortname, origin, verbose_rebasing);\n> else if (remote_is_branch && !origin)\n> - printf_ln(rebasing ?\n> - _(\"Branch %s set up to track local branch %s by rebasing.\") :\n> - _(\"Branch %s set up to track local branch %s.\"),\n> - local, shortname);\n> + printf_ln(_(verbose_prints[1]),\n> + local, shortname, verbose_rebasing);\n> else if (!remote_is_branch && origin)\n> - printf_ln(rebasing ?\n> - _(\"Branch %s set up to track remote ref %s by rebasing.\") :\n> - _(\"Branch %s set up to track remote ref %s.\"),\n> - local, remote);\n> + printf_ln(_(verbose_prints[2]),\n> + local, remote, verbose_rebasing);\n> else if (!remote_is_branch && !origin)\n> - printf_ln(rebasing ?\n> - _(\"Branch %s set up to track local ref %s by rebasing.\") :\n> - _(\"Branch %s set up to track local ref %s.\"),\n> - local, remote);\n> + printf_ln(_(verbose_prints[3]),\n> + local, remote, verbose_rebasing);\n\nThese hard-coded indexing constants (0, 1, 2, 3) are fragile and\nconvey little meaning to the reader. Try to consider how to compute\nthe index into verbose_prints[] based upon the values of\n'remote_is_branch' and 'origin'. What are the different ways you could\ndo so?\n\n> else\n> die(\"BUG: impossible combination of %d and %p\",\n> remote_is_branch, origin);\n> --\n> 1.8.3.2\n"},{"id":"236876","messageId":"loom.20140317T120153-546@post.gmane.org","threadId":"36189","inReplyTo":"CAPig+cTodcfSVmHZeHuAj2kuE_CxuZqZuaNHv33hrhDmQuSmuA@mail.gmail.com","subject":"Re: [PATCH] branch.c: simplify chain of if statements","fromName":"Dragos Foianu","fromEmail":"dragos.foianu@gmail.com","sentAt":"2014-03-17T11:46:23Z","receivedAt":"2014-03-17T11:46:23Z","isPatch":true,"sender":{"key":"dragos.foianu@gmail.com","avatar":null},"body":"Eric Sunshine sunshineco.com> writes:\n\n> In fact, this change is not table-driven (emphasis on *driven*). It\n> merely moves the strings into a table, but all the logic is still in\n> the code. To be table-driven, the logic would be encoded in the table\n> as well, and that logic would *drive* the code.\n> \n> This is not to say that the code must be table-driven. The GSoC\n> microproject merely asks if doing so would make sense. Whether it\n> would is for you to decide, and then to explain to reviewers the\n> reason(s) why you did or did not make it so.\n>\n> The if-chain is just sufficiently complex that the print messages may\n> actually help the reader understand the logic of the code, so this\n> argument seems specious.\n\nI understand now after reading your review. I will try to fix this in a\nfuture attempt.\n \n> Matthieu already mentioned [2] that this sort of \"lego\" string\n> construction is not internationalization-friendly. See section 4.3 [3]\n> of the gettext manual for details.\n\nI was hoping to get away with using less memory by having only four entries\nin the table. I suppose that is not possible. The rebasing check can still\nbe moved outside of the four if statements and calculate the index\ncorrectly. The strings would then have to be arranged in such a way to make\nthis work.\n\nUsing a multiple-dimension array as suggested in other submissions for this\nparticular microproject would probably be better, but it has already been done.\n\n> These hard-coded indexing constants (0, 1, 2, 3) are fragile and\n> convey little meaning to the reader. Try to consider how to compute\n> the index into verbose_prints[] based upon the values of\n> 'remote_is_branch' and 'origin'. What are the different ways you could\n> do so?\n\nI was going to do something like this: if !remote_is_branch the index goes\nincremented by 2, because the first two entries are of no interest and if\n!origin, the index is incremented by 1. This would correctly compute the\nindex. It should also work with the rebasing check if the four\nrebasing-specific messages are at the end of the table and when rebasing the\nindex is set to start at those messages.\n\nThe reason I did not go with this is because I would still need the four ifs\nin order to keep the bug check part of the code. I might be able to find a\nwork-around for it on the second attempt.\n\nI have seen N_() used in other code but I wasn't sure what its purpose was.\n\nThank you very much for the review.\n"},{"id":"236878","messageId":"CACBZZX6P38BEQ15w1nVh9cM6nMj0dq-HtT1ZJFfZadriXjZReA@mail.gmail.com","threadId":"36189","inReplyTo":"loom.20140317T120153-546@post.gmane.org","subject":"Re: [PATCH] branch.c: simplify chain of if statements","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2014-03-17T12:53:19Z","receivedAt":"2014-03-17T12:53:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, Mar 17, 2014 at 12:46 PM, Dragos Foianu wrote:\n> The reason I did not go with this is because I would still need the four ifs\n> in order to keep the bug check part of the code. I might be able to find a\n> work-around for it on the second attempt.\n>\n> I have seen N_() used in other code but I wasn't sure what its purpose was.\n\nAside from other comments here, more generally if you see code that\nlooks odd it helps to see why it was introduced initially.\n\nIn this case if you'd ran e.g.:\n\n git log --reverse -p -G'Branch %s set up to track remote branch %s\nfrom %s by rebasing' -- branch.c\n\nor otherwise searched for the first occurrence of that odd-looking\ncode you'd have gotten:\n\n commit d53a3503\n Author: Nguyễn Thái Ngọc Duy \n Date: Thu Jun 7 19:05:10 2012 +0700\n\n Remove i18n legos in notifying new branch tracking setup\n\n Signed-off-by: Nguyễn Thái Ngọc Duy \n Signed-off-by: Junio C Hamano \n\nAnd searching for that commit has plenty of context for why that was\ndone: https://www.google.com/search?q=%22Remove%20i18n%20legos%20in%20notifying%20new%20branch%20tracking%20setup%22\n"},{"id":"237013","messageId":"CAPig+cQaxLoJstV8sKvKb6+2JcZ967=f485Vp1938m+KwgvLqQ@mail.gmail.com","threadId":"36189","inReplyTo":"loom.20140317T120153-546@post.gmane.org","subject":"Re: [PATCH] branch.c: simplify chain of if statements","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-18T21:36:33Z","receivedAt":"2014-03-18T21:36:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 17, 2014 at 7:46 AM, Dragos Foianu wrote:\n> Eric Sunshine sunshineco.com> writes:\n>> Matthieu already mentioned [2] that this sort of \"lego\" string\n>> construction is not internationalization-friendly. See section 4.3 [3]\n>> of the gettext manual for details.\n>\n> I was hoping to get away with using less memory by having only four entries\n> in the table. I suppose that is not possible. The rebasing check can still\n> be moved outside of the four if statements and calculate the index\n> correctly. The strings would then have to be arranged in such a way to make\n> this work.\n>\n> Using a multiple-dimension array as suggested in other submissions for this\n> particular microproject would probably be better, but it has already been done.\n\nIf a multi-dimension table is indeed better than other alternatives,\nthen that's a good reason to choose it, even if others have already\nused that approach in their submissions. It's more important that the\ncode is clean and easy to understand and maintain than to be clever.\n\nIf you're really interested in trying an approach not already\nsubmitted by others, take a look at Jonathan's idea [1]. If you play\naround with it and find that it actually does make the code clearer\nand simpler, then perhaps it's worth submitting. If not, then not.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/198882/focus=198902\n\n>> These hard-coded indexing constants (0, 1, 2, 3) are fragile and\n>> convey little meaning to the reader. Try to consider how to compute\n>> the index into verbose_prints[] based upon the values of\n>> 'remote_is_branch' and 'origin'. What are the different ways you could\n>> do so?\n>\n> I was going to do something like this: if !remote_is_branch the index goes\n> incremented by 2, because the first two entries are of no interest and if\n> !origin, the index is incremented by 1. This would correctly compute the\n> index. It should also work with the rebasing check if the four\n> rebasing-specific messages are at the end of the table and when rebasing the\n> index is set to start at those messages.\n>\n> The reason I did not go with this is because I would still need the four ifs\n> in order to keep the bug check part of the code. I might be able to find a\n> work-around for it on the second attempt.\n\nSince the result is just a number, its possible to compute it directly\nwithout conditionals, however, it does start resembling a magical\nincantation. (I'll comment further in your v2 submission.)\n\n> I have seen N_() used in other code but I wasn't sure what its purpose was.\n>\n> Thank you very much for the review.\n"}]}