{"thread":{"id":"36145","subject":"[PATCH][GSOC2014] install_branch_config: change logical chain to lookup table","startedAt":"2014-03-12T21:24:10Z","lastAt":"2014-03-17T06:52:53Z","messageCount":4,"participants":["TamerTas","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"236639","messageId":"BLU0-SMTP223196BC56240FAE28FF9DD5760@phx.gbl","threadId":"36145","inReplyTo":"http://git.661346.n2.nabble.com/PATCH-GSOC2014-changed-logical-chain-in-branch-c-to-lookup-tables-tp7605343p7605444.html","subject":"[PATCH][GSOC2014] install_branch_config: change logical chain to lookup table","fromName":"TamerTas","fromEmail":"tamertas@outlook.com","sentAt":"2014-03-12T21:24:10Z","receivedAt":"2014-03-12T21:24:10Z","isPatch":true,"sender":{"key":"tamertas@outlook.com","avatar":"https://gravatar.com/avatar/618df903c9a2fa12e76148ca7a106e3236594de6002a190c70e1ace5c30e0e40?d=mp&s=160"},"body":"\nSigned-off-by: TamerTas <tamertas@outlook.com>\n--\nThanks for the feedback. Comments below. \n\nI've made the suggested changes [1] to patch [2] \nbut, since there are different number of format \nspecifiers, an if-else clause is necessary. \nRemoving the if-else chain completely doesn't seem to be possible. \nSo making the format table-driven seems to be like an optional change.\n\n[1]: http://git.661346.n2.nabble.com/PATCH-GSOC2014-changed-logical-chain-in-branch-c-to-lookup-tables-tp7605343p7605444.html\n[2]: http://git.661346.n2.nabble.com/PATCH-GSOC2014-changed-logical-chain-in-branch-c-to-lookup-tables-tp7605343p7605407.html\n--\n branch.c |   44 +++++++++++++++++++++-----------------------\n 1 file changed, 21 insertions(+), 23 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 723a36b..1ccf30f 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -50,10 +50,25 @@ static int should_setup_rebase(const char *origin)\n void install_branch_config(int flag, const char *local, const char *origin, const char *remote)\n {\n \tconst char *shortname = remote + 11;\n+\tconst char *setup_message[] = {\n+\t\tN_(\"Branch %s set up to track local ref %s.\"),\n+\t\tN_(\"Branch %s set up to track local branch %s.\"),\n+\t\tN_(\"Branch %s set up to track remote ref %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 local branch %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 remote branch %s from %s by rebasing.\"),\n+\t}; \n+\n \tint remote_is_branch = starts_with(remote, \"refs/heads/\");\n \tstruct strbuf key = STRBUF_INIT;\n \tint rebasing = should_setup_rebase(origin);\n \n+\tint msg_index = (!!remote_is_branch << 0) +\n+\t\t\t(!!origin << 1) +\n+\t\t\t(!!rebasing << 2);\n+\n \tif (remote_is_branch\n \t    && !strcmp(local, shortname)\n \t    && !origin) {\n@@ -77,29 +92,12 @@ 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\telse\n-\t\t\tdie(\"BUG: impossible combination of %d and %p\",\n-\t\t\t    remote_is_branch, origin);\n+\t    if(remote_is_branch && origin)\n+\t\tprintf_ln(_(setup_message[msg_index]), local, shortname, origin);\n+\t    else if (remote_is_branch && !origin)\n+\t\tprintf_ln(_(setup_message[msg_index]), local, shortname);\n+\t    else\n+\t\tprintf_ln(_(setup_message[msg_index]), local, remote);\n \t}\n }\n \n-- \n1.7.9.5\n"},{"id":"236692","messageId":"CAPig+cRCKCcfYQVM=pyXUQtTsbaD8g=OKff+K5+Bd+kBgqAufg@mail.gmail.com","threadId":"36145","inReplyTo":"BLU0-SMTP223196BC56240FAE28FF9DD5760@phx.gbl","subject":"Re: [PATCH][GSOC2014] install_branch_config: change logical chain to lookup table","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-13T22:21:01Z","receivedAt":"2014-03-13T22:21:01Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 12, 2014 at 5:24 PM, TamerTas <tamertas@outlook.com> wrote:\n>\n> Signed-off-by: TamerTas <tamertas@outlook.com>\n> --\n\nThere should be three hyphens \"---\" here but somehow you have only two\n\"--\". Since \"---\" is detected automatically when the patch is applied,\nthis deviation can be problematic.\n\n> Thanks for the feedback. Comments below.\n\nThanks for the resubmission. Comments below. :-)\n\n> I've made the suggested changes [1] to patch [2]\n\nBetter. This is a more well-crafted submission.\n\n> but, since there are different number of format\n> specifiers, an if-else clause is necessary.\n> Removing the if-else chain completely doesn't seem to be possible.\n> So making the format table-driven seems to be like an optional change.\n\nExplaining why you chose one approach over another is indeed good\netiquette, and can forestall questions which reviewers may otherwise\nask.\n\nNote, however, that it is possible to move this logic into a table.\nClue: it is not an error to supply more arguments than there are %s's\nin the format string. This is not saying that you must make it\ntable-driven, but perhaps it may alter the reasons you gave above for\nrejecting it (assuming you still do).\n\n> [1]: http://git.661346.n2.nabble.com/PATCH-GSOC2014-changed-logical-chain-in-branch-c-to-lookup-tables-tp7605343p7605444.html\n> [2]: http://git.661346.n2.nabble.com/PATCH-GSOC2014-changed-logical-chain-in-branch-c-to-lookup-tables-tp7605343p7605407.html\n> --\n>  branch.c |   44 +++++++++++++++++++++-----------------------\n>  1 file changed, 21 insertions(+), 23 deletions(-)\n>\n> diff --git a/branch.c b/branch.c\n> index 723a36b..1ccf30f 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -50,10 +50,25 @@ static int should_setup_rebase(const char *origin)\n>  void install_branch_config(int flag, const char *local, const char *origin, const char *remote)\n>  {\n>         const char *shortname = remote + 11;\n> +       const char *setup_message[] = {\n> +               N_(\"Branch %s set up to track local ref %s.\"),\n> +               N_(\"Branch %s set up to track local branch %s.\"),\n> +               N_(\"Branch %s set up to track remote ref %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 local branch %s by rebasing.\"),\n> +               N_(\"Branch %s set up to track remote ref %s by rebasing.\"),\n> +               N_(\"Branch %s set up to track remote branch %s from %s by rebasing.\"),\n\nSome compilers will warn about the trailing comma, so you might want to drop it.\n\n> +       };\n> +\n>         int remote_is_branch = starts_with(remote, \"refs/heads/\");\n>         struct strbuf key = STRBUF_INIT;\n>         int rebasing = should_setup_rebase(origin);\n>\n> +       int msg_index = (!!remote_is_branch << 0) +\n> +                       (!!origin << 1) +\n> +                       (!!rebasing << 2);\n\nBetter than the last (buggy) attempt. Nevertheless, it's a fairly\nmagical incantation requiring more thought than some other approaches.\nHave you considered instead using a multi-dimensional array for the\nmessages and then indexing into it with these variables as direct keys\n(after using ! or !! to constrain them to 0 or 1)? Would that be\nbetter or worse?\n\n>         if (remote_is_branch\n>             && !strcmp(local, shortname)\n>             && !origin) {\n> @@ -77,29 +92,12 @@ 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> -               else\n> -                       die(\"BUG: impossible combination of %d and %p\",\n> -                           remote_is_branch, origin);\n> +           if(remote_is_branch && origin)\n> +               printf_ln(_(setup_message[msg_index]), local, shortname, origin);\n> +           else if (remote_is_branch && !origin)\n> +               printf_ln(_(setup_message[msg_index]), local, shortname);\n> +           else\n> +               printf_ln(_(setup_message[msg_index]), local, remote);\n\nAssigning setup_message[msg_index] to a variable and referencing that\nvariable in the printf_ln() invocations might make this less noisy.\n\nConsider the clue given above about making the code table-driven. Even\nif you don't go the table-driven route, that clue can simplify this\ncode considerably.\n\n>         }\n>  }\n>\n> --\n> 1.7.9.5\n"},{"id":"236750","messageId":"BLU0-SMTP430B54F52C27DDA0C405A5D5700@phx.gbl","threadId":"36145","inReplyTo":"CAPig+cRCKCcfYQVM=pyXUQtTsbaD8g=OKff+K5+Bd+kBgqAufg@mail.gmail.com","subject":"[PATCH][GSOC2014] install_branch_config: change logical chain to lookup table","fromName":"TamerTas","fromEmail":"tamertas@outlook.com","sentAt":"2014-03-14T21:30:54Z","receivedAt":"2014-03-14T21:30:54Z","isPatch":true,"sender":{"key":"tamertas@outlook.com","avatar":"https://gravatar.com/avatar/618df903c9a2fa12e76148ca7a106e3236594de6002a190c70e1ace5c30e0e40?d=mp&s=160"},"body":"Signed-off-by: TamerTas <tamertas@outlook.com>\n---\n\nThanks again for the feedback it's been a great learning experience. Comments Below :)\n\nI have refactored the commit [1] to * suggested changes [2].\nformat-patch was placing 2 hyphens instead of 3 but it's fixed now.\nI've turned the table into a multidimensional one and I didn't put\nthe inner braces since this was used method in other multidimensional arrays\nthroughout the project.\nI've also changed shortname to short_name since that seems to be how variables are named\nin this project.\nIt appears that table-driven code might be more readable after all.\n\n[1]http://git.661346.n2.nabble.com/PATCH-GSOC2014-install-branch-config-change-logical-chain-to-lookup-table-tp7605550.html\n[2]http://git.661346.n2.nabble.com/PATCH-GSOC2014-install-branch-config-change-logical-chain-to-lookup-table-tp7605550p7605605.html\n---\n branch.c |   42 +++++++++++++++++-------------------------\n 1 file changed, 17 insertions(+), 25 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 723a36b..eab6fa4 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -49,13 +49,27 @@ static int should_setup_rebase(const char *origin)\n \n void install_branch_config(int flag, const char *local, const char *origin, const char *remote)\n {\n-\tconst char *shortname = remote + 11;\n+\tconst char *short_name = remote + 11;\n+\tconst char *setup_message[][2][2] = {\n+\t\tN_(\"Branch %s set up to track local ref %s.\"), \n+\t\tN_(\"Branch %s set up to track local branch %s.\"),\n+\t\tN_(\"Branch %s set up to track remote ref %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 local branch %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 remote branch %s from %s by rebasing.\")\n+\t}; \n+\n \tint remote_is_branch = starts_with(remote, \"refs/heads/\");\n \tstruct strbuf key = STRBUF_INIT;\n \tint rebasing = should_setup_rebase(origin);\n \n+\tconst char *remote_name = remote_is_branch? short_name : remote;\n+\tconst char *message = setup_message[!!rebasing][!!origin][!!remote_is_branch];\n+\n \tif (remote_is_branch\n-\t    && !strcmp(local, shortname)\n+\t    && !strcmp(local, short_name)\n \t    && !origin) {\n \t\twarning(_(\"Not setting branch %s as its own upstream.\"),\n \t\t\tlocal);\n@@ -77,29 +91,7 @@ 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\telse\n-\t\t\tdie(\"BUG: impossible combination of %d and %p\",\n-\t\t\t    remote_is_branch, origin);\n+\t\tprintf_ln(_(message), local, remote_name, origin);\n \t}\n }\n \n---\n1.7.9.5\n"},{"id":"236856","messageId":"CAPig+cSCD32e=XgK=rRHJm944Dc=_jk+81LV124nv6L_kheD5g@mail.gmail.com","threadId":"36145","inReplyTo":"BLU0-SMTP430B54F52C27DDA0C405A5D5700@phx.gbl","subject":"Re: [PATCH][GSOC2014] install_branch_config: change logical chain to lookup table","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-17T06:52:53Z","receivedAt":"2014-03-17T06:52:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 14, 2014 at 5:30 PM, TamerTas <tamertas@outlook.com> wrote:\n> Signed-off-by: TamerTas <tamertas@outlook.com>\n> ---\n> Thanks again for the feedback it's been a great learning experience. Comments Below :)\n>\n> I have refactored the commit [1] to * suggested changes [2].\n> format-patch was placing 2 hyphens instead of 3 but it's fixed now.\n> I've turned the table into a multidimensional one and I didn't put\n> the inner braces since this was used method in other multidimensional arrays\n> throughout the project.\n\nThanks, this is looking better. A few minor comments below, but it's\nprobably not worth a re-roll. What is important is that you have had a\ntaste of the review process on this project, and the GSoC mentors have\nhad a chance to observe your abilities and reviewer interaction.\n\n> I've also changed shortname to short_name since that seems to be how variables are named\n> in this project.\n\nThis is a conceptually distinct change which should be done as a\nseparate \"cleanup\" patch since it otherwise adds noise which obscures\nthe \"real\" change (rewriting the if-chain). One of the goals of a\npatch submitter is to make the review process as streamlined as\npossible, so noise should be avoided.\n\nApart from that, unless the variable is really improperly named and\nmisleading, this sort of change (renaming \"shortname\" to \"short_name\")\nis likely to be considered unnecessary code churn which will probably\nbe rejected. At any given time, there are many patch series in-flight\nwhich Junio has to juggle, and code churn increases possibility of\nconflict between them, which makes his job more difficult.\n\n> It appears that table-driven code might be more readable after all.\n>\n> [1]http://git.661346.n2.nabble.com/PATCH-GSOC2014-install-branch-config-change-logical-chain-to-lookup-table-tp7605550.html\n> [2]http://git.661346.n2.nabble.com/PATCH-GSOC2014-install-branch-config-change-logical-chain-to-lookup-table-tp7605550p7605605.html\n> ---\n>  branch.c |   42 +++++++++++++++++-------------------------\n>  1 file changed, 17 insertions(+), 25 deletions(-)\n>\n> diff --git a/branch.c b/branch.c\n> index 723a36b..eab6fa4 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -49,13 +49,27 @@ static int should_setup_rebase(const char *origin)\n>\n>  void install_branch_config(int flag, const char *local, const char *origin, const char *remote)\n>  {\n> -       const char *shortname = remote + 11;\n> +       const char *short_name = remote + 11;\n> +       const char *setup_message[][2][2] = {\n> +               N_(\"Branch %s set up to track local ref %s.\"),\n> +               N_(\"Branch %s set up to track local branch %s.\"),\n> +               N_(\"Branch %s set up to track remote ref %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 local branch %s by rebasing.\"),\n> +               N_(\"Branch %s set up to track remote ref %s by rebasing.\"),\n> +               N_(\"Branch %s set up to track remote branch %s from %s by rebasing.\")\n> +       };\n> +\n>         int remote_is_branch = starts_with(remote, \"refs/heads/\");\n>         struct strbuf key = STRBUF_INIT;\n>         int rebasing = should_setup_rebase(origin);\n>\n> +       const char *remote_name = remote_is_branch? short_name : remote;\n> +       const char *message = setup_message[!!rebasing][!!origin][!!remote_is_branch];\n\nIn the previous review, it was suggested to pull this value out into\nits own variable to make the code less noisy since it was being\naccessed frequently in just a few lines of code. However, since you've\ncollapsed the code to a single printf_ln() invocation in this patch,\nthe separate variable may be not helping clarity (especially as the\nassignment is divorced by some distance from the code which references\nit). The same is probably true of 'remote_name' which is used in just\nthe one printf_ln() call.\n\n>         if (remote_is_branch\n> -           && !strcmp(local, shortname)\n> +           && !strcmp(local, short_name)\n>             && !origin) {\n>                 warning(_(\"Not setting branch %s as its own upstream.\"),\n>                         local);\n> @@ -77,29 +91,7 @@ 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> -               else\n> -                       die(\"BUG: impossible combination of %d and %p\",\n> -                           remote_is_branch, origin);\n> +               printf_ln(_(message), local, remote_name, origin);\n>         }\n>  }\n>\n> ---\n> 1.7.9.5\n"}]}