{"thread":{"id":"36121","subject":"[PATCH v3] install_branch_config: simplify verbose diagnostic logic","startedAt":"2014-03-11T13:26:52Z","lastAt":"2014-03-11T19:56:01Z","messageCount":2,"participants":["Paweł Wawruch","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"236504","messageId":"1394544412-28409-1-git-send-email-pawlo@aleg.pl","threadId":"36121","inReplyTo":null,"subject":"[PATCH v3] install_branch_config: simplify verbose diagnostic logic","fromName":"Paweł Wawruch","fromEmail":"pawlo@aleg.pl","sentAt":"2014-03-11T13:26:52Z","receivedAt":"2014-03-11T13:26:52Z","isPatch":true,"sender":{"key":"pawlo@aleg.pl","avatar":"https://gravatar.com/avatar/17564135d51785c0ca06c280fdee1c08c803d68c3227d9e91b78db28b69d3945?d=mp&s=160"},"body":"Replace the chain of if statements with table of strings.\n\nSigned-off-by: Paweł Wawruch <pawlo@aleg.pl>\n---\nI changed the commit message. Logic of table has changed. To make it more\nclear I added three dimensions of the table. \n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/243502\n[2]: http://thread.gmane.org/gmane.comp.version-control.git/243849\n\n branch.c | 42 ++++++++++++++++++++----------------------\n 1 file changed, 20 insertions(+), 22 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 723a36b..741551a 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -53,6 +53,21 @@ 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 *message[][2][2] = {{{\n+\t\t\tN_(\"Branch %s set up to track remote branch %s from %s by rebasing.\"),\n+\t\t\tN_(\"Branch %s set up to track remote branch %s from %s.\"),\n+\t\t},{\n+\t\t\tN_(\"Branch %s set up to track local branch %s by rebasing.\"),\n+\t\t\tN_(\"Branch %s set up to track local branch %s.\"),\n+\t\t}},{{\n+\t\t\tN_(\"Branch %s set up to track remote ref %s by rebasing.\"),\n+\t\t\tN_(\"Branch %s set up to track remote ref %s.\"),\n+\t\t},{\n+\t\t\tN_(\"Branch %s set up to track local ref %s by rebasing.\"),\n+\t\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 +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\tif (origin && remote_is_branch)\n+\t\t\tprintf_ln(_(messages[!remote_is_branch][!origin][!rebasing]),\n+\t\t\t\tlocal, 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[!remote_is_branch][!origin][!rebasing]),\n+\t\t\t\tlocal, name);\n \t}\n }\n \n-- \n1.8.3.2\n"},{"id":"236525","messageId":"xmqqiorkpl0u.fsf@gitster.dls.corp.google.com","threadId":"36121","inReplyTo":"1394544412-28409-1-git-send-email-pawlo@aleg.pl","subject":"Re: [PATCH v3] install_branch_config: simplify verbose diagnostic logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-11T19:56:01Z","receivedAt":"2014-03-11T19:56:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paweł Wawruch <pawlo@aleg.pl> writes:\n\n> Replace the chain of if statements with table of strings.\n>\n> Signed-off-by: Paweł Wawruch <pawlo@aleg.pl>\n> ---\n> I changed the commit message. Logic of table has changed. To make it more\n> clear I added three dimensions of the table. \n\nI am not sure if the message is \"diagnostic\"; it looks more like\nreminder text to me.\n\n\n> diff --git a/branch.c b/branch.c\n> index 723a36b..741551a 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -53,6 +53,21 @@ 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 *message[][2][2] = {{{\n> +\t\t\tN_(\"Branch %s set up to track remote branch %s from %s by rebasing.\"),\n> +\t\t\tN_(\"Branch %s set up to track remote branch %s from %s.\"),\n> +\t\t},{\n> +\t\t\tN_(\"Branch %s set up to track local branch %s by rebasing.\"),\n> +\t\t\tN_(\"Branch %s set up to track local branch %s.\"),\n> +\t\t}},{{\n> +\t\t\tN_(\"Branch %s set up to track remote ref %s by rebasing.\"),\n> +\t\t\tN_(\"Branch %s set up to track remote ref %s.\"),\n> +\t\t},{\n> +\t\t\tN_(\"Branch %s set up to track local ref %s by rebasing.\"),\n> +\t\t\tN_(\"Branch %s set up to track local ref %s.\")\n> +\t}}};\n\nI almost agree with the above use of a strange brace opening/closing\nconvention in order to reduce the indentation levels [*1*]\nbut then perhaps the above can be dedented even further?\n\n> +\tconst char *message[][2][2] = {{{\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\t}, {\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\t}}, {{\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\t}, {\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\n\n> +\tconst char *name = remote_is_branch ? remote : shortname;\n> +\tint message_number;\n\nDo you still need this variable after making it a multi-dimentional\narray?\n\n\n[Footnote]\n\n*1* i.e. otherwise we would need something like\n\n\tmessage[][][] = {\n\t\t\t\t{\n\t\t\t\t\t{\n                                        \t...,\n                                                ...\n\t\t\t\t\t},\n\t\t\t\t\t{\n                                        \t...,\n                                                ...\n\t\t\t\t\t},\n\t\t\t\t},\n                                ...\n\t\t\t};\n"}]}