{"thread":{"id":"36117","subject":"[PATHv2] branch.c:install_branch_config():Simplify code generating verbose message.","startedAt":"2014-03-11T08:40:07Z","lastAt":"2014-03-11T10:16:49Z","messageCount":2,"participants":["Paweł Wawruch","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"236488","messageId":"1394527207-23065-1-git-send-email-pawlo@aleg.pl","threadId":"36117","inReplyTo":null,"subject":"[PATHv2] branch.c:install_branch_config():Simplify code generating verbose message.","fromName":"Paweł Wawruch","fromEmail":"pawlo@aleg.pl","sentAt":"2014-03-11T08:40:07Z","receivedAt":"2014-03-11T08:40:07Z","isPatch":false,"sender":{"key":"pawlo@aleg.pl","avatar":"https://gravatar.com/avatar/17564135d51785c0ca06c280fdee1c08c803d68c3227d9e91b78db28b69d3945?d=mp&s=160"},"body":"Simplify the long if chain in install_branch_config().\n\nThere is a long chain of if statements. The code can be more clear.\nReplace the chain with table of strings. New approach is more\ncompact.\n\nSigned-off-by: Paweł Wawruch <pawlo@aleg.pl>\n---\n branch.c | 40 ++++++++++++++++++----------------------\n 1 file changed, 18 insertions(+), 22 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 723a36b..8d3b219 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -53,6 +53,18 @@ void install_branch_config(int flag, const char *local, const char *origin, cons\n \tint remote_is_branch = starts_with(remote, \"refs/heads/\");\n \tstruct strbuf key = STRBUF_INIT;\n \tint rebasing = should_setup_rebase(origin);\n+\tconst char *messages[] = {\n+\t\tN_(\"Branch %s set up to track remote branch %s from %s by rebasing.\"),\n+\t\tN_(\"Branch %s set up to track remote branch %s from %s.\"),\n+\t\tN_(\"Branch %s set up to track local branch %s by rebasing.\"),\n+\t\tN_(\"Branch %s set up to track local branch %s.\"),\n+\t\tN_(\"Branch %s set up to track remote ref %s by rebasing.\"),\n+\t\tN_(\"Branch %s set up to track remote ref %s.\"),\n+\t\tN_(\"Branch %s set up to track local ref %s by rebasing.\"),\n+\t\tN_(\"Branch %s set up to track local ref %s.\")\n+\t};\n+\tconst char *name = remote_is_branch ? remote : shortname;\n+\tint message_number;\n \n \tif (remote_is_branch\n \t    && !strcmp(local, shortname)\n@@ -77,29 +89,13 @@ void install_branch_config(int flag, const char *local, const char *origin, cons\n \tstrbuf_release(&key);\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\tmessage_number = (!!rebasing) + 2 * (!!origin) + 4 * (!!remote_is_branch);\n+\t\tassert(message_number < ARRAY_SIZE(messages));\n+\n+\t\tif (message_number < 2)\n+\t\t\tprintf_ln(messages[message_number], local, name, origin);\n \t\telse\n-\t\t\tdie(\"BUG: impossible combination of %d and %p\",\n-\t\t\t    remote_is_branch, origin);\n+\t\t\tprintf_ln(messages[message_number], local, name);\n \t}\n }\n \n-- \n1.8.3.2\n"},{"id":"236491","messageId":"CAPig+cQdbXz_s2-i0eTOq+rmacNWUvvCbeSZ4Z=JY39oNKFQ_Q@mail.gmail.com","threadId":"36117","inReplyTo":"1394527207-23065-1-git-send-email-pawlo@aleg.pl","subject":"Re: [PATHv2] branch.c:install_branch_config():Simplify code generating verbose message.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-11T10:16:49Z","receivedAt":"2014-03-11T10:16:49Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Thanks for taking review comments from your previous attempt into\naccount. This is a more well-crafted submission. Additional comments\nbelow.\n\nOn Tue, Mar 11, 2014 at 4:40 AM, Paweł Wawruch <pawlo@aleg.pl> wrote:\n> Subject: [PATHv2] branch.c:install_branch_config():Simplify code generating verbose message.\n\nPATCH is misspelled as PATH. Adding the v2 is indeed good etiquette.\nIt's typical to have a space between PATCH and vN.\n\nInsert a space before \"Simplify\".\n\nSubmittingPatches suggests omitting the final period in the subject.\nAlso downcase \"simplify\".\n\n> Simplify the long if chain in install_branch_config().\n\nA bit misleading. You're not actually simplifying the 'if' chain, but\nrather replacing it.\n\n> There is a long chain of if statements. The code can be more clear.\n> Replace the chain with table of strings. New approach is more\n> compact.\n\nThe first sentence merely repeats what was said earlier. It can be\ndropped. The second sentence is self-evident since you already talk\nabout \"simplification\" and also can be dropped, as can the fourth\nsentence. The third sentence is a good explanation of how the code is\nbeing simplified and should be kept. You could, therefore, collapse\nthe commit message to something along these lines:\n\n    Subject: install_branch_config: simplify verbose diagnostic logic\n\n    Replace the 'if' chain with a table of strings.\n\n> Signed-off-by: Paweł Wawruch <pawlo@aleg.pl>\n> ---\n\nIt is considerate to reviewers to provide a link to the previous\nversion of the patch, like this [1], and to explain what changed in\nthis version relative to the last. This area just below the \"---\" line\nafter your sign off is where you would write such commentary.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/243502\n\n>  branch.c | 40 ++++++++++++++++++----------------------\n>  1 file changed, 18 insertions(+), 22 deletions(-)\n>\n> diff --git a/branch.c b/branch.c\n> index 723a36b..8d3b219 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -53,6 +53,18 @@ void install_branch_config(int flag, const char *local, const char *origin, cons\n>         int remote_is_branch = starts_with(remote, \"refs/heads/\");\n>         struct strbuf key = STRBUF_INIT;\n>         int rebasing = should_setup_rebase(origin);\n> +       const char *messages[] = {\n> +               N_(\"Branch %s set up to track remote branch %s from %s by rebasing.\"),\n> +               N_(\"Branch %s set up to track remote branch %s from %s.\"),\n> +               N_(\"Branch %s set up to track local branch %s by rebasing.\"),\n> +               N_(\"Branch %s set up to track local branch %s.\"),\n> +               N_(\"Branch %s set up to track remote ref %s by rebasing.\"),\n> +               N_(\"Branch %s set up to track remote ref %s.\"),\n> +               N_(\"Branch %s set up to track local ref %s by rebasing.\"),\n> +               N_(\"Branch %s set up to track local ref %s.\")\n> +       };\n\nOn this project, arrays are usually (though not consistently) named in\nsingular form (for instance message[]) so that a reference to a single\nitem, such as message[42], reads more grammatically correct.\n\n> +       const char *name = remote_is_branch ? remote : shortname;\n> +       int message_number;\n>\n>         if (remote_is_branch\n>             && !strcmp(local, shortname)\n> @@ -77,29 +89,13 @@ void install_branch_config(int flag, const char *local, const char *origin, cons\n>         strbuf_release(&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> +               message_number = (!!rebasing) + 2 * (!!origin) + 4 * (!!remote_is_branch);\n\nUnnecessary parentheses make the code more cumbersome to read and\nshould be dropped.\n\n> +               assert(message_number < ARRAY_SIZE(messages));\n> +\n> +               if (message_number < 2)\n> +                       printf_ln(messages[message_number], local, name, origin);\n\nEven though the strings are marked for translation via N_() in the\ninitializer, they still need to be passed through _() when they are\nactually used, so the invocation should be:\n\n    printf_ln(_(messages[message_number]), local, name, origin);\n\nDitto for the printf_ln() below.\n\nSee section 4.7 of the GNU gettext manual [2] for an explanation (and\nnote that N_() is shorthand for gettext_noop() mentioned in the\nmanual).\n\n[2]: http://www.gnu.org/software/gettext/manual/gettext.html\n\n>                 else\n> -                       die(\"BUG: impossible combination of %d and %p\",\n> -                           remote_is_branch, origin);\n> +                       printf_ln(messages[message_number], local, name);\n>         }\n\nAlthough this approach of making message retrieval table-driven does\ncollapse the code a bit, it's not obvious that it is indeed more\nclear. Magical incantations, such as '!!rebasing + 2 * !!origin + 4 *\n!!remote_is_branch' and 'if (message_number < 2)', convey little\nimmediate meaning, demand a greater cognitive load, and may make the\ncode less straightforward to modify in the future. Have you considered\nother ways of making the code table-driven which exhibit fewer\nshortcomings?\n\n>  }\n>\n> --\n> 1.8.3.2\n"}]}