{"thread":{"id":"36114","subject":"[PATCH][GSoC] branch.c:install_branch_config Simplified long chain of if statements","startedAt":"2014-03-10T22:04:01Z","lastAt":"2014-03-10T23:55:00Z","messageCount":3,"participants":["Vincenzo di Cicco","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"236467","messageId":"1394489041-2377-1-git-send-email-enzodicicco@gmail.com","threadId":"36114","inReplyTo":null,"subject":"[PATCH][GSoC] branch.c:install_branch_config Simplified long chain of if statements","fromName":"Vincenzo di Cicco","fromEmail":"enzodicicco@gmail.com","sentAt":"2014-03-10T22:04:01Z","receivedAt":"2014-03-10T22:04:01Z","isPatch":true,"sender":{"key":"enzodicicco@gmail.com","avatar":"https://gravatar.com/avatar/c16429519a685a3688e67a55b0bdb6876a3bc96a064ffa8806b26463166fa3e2?d=mp&s=160"},"body":"From: NaN \n\nHi there, I've made this patch in according to the rules to participate at GSoC.\nI've seen other patches about this issue very well constructed, so this is only another way to solve this microproject and to test how I can send a patch and discuss about it.\n\nThanks,\nNaN\n\nSigned-off-by: Vincenzo di Cicco \n---\n Table-driven approach to avoid the long chain of if statements.\n\n branch.c | 37 ++++++++++++++-----------------------\n 1 file changed, 14 insertions(+), 23 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 723a36b..cb8a544 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -53,6 +53,10 @@ 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+\tstruct strbuf msg = STRBUF_INIT;\n+\tchar *locations[2][2] = {{\"locate ref \\%s\", \"local branch \\%s\"},\n+\t\t\t\t {\"remote ref \\%s\", \"remote branch \\%s from \\%s\"}};\n+\tchar *location;\n\n \tif (remote_is_branch\n \t && !strcmp(local, shortname)\n@@ -77,30 +81,17 @@ 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\tlocation = locations[origin != NULL][remote_is_branch];\n+\n+\t\tstrbuf_addstr(&msg, \"Branch \\%s set up to track \");\n+\t\tstrbuf_addstr(&msg, location);\n+\t\tif(rebasing)\n+\t\t\tstrbuf_addstr(&msg, \" by rebasing\");\n+\t\tstrbuf_addch(&msg, '.');\n+\n+\t\tprintf_ln(_(msg.buf), local, remote_is_branch ? remote: shortname, origin);\n \t}\n+\tstrbuf_release(&msg);\n }\n\n /*\n--\n1.9.0\n"},{"id":"236469","messageId":"CAPig+cRZyvrPL69eunsV89-AycF=TWeN8x5cUvBhcF0MLVvOog@mail.gmail.com","threadId":"36114","inReplyTo":"1394489041-2377-1-git-send-email-enzodicicco@gmail.com","subject":"Re: [PATCH][GSoC] branch.c:install_branch_config Simplified long chain of if statements","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-10T23:17:19Z","receivedAt":"2014-03-10T23:17:19Z","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 feel for the\nGit review process...\n\nOn Mon, Mar 10, 2014 at 6:04 PM, Vincenzo di Cicco\n wrote:\n> From: NaN \n\nDrop this line unless it is intentionally different from your email\nFrom: header, which \"git am\" will pick up automatically when applying\nyour patch. On this project, real names are preferred (as you\ncorrectly used in your sign-off).\n\n> Hi there, I've made this patch in according to the rules to participate at GSoC.\n> I've seen other patches about this issue very well constructed, so this is only another way to solve this microproject and to test how I can send a patch and discuss about it.\n>\n> Thanks,\n> NaN\n\nThese \"commentary\" lines, which are not intended as part of the\nofficial commit message, belong below the \"---\" line following your\nsign-off. Wrap them to 65-70 characters.\n\n> Signed-off-by: Vincenzo di Cicco \n> ---\n> Table-driven approach to avoid the long chain of if statements.\n\nThis non-commentary information is suitable for the commit message,\nthus it belongs above your sign-off.\n\n> branch.c | 37 ++++++++++++++-----------------------\n> 1 file changed, 14 insertions(+), 23 deletions(-)\n>\n> diff --git a/branch.c b/branch.c\n> index 723a36b..cb8a544 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -53,6 +53,10 @@ 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> + struct strbuf msg = STRBUF_INIT;\n> + char *locations[2][2] = {{\"locate ref \\%s\", \"local branch \\%s\"},\n> + {\"remote ref \\%s\", \"remote branch \\%s from \\%s\"}};\n> + char *location;\n\nUse 'const char *'. You can probably drop the hard-coded array dimensions.\n\nThese strings ought to be internationalized, as in the original code,\nthus they should be wrapped with N_().\n\n> if (remote_is_branch\n> && !strcmp(local, shortname)\n> @@ -77,30 +81,17 @@ 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> + location = locations[origin != NULL][remote_is_branch];\n\nOn this project, it is more idiomatic to say !!origin than 'origin != NULL'.\n\nIf we take the time to trace through the code, we can see that\nremote_is_branch is indeed either 0 or 1, thus this expression is safe\ntoday, however, if the implementation of starts_with() ever changes so\nthat it returns a value other than 1 for true, then this code will\nbreak. To avoid such breakage, and to avoid placing burden of tracing\ncode, you might instead write the expression as:\n\n location = locations[!!origin][!!remote_is_branch];\n\n> + strbuf_addstr(&msg, \"Branch \\%s set up to track \");\n> + strbuf_addstr(&msg, location);\n> + if(rebasing)\n> + strbuf_addstr(&msg, \" by rebasing\");\n> + strbuf_addch(&msg, '.');\n\nThis approach of composing strings is problematic for translation,\nwhich is why the GSoC microproject states:\n\n Don't forget that the strangely-named function _() is used for\n internationalization and limits the possibility of gluing\n strings together.\n\nFor further details about why this approach is undesirable, see this\nother email thread [1].\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/243793\n\n> + printf_ln(_(msg.buf), local, remote_is_branch ? remote: shortname, origin);\n\ngettext function _() expects constant strings which it can present to\n(human) translators, so passing it a buffer containing a string\ncomposed at run-time will not work.\n\n> }\n> + strbuf_release(&msg);\n> }\n>\n> /*\n> --\n> 1.9.0\n"},{"id":"236471","messageId":"CAKOJyXcVM0xujy8yCMnOies_67CyWf1GgQ807rszXYxqf1JA5Q@mail.gmail.com","threadId":"36114","inReplyTo":"CAPig+cRZyvrPL69eunsV89-AycF=TWeN8x5cUvBhcF0MLVvOog@mail.gmail.com","subject":"Re: [PATCH][GSoC] branch.c:install_branch_config Simplified long chain of if statements","fromName":"Vincenzo di Cicco","fromEmail":"enzodicicco@gmail.com","sentAt":"2014-03-10T23:55:00Z","receivedAt":"2014-03-10T23:55:00Z","isPatch":true,"sender":{"key":"enzodicicco@gmail.com","avatar":"https://gravatar.com/avatar/c16429519a685a3688e67a55b0bdb6876a3bc96a064ffa8806b26463166fa3e2?d=mp&s=160"},"body":"Thank you very much for the answer!\nI've learned a lot in this few rows.\n\n> If we take the time to trace through the code, we can see that\n> remote_is_branch is indeed either 0 or 1, thus this expression is safe\n> today, however, if the implementation of starts_with() ever changes so\n> that it returns a value other than 1 for true, then this code will\n> break. To avoid such breakage, and to avoid placing burden of tracing\n> code, you might instead write the expression as:\n>\n> location = locations[!!origin][!!remote_is_branch];\n\nThanks for the good tip. I've erroneously assumed that starts_with()\nnever will change.\n\n> This approach of composing strings is problematic for translation,\n> which is why the GSoC microproject states:\n\nThanks to show me this issue. I've read the other thread and\nunderstood the problem.\n\nI will do better in future :)\n"}]}