{"thread":{"id":"36195","subject":"[PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()","startedAt":"2014-03-17T09:55:04Z","lastAt":"2014-03-24T12:22:45Z","messageCount":16,"participants":["Aleksey Mokhovikov","Matthieu Moy","Eric Sunshine","Junio C Hamano","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"236868","messageId":"1395050104-19152-1-git-send-email-moxobukob@gmail.com","threadId":"36195","inReplyTo":null,"subject":"[PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()","fromName":"Aleksey Mokhovikov","fromEmail":"moxobukob@gmail.com","sentAt":"2014-03-17T09:55:04Z","receivedAt":"2014-03-17T09:55:04Z","isPatch":true,"sender":{"key":"moxobukob@gmail.com","avatar":null},"body":"\nThis is a milliproject from git google summer of code page. The current code that selects the output message is quite easy to understand. So I tried to improve it by removing nested conditions and code duplication. The output string is generated by selecting the proper parts of the message and concatenating them the into one template string.  \n\n\n \nSigned-off-by: Aleksey Mokhovikov <moxobukob@gmail.com>\n---\n branch.c | 39 ++++++++++++++++-----------------------\n 1 file changed, 16 insertions(+), 23 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 723a36b..2ee353f 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -77,29 +77,22 @@ 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\tconst char *message_template_parts[] = {\n+\t\t\t\"Branch %s set up to track\",\n+\t\t\torigin ? \" remote\" : \" local\",\n+\t\t\tremote_is_branch ? \" branch %s\" : \" ref %s\",\n+\t\t\t(remote_is_branch && origin) ? \" from %s\" : \"\",\n+\t\t\trebasing ? \" by rebasing.\" : \".\"};\n+\t\tstruct strbuf message_template = STRBUF_INIT;\n+\t\tsize_t i = 0;\n+\t\t\n+\t\tfor (i = 0; i < sizeof(message_template_parts)/sizeof(const char *); ++i) {\n+\t\t\tstrbuf_addstr(&message_template, message_template_parts[i]);\n+\t\t}\n+\t\t\n+\t\tprintf_ln(_(message_template.buf), local, remote_is_branch ? shortname : remote, origin);\n+\t\t\n+\t\tstrbuf_release(&message_template);\n \t}\n }\n \n-- \n1.8.3.2\n"},{"id":"236869","messageId":"vpqd2hlrvo4.fsf@anie.imag.fr","threadId":"36195","inReplyTo":"1395050104-19152-1-git-send-email-moxobukob@gmail.com","subject":"Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-03-17T10:01:15Z","receivedAt":"2014-03-17T10:01:15Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Hi,\n\nAleksey Mokhovikov <moxobukob@gmail.com> writes:\n\n> This is a milliproject from git google summer of code page. The\n> current code that selects the output message is quite easy to\n> understand. So I tried to improve it by removing nested conditions and\n> code duplication. The output string is generated by selecting the\n> proper parts of the message and concatenating them the into one\n> template string.\n\nPlease, read the threads for other submissions for this microproject.\nMost remarks done there also apply for your case. See for example:\n\n  http://thread.gmane.org/gmane.comp.version-control.git/244210\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"236874","messageId":"CAPig+cQ_gvDWKkgN543TZtfGOqDkH0EYS39w+eEw8CdJO9Ux3Q@mail.gmail.com","threadId":"36195","inReplyTo":"1395050104-19152-1-git-send-email-moxobukob@gmail.com","subject":"Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-17T10:53:22Z","receivedAt":"2014-03-17T10:53:22Z","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 taste of the\nGit review process...\n\nOn Mon, Mar 17, 2014 at 5:55 AM, Aleksey Mokhovikov <moxobukob@gmail.com> wrote:\n> Subject: [GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()\n\nMentioning [GSoC] in the subject is indeed a good idea.\n\nThe subject should be concise. Try to keep it at 65-70 characters or\nless. More detailed information can be written following the subject\n(separated from the subject by a blank line).\n\nWrite in imperative tone: say \"replace X with Y\" rather than \"X is\nreplaced with Y\".\n\nMention the module or function you're touching.\n\nYou might say something like this:\n\n    Subject: install_branch_config: replace if-chain with string composition\n\n(But read below since that's not what you really want to do...)\n\n> This is a milliproject from git google summer of code page. The current code that selects the output message is quite easy to understand. So I tried to improve it by removing nested conditions and code duplication. The output string is generated by selecting the proper parts of the message and concatenating them the into one template string.\n\nWrap lines to 65-70 characters.\n\nI suspect you meant \"not quite easy\" rather than \"quite easy\".\n\nThis prose is almost pure email commentary. It doesn't really convey\nuseful information to a person reading the patch months or years from\nnow. Place commentary below the \"---\" line under your sign-off.\n\nMatthieu already pointed you at [1] which explains why this approach\nof composing the strings is not GNU gettext-friendly, so I'll review\nother aspects of the patch.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/244210\n\n> Signed-off-by: Aleksey Mokhovikov <moxobukob@gmail.com>\n> ---\n>  branch.c | 39 ++++++++++++++++-----------------------\n>  1 file changed, 16 insertions(+), 23 deletions(-)\n>\n> diff --git a/branch.c b/branch.c\n> index 723a36b..2ee353f 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -77,29 +77,22 @@ 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> +               const char *message_template_parts[] = {\n> +                       \"Branch %s set up to track\",\n> +                       origin ? \" remote\" : \" local\",\n> +                       remote_is_branch ? \" branch %s\" : \" ref %s\",\n> +                       (remote_is_branch && origin) ? \" from %s\" : \"\",\n> +                       rebasing ? \" by rebasing.\" : \".\"};\n\nFor portability, this project is still mostly restricted to C89, so\nthese non-constant C99 initializer expressions are probably a no-go.\n\n> +               struct strbuf message_template = STRBUF_INIT;\n> +               size_t i = 0;\n> +\n> +               for (i = 0; i < sizeof(message_template_parts)/sizeof(const char *); ++i) {\n\nYou can use the ARRAY_SIZE() macro instead of sizeof(...)/sizeof(...).\n\nOn this project, i++ is preferred when the context does not\nspecifically demand ++i.\n\n> +                       strbuf_addstr(&message_template, message_template_parts[i]);\n> +               }\n> +\n> +               printf_ln(_(message_template.buf), local, remote_is_branch ? shortname : remote, origin);\n> +\n> +               strbuf_release(&message_template);\n>         }\n>  }\n>\n> --\n> 1.8.3.2\n"},{"id":"236982","messageId":"loom.20140318T124348-742@post.gmane.org","threadId":"36195","inReplyTo":"1395050104-19152-1-git-send-email-moxobukob@gmail.com","subject":"Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()","fromName":"Aleksey Mokhovikov","fromEmail":"moxobukob@gmail.com","sentAt":"2014-03-18T12:22:06Z","receivedAt":"2014-03-18T12:22:06Z","isPatch":true,"sender":{"key":"moxobukob@gmail.com","avatar":null},"body":"\nThis patch replaces if chain that selects the message with\n2 dimensional array of format strings and arguments.\n\n\nSigned-off-by: Aleksey Mokhovikov <moxobukob@gmail.com>\n---\nThis patch is unrelated with previous one, but related to GSoC.\nSo I don't know if I should create new thread for this patch.\n\nCompare with original construction\nPros:\n1) Remove if chain.\n2) Single table contains all messages with arguments in one contruction.\n3) Less code duplication.\nCons:\n1) Harder to associate the case with message.\n2) Harder for compiler to warn the programmer about not\n   enough arguments for format string. \n3) Harder to add non-string argument to messages.\n\nIf !!(x) is out of the coding guide, then message_id construction\ncan be rewritten in several other ways.\nThe guideline tells that !!(x) is not welcome and should not be\nunless needed. But looks like this is normal place for it.\n\nP.S.\nThanks to comments I got, so\nI've read more GNU gettext manual to get info\nabout translation workflow. When I posted a first patch\nI've passed the same message format strings to gettext /*_()*/\nas in original, to save the context of the message. And they\nwill be translated if every possible string combination\nwill be marked separately for translation. Because of\nxgettext can extract string from source automatically,\nit ruins the idea to generate message from parts. Even if the\nexaclty same message format string is passed to gettext.\n\n branch.c | 53 ++++++++++++++++++++++++++++++-----------------------\n 1 file changed, 30 insertions(+), 23 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 723a36b..51a5faf 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -47,12 +47,32 @@ static int should_setup_rebase(const char *origin)\n \treturn 0;\n }\n \n+\t\n void install_branch_config(int flag, const char *local, const char *origin, \nconst char *remote)\n {\n \tconst char *shortname = remote + 11;\n \tint remote_is_branch = starts_with(remote, \"refs/heads/\");\n \tstruct strbuf key = STRBUF_INIT;\n \tint rebasing = should_setup_rebase(origin);\n+\tint message_id = (!!remote_is_branch << 2) | (!!origin << 1) | \n(!!rebasing);\n+\tconst char *message_table[][4] =\n+\t\t{{N_(\"Branch %s set up to track local ref %s.\"),\n+\t\t  local, remote},\n+\t\t {N_(\"Branch %s set up to track local ref %s by rebasing.\"),\n+\t\t  local, remote},\n+\t\t {N_(\"Branch %s set up to track remote ref %s.\"),\n+\t\t  local, remote},\n+\t\t {N_(\"Branch %s set up to track remote ref %s by \nrebasing.\"),\n+\t\t  local, remote},\n+\t\t {N_(\"Branch %s set up to track local branch %s.\"),\n+\t\t  local, shortname},\n+\t\t {N_(\"Branch %s set up to track local branch %s by \nrebasing.\"),\n+\t\t  local, shortname},\n+\t\t {N_(\"Branch %s set up to track remote branch %s from %s.\"),\n+\t\t  local, shortname, origin},\n+\t\t {N_(\"Branch %s set up to track remote branch %s from %s by \nrebasing.\"),\n+\t\t  local, shortname, origin}};\n+\tconst char **message = NULL;\n \n \tif (remote_is_branch\n \t    && !strcmp(local, shortname)\n@@ -68,7 +88,7 @@ void install_branch_config(int flag, const char *local, \nconst char *origin, cons\n \tstrbuf_reset(&key);\n \tstrbuf_addf(&key, \"branch.%s.merge\", local);\n \tgit_config_set(key.buf, remote);\n-\n+\t\n \tif (rebasing) {\n \t\tstrbuf_reset(&key);\n \t\tstrbuf_addf(&key, \"branch.%s.rebase\", local);\n@@ -77,29 +97,16 @@ 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 \n%s from %s by rebasing.\") :\n-\t\t\t\t  _(\"Branch %s set up to track remote branch \n%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 \n%s by rebasing.\") :\n-\t\t\t\t  _(\"Branch %s set up to track local branch \n%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 \nby rebasing.\") :\n-\t\t\t\t  _(\"Branch %s set up to track remote ref \n%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 \nby rebasing.\") :\n-\t\t\t\t  _(\"Branch %s set up to track local ref \n%s.\"),\n-\t\t\t\t  local, remote);\n+\t\tif ((0 <= message_id) && (message_id < \nARRAY_SIZE(message_table)))\n+\t\t\tmessage = message_table[message_id];\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\tdie(\"BUG: id is out of range:id=%d (is_branch=%d, \norigin=%p, rebasing=%d)\",\n+\t\t\t    message_id, remote_is_branch, origin, rebasing);\n+\n+\t\tif (!message || !message[0])\n+\t\t\tdie(\"BUG: message is NULL. Fix verbose message \ntable.\");\n+\n+\t\tprintf_ln(_(message[0]), message[1], message[2], \nmessage[3]);\n \t}\n }\n \n-- \n1.8.3.2\n"},{"id":"236986","messageId":"lg9l22$qto$1@ger.gmane.org","threadId":"36195","inReplyTo":"loom.20140318T124348-742@post.gmane.org","subject":"Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()","fromName":"Aleksey Mokhovikov","fromEmail":"moxobukob@gmail.com","sentAt":"2014-03-18T14:33:29Z","receivedAt":"2014-03-18T14:33:29Z","isPatch":true,"sender":{"key":"moxobukob@gmail.com","avatar":null},"body":"\nThis patch replaces if chain with\n2 dimensional array of format strings and arguments.\n\n\nSigned-off-by: Aleksey Mokhovikov <moxobukob@gmail.com>\n---\nThis patch is unrelated with previous one, but related to GSoC.\nSo I don't know if I should create new thread for this patch.\n\nCompare with original construction\nPros:\n1) Remove if chain.\n2) Single table contains all messages with arguments in one construction.\n3) Less code duplication.\nCons:\n1) Harder to associate the case with message.\n2) Harder for compiler to warn the programmer about not\n   enough arguments for format string.\n3) Harder to add non-string argument to messages.\n\nIf !!(x) is out of the coding guide, then message_id construction\ncan be rewritten in several other ways.\nThe guideline tells that !!(x) is not welcome and should not be\nunless needed. But looks like this is normal place for it.\n\nP.S.\nThanks to comments I got, so\nI've read more GNU gettext manual to get info\nabout translation workflow. When I posted a first patch\nI've passed the same message format strings to gettext /*_()*/\nas in original, to save the context of the message. And they\nwill be translated if every possible string combination\nwill be marked separately for translation. Because of\nxgettext can extract string from source automatically,\nit ruins the idea to generate message from parts. Even if the\nexaclty same message format string is passed to gettext.\n\n branch.c | 53 ++++++++++++++++++++++++++++++-----------------------\n 1 file changed, 30 insertions(+), 23 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 723a36b..51a5faf 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -47,12 +47,32 @@ static int should_setup_rebase(const char *origin)\n \treturn 0;\n }\n\n+\t\n void install_branch_config(int flag, const char *local, const char *origin, const char *remote)\n {\n \tconst char *shortname = remote + 11;\n \tint remote_is_branch = starts_with(remote, \"refs/heads/\");\n \tstruct strbuf key = STRBUF_INIT;\n \tint rebasing = should_setup_rebase(origin);\n+\tint message_id = (!!remote_is_branch << 2) | (!!origin << 1) | (!!rebasing);\n+\tconst char *message_table[][4] =\n+\t\t{{N_(\"Branch %s set up to track local ref %s.\"),\n+\t\t  local, remote},\n+\t\t {N_(\"Branch %s set up to track local ref %s by rebasing.\"),\n+\t\t  local, remote},\n+\t\t {N_(\"Branch %s set up to track remote ref %s.\"),\n+\t\t  local, remote},\n+\t\t {N_(\"Branch %s set up to track remote ref %s by rebasing.\"),\n+\t\t  local, remote},\n+\t\t {N_(\"Branch %s set up to track local branch %s.\"),\n+\t\t  local, shortname},\n+\t\t {N_(\"Branch %s set up to track local branch %s by rebasing.\"),\n+\t\t  local, shortname},\n+\t\t {N_(\"Branch %s set up to track remote branch %s from %s.\"),\n+\t\t  local, shortname, origin},\n+\t\t {N_(\"Branch %s set up to track remote branch %s from %s by rebasing.\"),\n+\t\t  local, shortname, origin}};\n+\tconst char **message = NULL;\n\n \tif (remote_is_branch\n \t    && !strcmp(local, shortname)\n@@ -68,7 +88,7 @@ void install_branch_config(int flag, const char *local, const char *origin, cons\n \tstrbuf_reset(&key);\n \tstrbuf_addf(&key, \"branch.%s.merge\", local);\n \tgit_config_set(key.buf, remote);\n-\n+\t\n \tif (rebasing) {\n \t\tstrbuf_reset(&key);\n \t\tstrbuf_addf(&key, \"branch.%s.rebase\", local);\n@@ -77,29 +97,16 @@ 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 ((0 <= message_id) && (message_id < ARRAY_SIZE(message_table)))\n+\t\t\tmessage = message_table[message_id];\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\tdie(\"BUG: id is out of range:id=%d (is_branch=%d, origin=%p, rebasing=%d)\",\n+\t\t\t    message_id, remote_is_branch, origin, rebasing);\n+\n+\t\tif (!message || !message[0])\n+\t\t\tdie(\"BUG: message is NULL. Fix verbose message table.\");\n+\n+\t\tprintf_ln(_(message[0]), message[1], message[2], message[3]);\n \t}\n }\n\n-- \n1.8.3.2\n"},{"id":"236987","messageId":"lg9l9t$uaa$1@ger.gmane.org","threadId":"36195","inReplyTo":"CAPig+cQ_gvDWKkgN543TZtfGOqDkH0EYS39w+eEw8CdJO9Ux3Q@mail.gmail.com","subject":"Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()","fromName":"Aleksey Mokhovikov","fromEmail":"moxobukob@gmail.com","sentAt":"2014-03-18T14:37:39Z","receivedAt":"2014-03-18T14:37:39Z","isPatch":true,"sender":{"key":"moxobukob@gmail.com","avatar":null},"body":"Eric Sunshine <sunshine <at> sunshineco.com> writes:\n\n> The subject should be concise. Try to keep it at 65-70 characters or\n> less. More detailed information can be written following the subject\n> (separated from the subject by a blank line).\n>\n> Write in imperative tone: say \"replace X with Y\" rather than \"X is\n> replaced with Y\".\n>\n> Mention the module or function you're touching.\n>\n> You might say something like this:\n>\n>     Subject: install_branch_config: replace if-chain with string composition\n> Wrap lines to 65-70 characters.\n>\n> This prose is almost pure email commentary. It doesn't really convey\n> useful information to a person reading the patch months or years from\n> now. Place commentary below the \"---\" line under your sign-off.\n\nThanks a lot for you language and message formatting style advices.\n\nI've make a new patch taking into account the GNU gettext requirements.\nI don't know if I should create a new thread for another patch, but\n\nI'd be glad if you will give me some information about new patch:\nhttp://permalink.gmane.org/gmane.comp.version-control.git/244357\n"},{"id":"236989","messageId":"lg9ljs$29p$1@ger.gmane.org","threadId":"36195","inReplyTo":"vpqd2hlrvo4.fsf@anie.imag.fr","subject":"Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()","fromName":"Aleksey Mokhovikov","fromEmail":"moxobukob@gmail.com","sentAt":"2014-03-18T14:42:58Z","receivedAt":"2014-03-18T14:42:58Z","isPatch":true,"sender":{"key":"moxobukob@gmail.com","avatar":null},"body":"Matthieu Moy <Matthieu.Moy <at> grenoble-inp.fr> writes:\n\n>\n> Hi,\n>\n> Aleksey Mokhovikov <moxobukob <at> gmail.com> writes:\n>\n> Please, read the threads for other submissions for this microproject.\n> Most remarks done there also apply for your case. See for example:\n>\n>   http://thread.gmane.org/gmane.comp.version-control.git/244210\n>\n\n\nThank you for your reply.\n\nI've read a bit more GNU gettext manual to get information\nabout translation with GNU gettext. Long story short, the idea to\ngenerate message from parts will produce even more problems, despite\nthe message strings passed to the _() are equal before and after the patch.\n\nSo I decided to make an array with all messages and mark them for translation with \"N_()\" to keep them together. Because\nwe now have an array we can improve it to make a table with message format string and arguments. Now we need just to\nfind the proper message index to print the message.\n\nPlease look at another approach:\nhttp://permalink.gmane.org/gmane.comp.version-control.git/244357\n"},{"id":"237051","messageId":"CAPig+cQLACyFwVypi08ZGQ14mpc0zt0fRRNhPzswRjsTaFQz2A@mail.gmail.com","threadId":"36195","inReplyTo":"lg9l22$qto$1@ger.gmane.org","subject":"Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-19T09:21:59Z","receivedAt":"2014-03-19T09:21:59Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Thanks for the resubmission. Comments below...\n\nOn Tue, Mar 18, 2014 at 10:33 AM, Aleksey Mokhovikov\n<moxobukob@gmail.com> wrote:\n> Subject: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()\n\nSay [PATCH v2] to indicate your second attempt.\n\nThis subject is quite long. Try to keep it short but informative.\nMention the module or function first. Use imperative voice. You might\nsay:\n\n    Subject: install_branch_config: replace if-chain with table lookup\n\n> This patch replaces if chain with\n> 2 dimensional array of format strings and arguments.\n\nThis _is_ a patch, so no need to say so explicitly. Use imperative\nvoice. You might say:\n\n    Replace if-chain, which selects message in verbose mode, with\n    table-driven approach.\n\n> Signed-off-by: Aleksey Mokhovikov <moxobukob@gmail.com>\n> ---\n> This patch is unrelated with previous one, but related to GSoC.\n> So I don't know if I should create new thread for this patch.\n\nKeeping it in the same thread provides continuity which can help\nreviewers. Providing a link to the previous attempt, like this [1], is\nalso courteous to reviewers.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/244237\n\n> Compare with original construction\n> Pros:\n> 1) Remove if chain.\n> 2) Single table contains all messages with arguments in one construction.\n> 3) Less code duplication.\n> Cons:\n> 1) Harder to associate the case with message.\n> 2) Harder for compiler to warn the programmer about not\n>    enough arguments for format string.\n> 3) Harder to add non-string argument to messages.\n\nNice summary. Do you draw any conclusions from it? Is the new code\nbetter? Easier to understand? Would it be better merely to refactor\nthe 'if' statements a bit instead of using a table?\n\n> If !!(x) is out of the coding guide, then message_id construction\n> can be rewritten in several other ways.\n> The guideline tells that !!(x) is not welcome and should not be\n> unless needed. But looks like this is normal place for it.\n\nCurious. !!x is indeed used regularly. Where did you read that it is\nnot welcome?\n\n> P.S.\n> Thanks to comments I got, so\n> I've read more GNU gettext manual to get info\n> about translation workflow. When I posted a first patch\n> I've passed the same message format strings to gettext /*_()*/\n> as in original, to save the context of the message. And they\n> will be translated if every possible string combination\n> will be marked separately for translation. Because of\n> xgettext can extract string from source automatically,\n> it ruins the idea to generate message from parts. Even if the\n> exaclty same message format string is passed to gettext.\n\nIndeed, it's a common and understandable misconception.\n\n>  branch.c | 53 ++++++++++++++++++++++++++++++-----------------------\n>  1 file changed, 30 insertions(+), 23 deletions(-)\n>\n> diff --git a/branch.c b/branch.c\n> index 723a36b..51a5faf 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -47,12 +47,32 @@ static int should_setup_rebase(const char *origin)\n>         return 0;\n>  }\n>\n> +\n\nUnnecessary blank line insertion. This adds noise to the patch which\ndistracts from the real changes.\n\n>  void install_branch_config(int flag, const char *local, const char *origin, const char *remote)\n>  {\n>         const char *shortname = remote + 11;\n>         int remote_is_branch = starts_with(remote, \"refs/heads/\");\n>         struct strbuf key = STRBUF_INIT;\n>         int rebasing = should_setup_rebase(origin);\n> +       int message_id = (!!remote_is_branch << 2) | (!!origin << 1) | (!!rebasing);\n\nIt works, but it's also a fairly magical incantation, placing greater\ncognitive demands on the reader. You could achieve a similar result\nwith a multi-dimensional table which is indexed directly by\n!!remote_is_branch, !!origin, and !!rebasing. (Whether you actually\nwant to do so is a different question.)\n\n> +       const char *message_table[][4] =\n> +               {{N_(\"Branch %s set up to track local ref %s.\"),\n> +                 local, remote},\n> +                {N_(\"Branch %s set up to track local ref %s by rebasing.\"),\n> +                 local, remote},\n> +                {N_(\"Branch %s set up to track remote ref %s.\"),\n> +                 local, remote},\n> +                {N_(\"Branch %s set up to track remote ref %s by rebasing.\"),\n> +                 local, remote},\n> +                {N_(\"Branch %s set up to track local branch %s.\"),\n> +                 local, shortname},\n> +                {N_(\"Branch %s set up to track local branch %s by rebasing.\"),\n> +                 local, shortname},\n> +                {N_(\"Branch %s set up to track remote branch %s from %s.\"),\n> +                 local, shortname, origin},\n> +                {N_(\"Branch %s set up to track remote branch %s from %s by rebasing.\"),\n> +                 local, shortname, origin}};\n\nIndeed, this is a reasonably decent way to keep the arguments and\nmessages together and can simplify the code nicely. Unfortunately,\nthis project is still restricted primarily to C89, so using\nnon-constant C99 initializers is likely to prevent the patch from\nbeing accepted.\n\n> +       const char **message = NULL;\n>\n>         if (remote_is_branch\n>             && !strcmp(local, shortname)\n> @@ -68,7 +88,7 @@ void install_branch_config(int flag, const char *local, const char *origin, cons\n>         strbuf_reset(&key);\n>         strbuf_addf(&key, \"branch.%s.merge\", local);\n>         git_config_set(key.buf, remote);\n> -\n> +\n\nUnnecessary whitespace change.\n\n>         if (rebasing) {\n>                 strbuf_reset(&key);\n>                 strbuf_addf(&key, \"branch.%s.rebase\", local);\n> @@ -77,29 +97,16 @@ 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 ((0 <= message_id) && (message_id < ARRAY_SIZE(message_table)))\n\nUnnecessary parentheses can make the code a bit more difficult to\nread, thus should be dropped.\n\n> +                       message = message_table[message_id];\n>                 else\n> -                       die(\"BUG: impossible combination of %d and %p\",\n> -                           remote_is_branch, origin);\n> +                       die(\"BUG: id is out of range:id=%d (is_branch=%d, origin=%p, rebasing=%d)\",\n> +                           message_id, remote_is_branch, origin, rebasing);\n\nAs this is indicates a programmer error, assert() may be more\nappropriate, and placed as near the computation of message_id as\npossible. The original code used die() because it didn't detect the\nerror until the end of the if-chain, but you can detect it as soon as\nyou compute message_id.\n\n> +               if (!message || !message[0])\n> +                       die(\"BUG: message is NULL. Fix verbose message table.\");\n\nMeh. May be overkill. If either of these is NULL, you'll get a crash\nanyhow, which is a good indication of a bug.\n\nOn the other hand, asserting that message_id is in range is a good\nidea since an out-of-bounds array access won't necessarily result in a\ncrash, thus such a bug could go undetected for some time.\n\n> +               printf_ln(_(message[0]), message[1], message[2], message[3]);\n>         }\n>  }\n>\n> --\n> 1.8.3.2\n"},{"id":"237166","messageId":"lgekju$u6t$1@ger.gmane.org","threadId":"36195","inReplyTo":"CAPig+cQLACyFwVypi08ZGQ14mpc0zt0fRRNhPzswRjsTaFQz2A@mail.gmail.com","subject":"Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()","fromName":"Aleksey Mokhovikov","fromEmail":"moxobukob@gmail.com","sentAt":"2014-03-20T11:56:38Z","receivedAt":"2014-03-20T11:56:38Z","isPatch":true,"sender":{"key":"moxobukob@gmail.com","avatar":null},"body":"On 03/19/2014 04:21 PM, Eric Sunshine wrote:\n> Thanks for the resubmission. Comments below...\n> \n> On Tue, Mar 18, 2014 at 10:33 AM, Aleksey Mokhovikov\n> <moxobukob@gmail.com> wrote:\n>> Subject: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()\n> \n> Say [PATCH v2] to indicate your second attempt.\n> \n> This subject is quite long. Try to keep it short but informative.\n> Mention the module or function first. Use imperative voice. You might\n> say:\n> \n>     Subject: install_branch_config: replace if-chain with table lookup\n\nThanks. This is my first experience with such newsgroups. I don't know explicitly how this mail-nntp newsgroups work\nunder the hood, so I was afraid, that if I'll change the subject, gmane will create a new thread instead of placing a\ncomment.\n\n>> Compare with original construction\n>> Pros:\n>> 1) Remove if chain.\n>> 2) Single table contains all messages with arguments in one construction.\n>> 3) Less code duplication.\n>> Cons:\n>> 1) Harder to associate the case with message.\n>> 2) Harder for compiler to warn the programmer about not\n>>    enough arguments for format string.\n>> 3) Harder to add non-string argument to messages.\n> \n> Nice summary. Do you draw any conclusions from it? Is the new code\n> better? Easier to understand? Would it be better merely to refactor\n> the 'if' statements a bit instead of using a table?\n\nI think if that code is heavily developed at this time, then if chain is better,\nbecause if construction is more simple and looks more flexible to major changes.\nAnd if there is no plans to make huge but minor changes,\nthen table driven approach looks better for me. So I would wrote the if chain at\nfirst, and later, If I had to change verbose message or something similar,\nI could rewrite it.\n\n\n>> If !!(x) is out of the coding guide, then message_id construction\n>> can be rewritten in several other ways.\n>> The guideline tells that !!(x) is not welcome and should not be\n>> unless needed. But looks like this is normal place for it.\n> \n> Curious. !!x is indeed used regularly. Where did you read that it is\n> not welcome?\n\nIn git/Documentation/CodingGuidelines:\n>   170   - Some clever tricks, like using the !! operator with arithmetic\n>   171     constructs, can be extremely confusing to others.  Avoid them,\n>   172     unless there is a compelling reason to use them.\n\n\n\n> Unnecessary blank line insertion. This adds noise to the patch which\n> distracts from the real changes.\n\n>>  void install_branch_config(int flag, const char *local, const char *origin, const char *remote)\n>>  {\n>>         const char *shortname = remote + 11;\n>>         int remote_is_branch = starts_with(remote, \"refs/heads/\");\n>>         struct strbuf key = STRBUF_INIT;\n>>         int rebasing = should_setup_rebase(origin);\n>> +       int message_id = (!!remote_is_branch << 2) | (!!origin << 1) | (!!rebasing);\n> \n> It works, but it's also a fairly magical incantation, placing greater\n> cognitive demands on the reader. You could achieve a similar result\n> with a multi-dimensional table which is indexed directly by\n> !!remote_is_branch, !!origin, and !!rebasing. (Whether you actually\n> want to do so is a different question.)\n\nI thought about a multidimensional table and about this approach before submitting a patch\nand it looks easier for me to read without multidimensional table. But I mentioned that indexing can be rewritten\nseveral ways. It will be even easier if the named function or macro is used:\n\n#define BOOL_TO_BITFLAG(x, shift) (!!(x) << (shift))\n...\nint message_id = BOOL_TO_BITFLAG(remote_is_branch, 2) | BOOL_TO_BITFLAG(origin, 1) | BOOL_TO_BITFLAG(rebasing, 0);\n\n\n>> +       const char *message_table[][4] =\n>> +               {{N_(\"Branch %s set up to track local ref %s.\"),\n>> +                 local, remote},\n>> +                {N_(\"Branch %s set up to track local ref %s by rebasing.\"),\n>> +                 local, remote},\n>> +                {N_(\"Branch %s set up to track remote ref %s.\"),\n>> +                 local, remote},\n>> +                {N_(\"Branch %s set up to track remote ref %s by rebasing.\"),\n>> +                 local, remote},\n>> +                {N_(\"Branch %s set up to track local branch %s.\"),\n>> +                 local, shortname},\n>> +                {N_(\"Branch %s set up to track local branch %s by rebasing.\"),\n>> +                 local, shortname},\n>> +                {N_(\"Branch %s set up to track remote branch %s from %s.\"),\n>> +                 local, shortname, origin},\n>> +                {N_(\"Branch %s set up to track remote branch %s from %s by rebasing.\"),\n>> +                 local, shortname, origin}};\n> \n> Indeed, this is a reasonably decent way to keep the arguments and\n> messages together and can simplify the code nicely. Unfortunately,\n> this project is still restricted primarily to C89, so using\n> non-constant C99 initializers is likely to prevent the patch from\n> being accepted.\n\nStrange. This is not a static variable. N_(x) is expanded to x - is just a marker for xgetext.\narray dimensions are known on compile time. Thought here will be no problems with standard.\n\n>> +                       message = message_table[message_id];\n>>                 else\n>> -                       die(\"BUG: impossible combination of %d and %p\",\n>> -                           remote_is_branch, origin);\n>> +                       die(\"BUG: id is out of range:id=%d (is_branch=%d, origin=%p, rebasing=%d)\",\n>> +                           message_id, remote_is_branch, origin, rebasing);\n> \n> As this is indicates a programmer error, assert() may be more\n> appropriate, and placed as near the computation of message_id as\n> possible. The original code used die() because it didn't detect the\n> error until the end of the if-chain, but you can detect it as soon as\n> you compute message_id.\n\nThanks. At first I wrote assert, but decided to leave die because if was there before.\n\n\n>> +               if (!message || !message[0])\n>> +                       die(\"BUG: message is NULL. Fix verbose message table.\");\n> \n> Meh. May be overkill. If either of these is NULL, you'll get a crash\n> anyhow, which is a good indication of a bug.\n\nIf either of them is NULL it can be UB. And if message[0] is NULL, it's quite likely. that\nprintf will just skip it, so I think it's better to assert message[0].\n"},{"id":"237258","messageId":"CAPig+cRv-WvoMVY0PDgtLawywe0QfpX8oPetzNwD-9Jq6BOjyw@mail.gmail.com","threadId":"36195","inReplyTo":"lgekju$u6t$1@ger.gmane.org","subject":"Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-21T04:03:28Z","receivedAt":"2014-03-21T04:03:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 20, 2014 at 7:56 AM, Aleksey Mokhovikov <moxobukob@gmail.com> wrote:\n> On 03/19/2014 04:21 PM, Eric Sunshine wrote:\n>> Thanks for the resubmission. Comments below...\n>>\n>> On Tue, Mar 18, 2014 at 10:33 AM, Aleksey Mokhovikov\n>> <moxobukob@gmail.com> wrote:\n>>> Subject: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()\n>>\n>> Say [PATCH v2] to indicate your second attempt.\n>>\n>> This subject is quite long. Try to keep it short but informative.\n>> Mention the module or function first. Use imperative voice. You might\n>> say:\n>>\n>>     Subject: install_branch_config: replace if-chain with table lookup\n>\n> Thanks. This is my first experience with such newsgroups. I don't know explicitly how this mail-nntp newsgroups work\n> under the hood, so I was afraid, that if I'll change the subject, gmane will create a new thread instead of placing a\n> comment.\n\nThe --in-reply-to option of \"git send-email\" may be useful. It's\nimportant to satisfy the requirements of patch correctness; and\nprobably not worth worrying about satisfying Gmane, or any of the\nother services archiving the mailing list, which are beyond our\ncontrol.\n\n>>> If !!(x) is out of the coding guide, then message_id construction\n>>> can be rewritten in several other ways.\n>>> The guideline tells that !!(x) is not welcome and should not be\n>>> unless needed. But looks like this is normal place for it.\n>>\n>> Curious. !!x is indeed used regularly. Where did you read that it is\n>> not welcome?\n>\n> In git/Documentation/CodingGuidelines:\n>>   170   - Some clever tricks, like using the !! operator with arithmetic\n>>   171     constructs, can be extremely confusing to others.  Avoid them,\n>>   172     unless there is a compelling reason to use them.\n\nInteresting. A quick grep shows that most !! are used on pointer\nvalues but some are used with arithmetic constructs (mentioned in the\nexcerpt). In the context of this type of patch where you need to\nrestrict those values to 0 or 1 before multiplying or shifting, !!\nmight qualify for the \"unless there is a compelling reason\" exception.\n\n>>>  void install_branch_config(int flag, const char *local, const char *origin, const char *remote)\n>>>  {\n>>>         const char *shortname = remote + 11;\n>>>         int remote_is_branch = starts_with(remote, \"refs/heads/\");\n>>>         struct strbuf key = STRBUF_INIT;\n>>>         int rebasing = should_setup_rebase(origin);\n>>> +       int message_id = (!!remote_is_branch << 2) | (!!origin << 1) | (!!rebasing);\n>>\n>> It works, but it's also a fairly magical incantation, placing greater\n>> cognitive demands on the reader. You could achieve a similar result\n>> with a multi-dimensional table which is indexed directly by\n>> !!remote_is_branch, !!origin, and !!rebasing. (Whether you actually\n>> want to do so is a different question.)\n>\n> I thought about a multidimensional table and about this approach before submitting a patch\n> and it looks easier for me to read without multidimensional table. But I mentioned that indexing can be rewritten\n> several ways. It will be even easier if the named function or macro is used:\n>\n> #define BOOL_TO_BITFLAG(x, shift) (!!(x) << (shift))\n> ...\n> int message_id = BOOL_TO_BITFLAG(remote_is_branch, 2) | BOOL_TO_BITFLAG(origin, 1) | BOOL_TO_BITFLAG(rebasing, 0);\n\nVerbosity doesn't necessarily equal clarity. (1 << n) is a standard\nidiom for setting the n'th bit, and should be well understood. The\nmacro usage also doesn't reduce the cognitive load since the reader\nstill has to puzzle out how 'bool' and 'bitflag' relate to table\npositions.\n\n(But that's only my opinion, and as part of the review process, you\nare welcome to present yours. And, as it's Junio's tree into which\npatches will be accepted -- or not -- he has final say-so.)\n\n>>> +       const char *message_table[][4] =\n>>> +               {{N_(\"Branch %s set up to track local ref %s.\"),\n>>> +                 local, remote},\n>>> +                {N_(\"Branch %s set up to track local ref %s by rebasing.\"),\n>>> +                 local, remote},\n>>> +                {N_(\"Branch %s set up to track remote ref %s.\"),\n>>> +                 local, remote},\n>>> +                {N_(\"Branch %s set up to track remote ref %s by rebasing.\"),\n>>> +                 local, remote},\n>>> +                {N_(\"Branch %s set up to track local branch %s.\"),\n>>> +                 local, shortname},\n>>> +                {N_(\"Branch %s set up to track local branch %s by rebasing.\"),\n>>> +                 local, shortname},\n>>> +                {N_(\"Branch %s set up to track remote branch %s from %s.\"),\n>>> +                 local, shortname, origin},\n>>> +                {N_(\"Branch %s set up to track remote branch %s from %s by rebasing.\"),\n>>> +                 local, shortname, origin}};\n>>\n>> Indeed, this is a reasonably decent way to keep the arguments and\n>> messages together and can simplify the code nicely. Unfortunately,\n>> this project is still restricted primarily to C89, so using\n>> non-constant C99 initializers is likely to prevent the patch from\n>> being accepted.\n>\n> Strange. This is not a static variable. N_(x) is expanded to x - is just a marker for xgetext.\n> array dimensions are known on compile time. Thought here will be no problems with standard.\n\nI was referring to the variables 'local', 'remote', 'shortname',\n'origin', all of which are, um, variable (non-constant).\n\n>>> +               if (!message || !message[0])\n>>> +                       die(\"BUG: message is NULL. Fix verbose message table.\");\n>>\n>> Meh. May be overkill. If either of these is NULL, you'll get a crash\n>> anyhow, which is a good indication of a bug.\n>\n> If either of them is NULL it can be UB. And if message[0] is NULL, it's quite likely. that\n> printf will just skip it, so I think it's better to assert message[0].\n\nSorry, you're right about message[0] case not being a crasher (though\nthe assert() still seems overkill).\n"},{"id":"237326","messageId":"xmqqr45vv5q8.fsf@gitster.dls.corp.google.com","threadId":"36195","inReplyTo":"CAPig+cRv-WvoMVY0PDgtLawywe0QfpX8oPetzNwD-9Jq6BOjyw@mail.gmail.com","subject":"Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-21T17:09:19Z","receivedAt":"2014-03-21T17:09:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> Sorry, you're right about message[0] case not being a crasher (though\n> the assert() still seems overkill).\n\nAssert() often becomes no-op in production build.  I think this may\nbe an indication that \"table-driven\" may not be as good an approach\nas many candidates thought.  The microproject suggestion asks them\nto think _if_ that makes sense, and it is perfectly fine for them if\nthey answer \"no, it introduces more problems than it solves\".\n"},{"id":"237372","messageId":"532CAB89.7030303@alum.mit.edu","threadId":"36195","inReplyTo":"xmqqr45vv5q8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-21T21:13:45Z","receivedAt":"2014-03-21T21:13:45Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/21/2014 06:09 PM, Junio C Hamano wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> \n>> Sorry, you're right about message[0] case not being a crasher (though\n>> the assert() still seems overkill).\n> \n> Assert() often becomes no-op in production build.  I think this may\n> be an indication that \"table-driven\" may not be as good an approach\n> as many candidates thought.  The microproject suggestion asks them\n> to think _if_ that makes sense, and it is perfectly fine for them if\n> they answer \"no, it introduces more problems than it solves\".\n\nMy expectation when I invented that microproject was that converting the\ncode to be table-driven would be judged *not* to be an improvement.  I\nwas hoping that a student would say \"the 'if' statement is OK, but let's\ndelete this ridiculous unreachable else branch\".  Possibly they would\nconvert the \"if\" chain into nested \"if\"s, which I think would allow some\ncode consolidation in one of the branches.\n\nBut not a single student agreed with me, so I must be in a minority of\none (which, unfortunately, is the definition of lunacy).\n\nThe multidimensional array lookup table is not so terrible, but I\npersonally still prefer the \"if\".\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"237376","messageId":"CAPig+cSYieZRA-kx0PUnrLjG5E6LYi_qOFqnZiE09wqeCvN3oA@mail.gmail.com","threadId":"36195","inReplyTo":"532CAB89.7030303@alum.mit.edu","subject":"Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-21T21:33:03Z","receivedAt":"2014-03-21T21:33:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 21, 2014 at 5:13 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> On 03/21/2014 06:09 PM, Junio C Hamano wrote:\n>> Eric Sunshine <sunshine@sunshineco.com> writes:\n>>\n>>> Sorry, you're right about message[0] case not being a crasher (though\n>>> the assert() still seems overkill).\n>>\n>> Assert() often becomes no-op in production build.  I think this may\n>> be an indication that \"table-driven\" may not be as good an approach\n>> as many candidates thought.  The microproject suggestion asks them\n>> to think _if_ that makes sense, and it is perfectly fine for them if\n>> they answer \"no, it introduces more problems than it solves\".\n>\n> My expectation when I invented that microproject was that converting the\n> code to be table-driven would be judged *not* to be an improvement.  I\n> was hoping that a student would say \"the 'if' statement is OK, but let's\n> delete this ridiculous unreachable else branch\".  Possibly they would\n> convert the \"if\" chain into nested \"if\"s, which I think would allow some\n> code consolidation in one of the branches.\n>\n> But not a single student agreed with me, so I must be in a minority of\n> one (which, unfortunately, is the definition of lunacy).\n\nAdam NoLastName did stick with the 'if' statements and removed the\nunreachable branch  [1], although he didn't say if he had considered\nthe table-driven approach and discarded it.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/243704\n\n> The multidimensional array lookup table is not so terrible, but I\n> personally still prefer the \"if\".\n>\n> Michael\n>\n> --\n> Michael Haggerty\n> mhagger@alum.mit.edu\n> http://softwareswirl.blogspot.com/\n"},{"id":"237379","messageId":"532CB315.50607@alum.mit.edu","threadId":"36195","inReplyTo":"CAPig+cSYieZRA-kx0PUnrLjG5E6LYi_qOFqnZiE09wqeCvN3oA@mail.gmail.com","subject":"Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-21T21:45:57Z","receivedAt":"2014-03-21T21:45:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/21/2014 10:33 PM, Eric Sunshine wrote:\n> On Fri, Mar 21, 2014 at 5:13 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>> On 03/21/2014 06:09 PM, Junio C Hamano wrote:\n>>> Assert() often becomes no-op in production build.  I think this may\n>>> be an indication that \"table-driven\" may not be as good an approach\n>>> as many candidates thought.  The microproject suggestion asks them\n>>> to think _if_ that makes sense, and it is perfectly fine for them if\n>>> they answer \"no, it introduces more problems than it solves\".\n>>\n>> My expectation when I invented that microproject was that converting the\n>> code to be table-driven would be judged *not* to be an improvement.  I\n>> was hoping that a student would say \"the 'if' statement is OK, but let's\n>> delete this ridiculous unreachable else branch\".  Possibly they would\n>> convert the \"if\" chain into nested \"if\"s, which I think would allow some\n>> code consolidation in one of the branches.\n>>\n>> But not a single student agreed with me, so I must be in a minority of\n>> one (which, unfortunately, is the definition of lunacy).\n> \n> Adam NoLastName did stick with the 'if' statements and removed the\n> unreachable branch  [1], although he didn't say if he had considered\n> the table-driven approach and discarded it.\n> \n> [1]: http://thread.gmane.org/gmane.comp.version-control.git/243704\n\nThanks for correcting my oversight.\n\nSo...there's still hope for my sanity after all :-)  Adam must have\ntasted from the tree of wisdom :-)\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"237433","messageId":"lgomc3$rh0$1@ger.gmane.org","threadId":"36195","inReplyTo":"532CAB89.7030303@alum.mit.edu","subject":"Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()","fromName":"Aleksey Mokhovikov","fromEmail":"moxobukob@gmail.com","sentAt":"2014-03-24T07:28:02Z","receivedAt":"2014-03-24T07:28:02Z","isPatch":true,"sender":{"key":"moxobukob@gmail.com","avatar":null},"body":"On 03/22/2014 04:13 AM, Michael Haggerty wrote:\n> My expectation when I invented that microproject was that converting the\n> code to be table-driven would be judged *not* to be an improvement.  I\n> was hoping that a student would say \"the 'if' statement is OK, but let's\n> delete this ridiculous unreachable else branch\".  Possibly they would\n> convert the \"if\" chain into nested \"if\"s, which I think would allow some\n> code consolidation in one of the branches.\n>\n> But not a single student agreed with me, so I must be in a minority of\n> one (which, unfortunately, is the definition of lunacy).\n>\n> The multidimensional array lookup table is not so terrible, but I\n> personally still prefer the \"if\".\n>\n> Michael\n>\n\nThat was expectable. But the main goal for me was to participate in git\ndevelopment process, to become familiar with it.\nIt looks hard to participate when not proposing a patch.\nI thought about make a small change in if chain, but it looked to minor\nto feel whole development process.\nI've used git features for formatting and sending a patch to mailing list.\nI've met the GNU gettext restrictions when proposed a first patch.\nProposed another patch and tried to show Pros and Cons.\nIt didn't look like applying a patch to git master branch was the main goal.\nAs for me that was quite interesting and useful.\n"},{"id":"237442","messageId":"53302395.6090207@alum.mit.edu","threadId":"36195","inReplyTo":"lgomc3$rh0$1@ger.gmane.org","subject":"Re: [PATCH][GSOC] Selection of the verbose message is replaced with generated message in install_branch_config()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T12:22:45Z","receivedAt":"2014-03-24T12:22:45Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/24/2014 08:28 AM, Aleksey Mokhovikov wrote:\n> On 03/22/2014 04:13 AM, Michael Haggerty wrote:\n>> My expectation when I invented that microproject was that converting the\n>> code to be table-driven would be judged *not* to be an improvement.  I\n>> was hoping that a student would say \"the 'if' statement is OK, but let's\n>> delete this ridiculous unreachable else branch\".  Possibly they would\n>> convert the \"if\" chain into nested \"if\"s, which I think would allow some\n>> code consolidation in one of the branches.\n>>\n>> But not a single student agreed with me, so I must be in a minority of\n>> one (which, unfortunately, is the definition of lunacy).\n>>\n>> The multidimensional array lookup table is not so terrible, but I\n>> personally still prefer the \"if\".\n> \n> That was expectable. But the main goal for me was to participate in git\n> development process, to become familiar with it.\n> It looks hard to participate when not proposing a patch.\n> I thought about make a small change in if chain, but it looked to minor\n> to feel whole development process.\n> I've used git features for formatting and sending a patch to mailing list.\n> I've met the GNU gettext restrictions when proposed a first patch.\n> Proposed another patch and tried to show Pros and Cons.\n> It didn't look like applying a patch to git master branch was the main goal.\n> As for me that was quite interesting and useful.\n\nSorry if there was a misunderstanding.  I didn't mean to criticize you\nand certainly not to single you out among the many students who went for\na table-driven solution.  I was rather replying to Junio's general\ncomment, that maybe changing the code to be table-driven wasn't such a\ngood idea.  These things are always a matter a taste, and sometimes hard\nto predict before one has tried writing the code a couple of different ways.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"}]}