{"thread":{"id":"36072","subject":"[PATH] branch.c:install_branch_config():Simplify code generating verbose message.","startedAt":"2014-03-06T07:58:59Z","lastAt":"2014-03-07T08:55:42Z","messageCount":2,"participants":["Paweł Wawruch","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"236150","messageId":"8889967.JbMR8CXVSx@endymion","threadId":"36072","inReplyTo":null,"subject":"[PATH] branch.c:install_branch_config():Simplify code generating verbose message.","fromName":"Paweł Wawruch","fromEmail":"pawlo@aleg.pl","sentAt":"2014-03-06T07:58:59Z","receivedAt":"2014-03-06T07:58:59Z","isPatch":false,"sender":{"key":"pawlo@aleg.pl","avatar":"https://gravatar.com/avatar/17564135d51785c0ca06c280fdee1c08c803d68c3227d9e91b78db28b69d3945?d=mp&s=160"},"body":">From adfcfa0a334378a6242347efc0d614fa193610db Mon Sep 17 00:00:00 2001\nFrom: =?UTF-8?q?Pawe=C5=82=20Wawruch?= \nDate: Thu, 6 Mar 2014 00:05:00 +0100\nSubject: [PATCH] branch.c:install_branch_config(): Simplify the long chain of\n if statements. Threre was a long chain of if statements. The code can be more\n clear. The chain is replaced with table of strings. New approach is more\n compact.\n\n---\n branch.c | 38 ++++++++++++++++----------------------\n 1 file changed, 16 insertions(+), 22 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 723a36b..ebc2172 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -77,29 +77,23 @@ void install_branch_config(int flag, const char *local, \nconst 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 \nrebasing.\") :\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\tconst char *messages[8] ;\n+\t\tmessages[0] = _(\"Branch %s set up to track remote branch %s from %s by \nrebasing.\");\n+\t\tmessages[1] = _(\"Branch %s set up to track remote branch %s from %s.\");\n+\t\tmessages[2] = _(\"Branch %s set up to track local branch %s by \nrebasing.\");\n+\t\tmessages[3] = _(\"Branch %s set up to track local branch %s.\");\n+\t\tmessages[4] = _(\"Branch %s set up to track remote ref %s by rebasing.\");\n+\t\tmessages[5] = _(\"Branch %s set up to track remote ref %s.\");\n+\t\tmessages[6] = _(\"Branch %s set up to track local ref %s by rebasing.\");\n+\t\tmessages[7] = _(\"Branch %s set up to track local ref %s.\");\n+\n+\t\tconst char *name = remote_is_branch ? remote : shortname;\n+\t\tint message_number = rebasing + 2 * (origin != NULL) + 4 * \nremote_is_branch;\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":"236255","messageId":"CAPig+cQTSMZq1X7131cbnAB7py_3GXf60ricX4hq6DNB7uHsbQ@mail.gmail.com","threadId":"36072","inReplyTo":"8889967.JbMR8CXVSx@endymion","subject":"Re: [PATH] branch.c:install_branch_config():Simplify code generating verbose message.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-07T08:55:42Z","receivedAt":"2014-03-07T08:55:42Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Thanks for the submission. Some comments below to give you a taste of\nthe Git review process...\n\nOn Thu, Mar 6, 2014 at 2:58 AM, Paweł Wawruch wrote:\n> From adfcfa0a334378a6242347efc0d614fa193610db Mon Sep 17 00:00:00 2001\n> From: =?UTF-8?q?Pawe=C5=82=20Wawruch?= \n> Date: Thu, 6 Mar 2014 00:05:00 +0100\n\nDrop these lines. They are extracted automatically by \"git am\" from\nthe headers of your mail message.\n\n> Subject: [PATCH] branch.c:install_branch_config(): Simplify the long chain of\n> if statements. Threre was a long chain of if statements. The code can be more\n> clear. The chain is replaced with table of strings. New approach is more\n> compact.\n\nThe Subject: also is extracted automatically from the email, so it\nshould not be pasted here. Instead, when you compose your commit\nmessage, write a very concise summary of the patch as the very first\nline, then a blank line, then the meat of the description using as\nmany lines as needed. Wrap the lines to about 65-70 characters. \"git\nformat-patch\" automatically uses that concise first line as the\nSubject: of the mail message. This is all explained in section \"(2)\nDescribe your changes well\" of Documentation/SubmittingPatches.\n\nWrite in imperative tone. Rather than \"There was a long...\", say\n\"There is a long...\". Rather than \"The chain is replaced...\", say\n\"Replace the chain...\".\n\nYou misspelled \"There\" as \"Threre\".\n\nYour patch sign-off is missing. It should be placed right here above\nthe \"---\" line. See Documentation/SubmittingPatches.\n\n> ---\n\nThe Git GSoC page on which you found this microproject suggests\nmentioning that this is a GSoC submission so that reviewers don't\noverlook it. Either do so by adding [GSoC] to the subject or by\nmentioning it right here as \"patch commentary\" after the \"---\" line.\n\n> branch.c | 38 ++++++++++++++++----------------------\n> 1 file changed, 16 insertions(+), 22 deletions(-)\n>\n> diff --git a/branch.c b/branch.c\n> index 723a36b..ebc2172 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -77,29 +77,23 @@ void install_branch_config(int flag, const char *local,\n> const char *origin, cons\n> strbuf_release(&key);\n\nYour patch is whitespace damaged, and thus not easily applied. This\nprobably resulted from pasting it into your email client. Using \"git\nsend-email\" avoids this problem.\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\n> 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> + const char *messages[8] ;\n\nDrop the space before the semicolon.\n\nOn this project, we avoid intermixing declarations with code, so move\nthe declaration of messages[] to the top of the block.\n\n> + messages[0] = _(\"Branch %s set up to track remote branch %s from %s by\n> rebasing.\");\n> + messages[1] = _(\"Branch %s set up to track remote branch %s from %s.\");\n> + messages[2] = _(\"Branch %s set up to track local branch %s by\n> rebasing.\");\n> + messages[3] = _(\"Branch %s set up to track local branch %s.\");\n> + messages[4] = _(\"Branch %s set up to track remote ref %s by rebasing.\");\n> + messages[5] = _(\"Branch %s set up to track remote ref %s.\");\n> + messages[6] = _(\"Branch %s set up to track local ref %s by rebasing.\");\n> + messages[7] = _(\"Branch %s set up to track local ref %s.\");\n\nRather than hardcoding the array size and then assigning the elements\nindividually, perhaps you can construct the array with an initializer.\nUse N_(...) rather than _(...).\n\n const char *messages[] = {\n N_(\"...\"),\n ...\n };\n\n> + const char *name = remote_is_branch ? remote : shortname;\n> + int message_number = rebasing + 2 * (origin != NULL) + 4 *\n> remote_is_branch;\n\nMove the declarations of 'name' and 'message_number' to the top of the block.\n\nI don't know how other people feel about it, but it disturbs me a bit\nto see message_number computed from these other variables without\nensuring that their values really are 0 or 1. I checked the\nimplementation of the functions from which these variables are set, so\nI know that they are indeed 0 or 1, but doing so was an extra burden\nwhich wouldn't have been required if this code was more explicit about\ntheir values. For instance:\n\n int message_number = !!rebasing + 2 * !!origin + 4 * !!remote_is_branch;\n\n> + if (message_number < 2)\n> + printf_ln(messages[message_number], local, name, origin);\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\nAgain, it disturbs me to see messages[] indexed without ensuring that\nmessage_number is within range. At the very least, an\n\n assert(message_number < ARRAY_SIZE(messages));\n\nafter computing message_number's value would help to protect against\nprogrammer error in future changes.\n\n> }\n> }\n>\n> --\n> 1.8.3.2\n"}]}