{"thread":{"id":"36129","subject":"[PATCH v4] install_branch_config: simplify verbose messages logic","startedAt":"2014-03-12T00:33:32Z","lastAt":"2014-03-13T22:45:06Z","messageCount":7,"participants":["Paweł Wawruch","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"236553","messageId":"1394584412-7735-1-git-send-email-pawlo@aleg.pl","threadId":"36129","inReplyTo":null,"subject":"[PATCH v4] install_branch_config: simplify verbose messages logic","fromName":"Paweł Wawruch","fromEmail":"pawlo@aleg.pl","sentAt":"2014-03-12T00:33:32Z","receivedAt":"2014-03-12T00:33:32Z","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 \n---\nThe changes proposed by Junio C Hamano:\nImprovement of indentations. Removed an unused variable.\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[3]: http://thread.gmane.org/gmane.comp.version-control.git/243865\n\n branch.c | 41 +++++++++++++++++++----------------------\n 1 file changed, 19 insertions(+), 22 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 723a36b..2a4b911 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -53,6 +53,20 @@ 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\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+\tconst char *name = remote_is_branch ? remote : shortname;\n \n \tif (remote_is_branch\n \t && !strcmp(local, shortname)\n@@ -77,29 +91,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(_(message[!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(_(message[!remote_is_branch][!origin][!rebasing]),\n+\t\t\t\tlocal, name);\n \t}\n }\n \n-- \n1.8.3.2\n"},{"id":"236559","messageId":"CAPig+cTmdF06PrWboJFNFEGKLCa=3rmGrcPusSsYZxdtfSEewA@mail.gmail.com","threadId":"36129","inReplyTo":"1394584412-7735-1-git-send-email-pawlo@aleg.pl","subject":"Re: [PATCH v4] install_branch_config: simplify verbose messages logic","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-12T05:36:57Z","receivedAt":"2014-03-12T05:36:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 11, 2014 at 8:33 PM, Paweł Wawruch wrote:\n> Replace the chain of if statements with table of strings.\n>\n> Signed-off-by: Paweł Wawruch \n> ---\n> The changes proposed by Junio C Hamano:\n> Improvement of indentations. Removed an unused variable.\n\nBetter, thanks. More below.\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> [3]: http://thread.gmane.org/gmane.comp.version-control.git/243865\n\nThe [n]: notation typically is used for footnotes which you reference\nin the running text as [n] (or sometimes [*n*], depending upon\npreference). If you're simply listing links to previous attempts, it\nwould be less confusing to say:\n\nv3: http://...\nv2: http://...\nv1: http://...\n\n> branch.c | 41 +++++++++++++++++++----------------------\n> 1 file changed, 19 insertions(+), 22 deletions(-)\n>\n> diff --git a/branch.c b/branch.c\n> index 723a36b..2a4b911 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -53,6 +53,20 @@ 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> + const char *message[][2][2] = {{{\n> + N_(\"Branch %s set up to track remote branch %s from %s by rebasing.\"),\n> + N_(\"Branch %s set up to track remote branch %s from %s.\"),\n> + },{\n> + N_(\"Branch %s set up to track local branch %s by rebasing.\"),\n> + N_(\"Branch %s set up to track local branch %s.\"),\n> + }},{{\n> + N_(\"Branch %s set up to track remote ref %s by rebasing.\"),\n> + N_(\"Branch %s set up to track remote ref %s.\"),\n> + },{\n> + N_(\"Branch %s set up to track local ref %s by rebasing.\"),\n> + N_(\"Branch %s set up to track local ref %s.\")\n> + }}};\n> + const char *name = remote_is_branch ? remote : shortname;\n>\n> if (remote_is_branch\n> && !strcmp(local, shortname)\n> @@ -77,29 +91,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> + if (origin && remote_is_branch)\n> + printf_ln(_(message[!remote_is_branch][!origin][!rebasing]),\n> + local, name, origin);\n> else\n> - die(\"BUG: impossible combination of %d and %p\",\n> - remote_is_branch, origin);\n> + printf_ln(_(message[!remote_is_branch][!origin][!rebasing]),\n> + local, name);\n\nShouldn't this logic also be encoded in the table? After all, the\npoint of making the code table-driven is so that such hard-coded logic\ncan be avoided. It shouldn't be difficult to do.\n\nThe same argument also applies to computation of the 'name' variable\nabove. It too can be pushed into the the table.\n\n> }\n> }\n>\n> --\n> 1.8.3.2\n"},{"id":"236642","messageId":"xmqqob1bjcti.fsf@gitster.dls.corp.google.com","threadId":"36129","inReplyTo":"CAPig+cTmdF06PrWboJFNFEGKLCa=3rmGrcPusSsYZxdtfSEewA@mail.gmail.com","subject":"Re: [PATCH v4] install_branch_config: simplify verbose messages logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-12T22:02:01Z","receivedAt":"2014-03-12T22:02:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> + if (origin && remote_is_branch)\n>> + printf_ln(_(message[!remote_is_branch][!origin][!rebasing]),\n>> + local, name, origin);\n>> else\n>> - die(\"BUG: impossible combination of %d and %p\",\n>> - remote_is_branch, origin);\n>> + printf_ln(_(message[!remote_is_branch][!origin][!rebasing]),\n>> + local, name);\n>\n> Shouldn't this logic also be encoded in the table? After all, the\n> point of making the code table-driven is so that such hard-coded logic\n> can be avoided. It shouldn't be difficult to do.\n\nHmph. Is it even necessary in the first place? Does it hurt if you\ngive more parameters than the number of placeholders in the format\nstring?\n"},{"id":"236646","messageId":"CAPig+cRviwyxDa3xNt6JgmRURFC-AbLRS8NnBQ4SAnm3g=Cc7g@mail.gmail.com","threadId":"36129","inReplyTo":"xmqqob1bjcti.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4] install_branch_config: simplify verbose messages logic","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-12T22:49:51Z","receivedAt":"2014-03-12T22:49:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 12, 2014 at 6:02 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>\n>>> + if (origin && remote_is_branch)\n>>> + printf_ln(_(message[!remote_is_branch][!origin][!rebasing]),\n>>> + local, name, origin);\n>>> else\n>>> - die(\"BUG: impossible combination of %d and %p\",\n>>> - remote_is_branch, origin);\n>>> + printf_ln(_(message[!remote_is_branch][!origin][!rebasing]),\n>>> + local, name);\n>>\n>> Shouldn't this logic also be encoded in the table? After all, the\n>> point of making the code table-driven is so that such hard-coded logic\n>> can be avoided. It shouldn't be difficult to do.\n>\n> Hmph. Is it even necessary in the first place? Does it hurt if you\n> give more parameters than the number of placeholders in the format\n> string?\n\nIt's not necessary, which is why my question was posed: as a clue. By\nasking GSoC applicants to think about how the logic can be\ntable-driven, it is hoped that they will arrive at the realization\nthat it is okay to pass in more parameters than placeholders. With\nthat idea in mind, the code can be simplified whether table-driven or\nnot.\n\n(I suspect Michael had this in mind when he composed this GSoC\nmicroproject and asked students to consider if it would make sense to\nmake the code table-driven.)\n"},{"id":"236680","messageId":"xmqqr466hrru.fsf@gitster.dls.corp.google.com","threadId":"36129","inReplyTo":"CAPig+cTmdF06PrWboJFNFEGKLCa=3rmGrcPusSsYZxdtfSEewA@mail.gmail.com","subject":"Re: [PATCH v4] install_branch_config: simplify verbose messages logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-13T18:34:13Z","receivedAt":"2014-03-13T18:34:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Shouldn't this logic [to decide what the printf arguments should\n> be] also be encoded in the table?\n> ...\n> The same argument also applies to computation of the 'name' variable\n> above. It too can be pushed into the the table.\n\nBecause the \"printf argument\" logic does not have to be in the\ntable, the same argument does not apply to the 'name' thing.\n\nAfter looking at the v5 patch, I do not think an extra two-element\narray to switch between remote vs shortname is making it any easier\nto read. I would have to say that personally I find that\n\n\tconst char *name[] = {remote, shortname};\n\t... long swath of code ...\n\tprintf_ln(... name[!remote_is_branch] ...);\n\nis a lot harder to read than:\n\n\tprintf_ln(... remote_is_branch ? shortname : branch ...);\n\nHTH, and thanks.\n"},{"id":"236687","messageId":"CAPig+cQU5__EPA2C3QvCPAT4od3Ux8kCgq-2H8nCddGdNqaG0A@mail.gmail.com","threadId":"36129","inReplyTo":"xmqqr466hrru.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4] install_branch_config: simplify verbose messages logic","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-13T20:35:31Z","receivedAt":"2014-03-13T20:35:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 13, 2014 at 2:34 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>\n>> Shouldn't this logic [to decide what the printf arguments should\n>> be] also be encoded in the table?\n>> ...\n>> The same argument also applies to computation of the 'name' variable\n>> above. It too can be pushed into the the table.\n>\n> Because the \"printf argument\" logic does not have to be in the\n> table, the same argument does not apply to the 'name' thing.\n>\n> After looking at the v5 patch, I do not think an extra two-element\n> array to switch between remote vs shortname is making it any easier\n> to read. I would have to say that personally I find that\n>\n> const char *name[] = {remote, shortname};\n> ... long swath of code ...\n> printf_ln(... name[!remote_is_branch] ...);\n>\n> is a lot harder to read than:\n>\n> printf_ln(... remote_is_branch ? shortname : branch ...);\n\nIndeed, that's a step backward, and is not what was asked. Merely\npushing data into tables does not make the logic table-driven\n(emphasis on *driven*). The GSoC microproject did not demand a\ntable-driven approach, but instead asked students if such an approach\nwould make sense. A more table-driven approach might look something\nlike this:\n\n struct M { const char *s; const char **a1; const char **a2; }\n message[][2][2] = {{{\n { \"Branch %s set ... %s ... %s\", &shortname, &origin },\n ...\n }},{{\n { \"Branch %s set ... %s\", &remote, NULL },\n ...\n }}};\n\n const struct M *m = message[!remote_is_branch][!origin][!rebasing];\n printf_ln(m->s, local, *m->a1, *m->a2);\n\nWhether this approach is more clear than the original code is a matter\nfor debate [1], however, it's obvious from the table which arguments\nbelong with each message, and the printf_ln() invocation does not\nrequire any logic. When moving only messages into a table, they become\ndisconnected from their arguments which makes reasoning about them a\nbit more difficult. The original code does not have this problem, nor\ndoes a table-driven approach.\n\n[1]: While ungainly, the original code may not be sufficiently bad to\nwarrant the extra complications of a table. A simple refactoring, such\nas [2], can make the code a bit easier to read without adding\ncomplexity.\n\n[2]: http://thread.gmane.org/gmane.comp.version-control.git/243704\n"},{"id":"236696","messageId":"CAPig+cS9n64boSSgoK-z=V9QFkvr=1Z2Db5KRrsyZNDthDE27A@mail.gmail.com","threadId":"36129","inReplyTo":"CAPig+cQU5__EPA2C3QvCPAT4od3Ux8kCgq-2H8nCddGdNqaG0A@mail.gmail.com","subject":"Re: [PATCH v4] install_branch_config: simplify verbose messages logic","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-13T22:45:06Z","receivedAt":"2014-03-13T22:45:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 13, 2014 at 4:35 PM, Eric Sunshine wrote:\n> A more table-driven approach might look something\n> like this:\n>\n> struct M { const char *s; const char **a1; const char **a2; }\n> message[][2][2] = {{{\n> { \"Branch %s set ... %s ... %s\", &shortname, &origin },\n> ...\n> }},{{\n> { \"Branch %s set ... %s\", &remote, NULL },\n> ...\n> }}};\n>\n> const struct M *m = message[!remote_is_branch][!origin][!rebasing];\n> printf_ln(m->s, local, *m->a1, *m->a2);\n\nOf course, using NULL in the table like that would crash when\ndereferenced: *m->a2. It was just an quick example typed on-the-fly.\nReal code would want to be more careful.\n"}]}