{"thread":{"id":"36136","subject":"[PATCH] install_branch_config(): switch from 'else-if' series to 'nested if-else'","startedAt":"2014-03-12T08:34:35Z","lastAt":"2014-03-12T09:23:01Z","messageCount":2,"participants":["Nishhal Gaba","Matthieu Moy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"236565","messageId":"1394613275-3327-1-git-send-email-nishgaba9@gmail.com","threadId":"36136","inReplyTo":null,"subject":"[PATCH] install_branch_config(): switch from 'else-if' series to 'nested if-else'","fromName":"Nishhal Gaba","fromEmail":"nishgaba9@gmail.com","sentAt":"2014-03-12T08:34:35Z","receivedAt":"2014-03-12T08:34:35Z","isPatch":true,"sender":{"key":"nishgaba9@gmail.com","avatar":null},"body":"From: Nishchal <nishgaba9@gmail.com>\n\nI am Nishchal Gaba, a GSOC 2014 aspirant, submitting patch in response to microproject(8)\nSimilar Execution Time, but increased readability\n\nAlternate Solution Discarded: Table driven code using commonanilty of the statements to be printed due to _()\n\nSigned-off-by: Nishchal Gaba <nishgaba9@gmail.com>\n---\n---\n branch.c | 28 +++++++++++++++++-----------\n 1 file changed, 17 insertions(+), 11 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 723a36b..04e9e24 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -77,26 +77,32 @@ 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\tif (origin){\n+\t\t\tif(remote_is_branch)\n+\t\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\telse\n+\t\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}\n+\n+\t\telse if (!origin){\n+\t\t\tif(remote_is_branch)\n+\t\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\t\telse\n+\t\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}\n+\n \t\telse\n \t\t\tdie(\"BUG: impossible combination of %d and %p\",\n \t\t\t    remote_is_branch, origin);\n-- \n1.8.1.2\n"},{"id":"236568","messageId":"vpqmwgvrcsq.fsf@anie.imag.fr","threadId":"36136","inReplyTo":"1394613275-3327-1-git-send-email-nishgaba9@gmail.com","subject":"Re: [PATCH] install_branch_config(): switch from 'else-if' series to 'nested if-else'","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-03-12T09:23:01Z","receivedAt":"2014-03-12T09:23:01Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Nishhal Gaba <nishgaba9@gmail.com> writes:\n\n> From: Nishchal <nishgaba9@gmail.com>\n\nSet user.email/user.name and sendemail.from to the same address to avoid\nthis inline From:.\n\n> I am Nishchal Gaba, a GSOC 2014 aspirant, submitting patch in response to microproject(8)\n\nThis part of your message is the commit message. It should justify why\nthe change is good, but who you are is not very interesting here (think\nof someone running \"git log\" or \"git blame\" a year from now and going\nthrough your commit, what would he expect?). The first sentence could go\nbelow the ---.\n\nPlease, wrap your messages (less than 80 characters per line).\n\n> Similar Execution Time, but increased readability\n\nWhy capitalize Execution Time?\n\n> +\t\tif (origin){\n\nHere and below: space before {\n\n> +\t\t\tif(remote_is_branch)\n\nspace before (\n\n> +\t\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\telse\n> +\t\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\nAt this point, it would make sense to me to factor the printf_ln call\nlike\n\nconst char *msg;\nif (...)\n\tmsg = rebasing ? _(\"...\") : _(\"...\");\nelse\n\tmsg = rebasing ? _(\"...\") : _(\"...\");\nprintf_ln(msg, local, shortname);\n\n(but that's very subjective)\n\n> -\t\telse if (!remote_is_branch && !origin)\n> -\t\t\tprintf_ln(rebasing ?\n> +\t\t}\n> +\n> +\t\telse if (!origin){\n\nErr, isn't this the else branch of \"if (origin)\" ? If so, why repeat\n\"!origin\", and more specifically, isn't the next \"else\" branch dead\ncode:\n\n> +\t\t}\n> +\n>  \t\telse\n>  \t\t\tdie(\"BUG: impossible combination of %d and %p\",\n>  \t\t\t    remote_is_branch, origin);\n\nI mean: obviously, it has to be dead code, but it seems a bit strange to\nread\n\nif (x)\n\t...\nelse if (!x)\n\t...\nelse\n\tdie(...)\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"}]}