{"thread":{"id":"36198","subject":"[PATCHv2] branch.c: simplify chain of if statements","startedAt":"2014-03-17T15:51:33Z","lastAt":"2014-03-21T16:50:58Z","messageCount":7,"participants":["Dragos Foianu","Eric Sunshine","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"236885","messageId":"1395071493-31435-1-git-send-email-dragos.foianu@gmail.com","threadId":"36198","inReplyTo":null,"subject":"[PATCHv2] branch.c: simplify chain of if statements","fromName":"Dragos Foianu","fromEmail":"dragos.foianu@gmail.com","sentAt":"2014-03-17T15:51:33Z","receivedAt":"2014-03-17T15:51:33Z","isPatch":false,"sender":{"key":"dragos.foianu@gmail.com","avatar":null},"body":"This patch uses a table to store the different messages that can\nbe emitted by the verbose install_branch_config function. It\ncomputes an index based on the three flags and prints the message\nlocated at the specific index in the table of messages. If the\nindex somehow is not within the table, we have a bug.\n\nSigned-off-by: Dragos Foianu \n---\n branch.c | 44 +++++++++++++++++++++++++-------------------\n 1 file changed, 25 insertions(+), 19 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 723a36b..95645d5 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -54,6 +54,18 @@ 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 *messages[] = {\n+\t\tN_(\"Branch %s set up to track local ref %s.\"),\n+\t\tN_(\"Branch %s set up to track remote ref %s.\"),\n+\t\tN_(\"Branch %s set up to track local branch %s.\"),\n+\t\tN_(\"Branch %s set up to track remote branch %s from %s.\"),\n+\t\tN_(\"Branch %s set up to track local ref %s by rebasing.\"),\n+\t\tN_(\"Branch %s set up to track remote ref %s by rebasing.\"),\n+\t\tN_(\"Branch %s set up to track local branch %s by rebasing.\"),\n+\t\tN_(\"Branch %s set up to track remote branch %s from %s by rebasing.\")\n+\t};\n+\tint index = 0;\n+\n \tif (remote_is_branch\n \t && !strcmp(local, shortname)\n \t && !origin) {\n@@ -76,28 +88,22 @@ void install_branch_config(int flag, const char *local, const char *origin, cons\n \t}\n \tstrbuf_release(&key);\n \n+\tif (origin)\n+\t\tindex += 1;\n+\tif (remote_is_branch)\n+\t\tindex += 2;\n+\tif (rebasing)\n+\t\tindex += 4;\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\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\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\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(_(messages[index]),\n+\t\t\t\tlocal, shortname, origin);\n \t\telse\n+\t\t\tprintf_ln(_(messages[index]),\n+\t\t\t\tlocal, (!remote_is_branch) ? remote : shortname);\n+\n+\t\tif (index < 0 || index > sizeof(messages) / sizeof(*messages))\n \t\t\tdie(\"BUG: impossible combination of %d and %p\",\n \t\t\t remote_is_branch, origin);\n \t}\n-- \n1.8.3.2\n"},{"id":"237018","messageId":"CAPig+cS9QApn1T3-R8n+W+1ee9FbNftsmhrr90SJKs+gqzvC5A@mail.gmail.com","threadId":"36198","inReplyTo":"1395071493-31435-1-git-send-email-dragos.foianu@gmail.com","subject":"Re: [PATCHv2] branch.c: simplify chain of if statements","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-18T22:31:07Z","receivedAt":"2014-03-18T22:31:07Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Thanks for the resubmission. Comments below...\n\nOn Mon, Mar 17, 2014 at 11:51 AM, Dragos Foianu wrote:\n> This patch uses a table to store the different messages that can\n> be emitted by the verbose install_branch_config function. It\n> computes an index based on the three flags and prints the message\n> located at the specific index in the table of messages. If the\n> index somehow is not within the table, we have a bug.\n\nMost of this text can be dropped due to redundancy.\n\nSaying \"This patch...\" is unnecessary.\n\nThe remaining text primarily says in prose what the patch itself\nconveys more concisely and precisely. It's easier to read and\nunderstand the actual code than it is to wade through a lengthy\ndescription of the code change.\n\nSpeak in imperative voice: \"Use a table to store...\"\n\nYou might, for instance, say instead something like this:\n\n install_branch_config() uses a long, somewhat complex if-chain to\n select a message to display in verbose mode. Simplify the logic\n by moving the messages to a table from which they can be\n easily retrieved without complex logic.\n\n> Signed-off-by: Dragos Foianu \n> ---\n> branch.c | 44 +++++++++++++++++++++++++-------------------\n> 1 file changed, 25 insertions(+), 19 deletions(-)\n>\n> diff --git a/branch.c b/branch.c\n> index 723a36b..95645d5 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -54,6 +54,18 @@ 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 *messages[] = {\n> + N_(\"Branch %s set up to track local ref %s.\"),\n> + N_(\"Branch %s set up to track remote ref %s.\"),\n> + N_(\"Branch %s set up to track local branch %s.\"),\n> + N_(\"Branch %s set up to track remote branch %s from %s.\"),\n> + N_(\"Branch %s set up to track local ref %s by rebasing.\"),\n> + N_(\"Branch %s set up to track remote ref %s by rebasing.\"),\n> + N_(\"Branch %s set up to track local branch %s by rebasing.\"),\n> + N_(\"Branch %s set up to track remote branch %s from %s by rebasing.\")\n> + };\n> + int index = 0;\n> +\n> if (remote_is_branch\n> && !strcmp(local, shortname)\n> && !origin) {\n> @@ -76,28 +88,22 @@ void install_branch_config(int flag, const char *local, const char *origin, cons\n> }\n> strbuf_release(&key);\n>\n> + if (origin)\n> + index += 1;\n> + if (remote_is_branch)\n> + index += 2;\n> + if (rebasing)\n> + index += 4;\n\nOther submissions have computed this value mathematically without need\nfor conditionals. For instance, we've seen:\n\n index = (!!origin << 0) + (!!remote_is_branch << 1) + (!!rebasing << 2)\n\nas, well as the equivalent:\n\n index = !!origin + !!remote_is_branch * 2 + !!rebasing * 4\n\nAlthough this works, it does place greater cognitive demands on the\nreader by requiring more effort to figure out what is going on and how\nit relates to table position. The original (ungainly) chain of 'if'\nstatements in the original code does not suffer this problem. It\nlikewise is harder to understand than merely indexing into a\nmulti-dimension table where each variable is a key.\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> - 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> - 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> - 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(_(messages[index]),\n> + local, shortname, origin);\n> else\n> + printf_ln(_(messages[index]),\n> + local, (!remote_is_branch) ? remote : shortname);\n\nIt's possible to simplify this logic and have only a single\nprintf_ln() invocation. Hint: It's safe to pass in more arguments than\nthere are %s directives in the format string.\n\n> +\n> + if (index < 0 || index > sizeof(messages) / sizeof(*messages))\n> die(\"BUG: impossible combination of %d and %p\",\n> remote_is_branch, origin);\n\nYou can use ARRAY_SIZE() in place of sizeof(...)/sizeof(...).\n\nSince an out-of-bound index would be a programmer bug, it would\nprobably be more appropriate to use an assert(), just after 'index' is\ncomputed, rather than if+die(). The original code used die() because\nit couldn't detect the error until the end of the if-chain.\n\n> }\n> --\n> 1.8.3.2\n"},{"id":"237026","messageId":"CAPig+cQKHQFNBob18g9UmZuE_mOpF3UMCBPfSKJYEYQpk1Z_tw@mail.gmail.com","threadId":"36198","inReplyTo":"CAPig+cS9QApn1T3-R8n+W+1ee9FbNftsmhrr90SJKs+gqzvC5A@mail.gmail.com","subject":"Re: [PATCHv2] branch.c: simplify chain of if statements","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-18T23:50:59Z","receivedAt":"2014-03-18T23:50:59Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 18, 2014 at 6:31 PM, Eric Sunshine wrote:\n> On Mon, Mar 17, 2014 at 11:51 AM, Dragos Foianu wrote:\n>> This patch uses a table to store the different messages that can\n>> be emitted by the verbose install_branch_config function. It\n>> computes an index based on the three flags and prints the message\n>> located at the specific index in the table of messages. If the\n>> index somehow is not within the table, we have a bug.\n>>\n>> Signed-off-by: Dragos Foianu \n>> ---\n>> branch.c | 44 +++++++++++++++++++++++++-------------------\n>> 1 file changed, 25 insertions(+), 19 deletions(-)\n>>\n>> diff --git a/branch.c b/branch.c\n>> index 723a36b..95645d5 100644\n>> --- a/branch.c\n>> +++ b/branch.c\n>> @@ -54,6 +54,18 @@ void install_branch_config(int flag, const char *local, const char *origin, cons\n>> + int index = 0;\n>> + if (origin)\n>> + index += 1;\n>> + if (remote_is_branch)\n>> + index += 2;\n>> + if (rebasing)\n>> + index += 4;\n>> +\n>> + if (index < 0 || index > sizeof(messages) / sizeof(*messages))\n>> die(\"BUG: impossible combination of %d and %p\",\n>> remote_is_branch, origin);\n\nOne other observation: You have a one-off error in your out-of-bounds\ncheck. It should be 'index >= sizeof...'\n\n> You can use ARRAY_SIZE() in place of sizeof(...)/sizeof(...).\n>\n> Since an out-of-bound index would be a programmer bug, it would\n> probably be more appropriate to use an assert(), just after 'index' is\n> computed, rather than if+die(). The original code used die() because\n> it couldn't detect the error until the end of the if-chain.\n>\n>> }\n>> --\n>> 1.8.3.2\n"},{"id":"237131","messageId":"loom.20140320T001131-702@post.gmane.org","threadId":"36198","inReplyTo":"CAPig+cQKHQFNBob18g9UmZuE_mOpF3UMCBPfSKJYEYQpk1Z_tw@mail.gmail.com","subject":"Re: [PATCHv2] branch.c: simplify chain of if statements","fromName":"Dragos Foianu","fromEmail":"dragos.foianu@gmail.com","sentAt":"2014-03-19T23:12:14Z","receivedAt":"2014-03-19T23:12:14Z","isPatch":false,"sender":{"key":"dragos.foianu@gmail.com","avatar":null},"body":"Eric Sunshine sunshineco.com> writes:\n\n> \n> Other submissions have computed this value mathematically without need\n> for conditionals. For instance, we've seen:\n> \n> index = (!!origin << 0) + (!!remote_is_branch << 1) + (!!rebasing << 2)\n> \n> as, well as the equivalent:\n> \n> index = !!origin + !!remote_is_branch * 2 + !!rebasing * 4\n> \n> Although this works, it does place greater cognitive demands on the\n> reader by requiring more effort to figure out what is going on and how\n> it relates to table position. The original (ungainly) chain of 'if'\n> statements in the original code does not suffer this problem. It\n> likewise is harder to understand than merely indexing into a\n> multi-dimension table where each variable is a key.\n\nI have seen other submissions using this logic, but I didn't think it\naccomplished the goal of the patch - simplifying the code. Not that my\napproach does either, but I found it a little easier to understand.\n\nIndexing into a table like this is always going to have this problem so this\nis probably not the right approach to accomplishing the microproject's goals.\n\n> It's possible to simplify this logic and have only a single\n> printf_ln() invocation. Hint: It's safe to pass in more arguments than\n> there are %s directives in the format string.\n\nIndeed. It's a habit of mine to pass the exact number of arguments to printf\nfunctions and I can't seem to get away from it.\n\n> You can use ARRAY_SIZE() in place of sizeof(...)/sizeof(...).\n> \n> Since an out-of-bound index would be a programmer bug, it would\n> probably be more appropriate to use an assert(), just after 'index' is\n> computed, rather than if+die(). The original code used die() because\n> it couldn't detect the error until the end of the if-chain.\n\nThank you for this hint. Using already defined helpers in the project is\nbetter and will prevent the need to patch the constructs later on.\n\n> On Tue, Mar 18, 2014 at 6:31 PM, Eric Sunshine \nsunshineco.com> wrote:\n> \n> One other observation: You have a one-off error in your out-of-bounds\n> check. It should be 'index >= sizeof...'\n\nWell this is embarrasing.\n\nThank you again for the feedback. It's incredibily helpful and I learned a\nlot from submitting these patches. Making the code simple is harder than it\nappears at first sight.\n\nI'm not sure it's worth pursuing the table approach further, especially\nsince a solution has already been accepted and merged into the codebase.\n\nIn this case, is it okay to try another microproject? I was thinking about\ntrying #17 (finding bugs/inefficiencies in builtin/apply.c), but I've\nalready had my one microproject.\n\nAll the best,\nDragos\n"},{"id":"237246","messageId":"CAPig+cQpO+0hfqVKCzi1DwbosQ=smK=JiTPcYM8iP4r7VnSKjQ@mail.gmail.com","threadId":"36198","inReplyTo":"loom.20140320T001131-702@post.gmane.org","subject":"Re: [PATCHv2] branch.c: simplify chain of if statements","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-21T00:40:40Z","receivedAt":"2014-03-21T00:40:40Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 19, 2014 at 7:12 PM, Dragos Foianu wrote:\n> Eric Sunshine sunshineco.com> writes:\n>> On Tue, Mar 18, 2014 at 6:31 PM, Eric Sunshine \n> sunshineco.com> wrote:\n>>\n>> One other observation: You have a one-off error in your out-of-bounds\n>> check. It should be 'index >= sizeof...'\n>\n> Well this is embarrassing.\n\nIt's a good illustration of the value of the review process. It's easy\nto overlook omissions and problems in our one's work because one reads\nit with the bias of knowing what it's _supposed_ to say. Reviewers\n(hopefully) don't have such bias: they read the code afresh.\n\n> Thank you again for the feedback. It's incredibily helpful and I learned a\n> lot from submitting these patches. Making the code simple is harder than it\n> appears at first sight.\n>\n> I'm not sure it's worth pursuing the table approach further, especially\n> since a solution has already been accepted and merged into the codebase.\n\nAgreed.\n\n> In this case, is it okay to try another microproject? I was thinking about\n> trying #17 (finding bugs/inefficiencies in builtin/apply.c), but I've\n> already had my one micro project.\n\nAccording to the description for #17, there are plenty of opportunities, so...\n\n> All the best,\n> Dragos\n"},{"id":"237247","messageId":"CAPig+cS2rQSAPVEN6bzSNnjFoEzf9fyBA7X7P9+cmBFOsfA1xg@mail.gmail.com","threadId":"36198","inReplyTo":"CAPig+cQpO+0hfqVKCzi1DwbosQ=smK=JiTPcYM8iP4r7VnSKjQ@mail.gmail.com","subject":"Re: [PATCHv2] branch.c: simplify chain of if statements","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-21T00:44:09Z","receivedAt":"2014-03-21T00:44:09Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 20, 2014 at 8:40 PM, Eric Sunshine wrote:\n> On Wed, Mar 19, 2014 at 7:12 PM, Dragos Foianu wrote:\n>> Eric Sunshine sunshineco.com> writes:\n>>> On Tue, Mar 18, 2014 at 6:31 PM, Eric Sunshine \n>> sunshineco.com> wrote:\n>>>\n>>> One other observation: You have a one-off error in your out-of-bounds\n>>> check. It should be 'index >= sizeof...'\n>>\n>> Well this is embarrassing.\n>\n> It's a good illustration of the value of the review process. It's easy\n> to overlook omissions and problems in our one's work because one reads\n> it with the bias of knowing what it's _supposed_ to say. Reviewers\n> (hopefully) don't have such bias: they read the code afresh.\n\nAnd, this is a perfect example. I knew that I wanted to say \"problems\nin one's own work\", and even though I proof-read, I still missed that\nI wrote \"problems in our one's work\".\n"},{"id":"237322","messageId":"xmqq4n2rwl59.fsf@gitster.dls.corp.google.com","threadId":"36198","inReplyTo":"loom.20140320T001131-702@post.gmane.org","subject":"Re: [PATCHv2] branch.c: simplify chain of if statements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-21T16:50:58Z","receivedAt":"2014-03-21T16:50:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragos Foianu writes:\n\n> I'm not sure it's worth pursuing the table approach further, especially\n> since a solution has already been accepted and merged into the codebase.\n\nYes.\n\nI would further say that you already qualify as having finished a\nmicroproject, if I were a part of the candidate selection panel.\n\nThe important thing is for potential candidates to learn the\nprocess, not to have their change merged somewhere my tree, and you\nand many others who did a microproject and tasted the process of\nproposing a change, getting reviewed and learning what are expected\nof their patch submissions have finished that part already.\n"}]}