{"thread":{"id":"36131","subject":"[PATCH] SoC 2014 MicroProject No.8:change multiple if-else statement to table-driven approach","startedAt":"2014-03-12T03:47:29Z","lastAt":"2014-03-17T20:39:34Z","messageCount":13,"participants":["Yao Zhao","Eric Sunshine","Junio C Hamano","Michael Haggerty","Quint Guvernator"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"236558","messageId":"1394596049-8767-1-git-send-email-zhaox383@umn.edu","threadId":"36131","inReplyTo":"zhaox383@umn.edu","subject":"[PATCH] SoC 2014 MicroProject No.8:change multiple if-else statement to table-driven approach","fromName":"Yao Zhao","fromEmail":"zhaox383@umn.edu","sentAt":"2014-03-12T03:47:29Z","receivedAt":"2014-03-12T03:47:29Z","isPatch":true,"sender":{"key":"zhaox383@umn.edu","avatar":null},"body":"Signed-off-by: Yao Zhao \n---\n branch.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 53 insertions(+), 2 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 723a36b..6432e27 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -53,7 +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-\n+\tchar** print_list = malloc(8 * sizeof(char*));\n+\tchar* arg1=NULL;\n+\tchar* arg2=NULL;\n+\tchar* arg3=NULL;\n+\tint index=0;\n+\n+\tprint_list[7] = _(\"Branch %s set up to track remote branch %s from %s by rebasing.\");\n+\tprint_list[6] = _(\"Branch %s set up to track remote branch %s from %s.\");\n+\tprint_list[5] = _(\"Branch %s set up to track local branch %s by rebasing.\");\n+\tprint_list[4] = _(\"Branch %s set up to track local branch %s.\");\n+\tprint_list[3] = _(\"Branch %s set up to track remote ref %s by rebasing.\");\n+\tprint_list[2] = _(\"Branch %s set up to track remote ref %s.\");\n+\tprint_list[1] = _(\"Branch %s set up to track local ref %s by rebasing.\");\n+\tprint_list[0] = _(\"Branch %s set up to track local ref %s.\");\n \tif (remote_is_branch\n \t && !strcmp(local, shortname)\n \t && !origin) {\n@@ -77,7 +90,44 @@ 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\tif(remote_is_branch)\n+\t\t\t\tindex += 4;\n+\t\tif(origin)\n+\t\t\t\tindex += 2;\n+\t\tif(rebasing)\n+\t\t\t\tindex += 1;\n+\t\t\n+\t\tif(index < 0 || index > 7)\n+\t\t{\n+\t\t\tdie(\"BUG: impossible combination of %d and %p\",\n+\t\t\t remote_is_branch, origin);\n+\t\t}\n+\n+\t\tif(index <= 4) {\n+\t\t\targ1 = local;\n+\t\t\targ2 = remote;\n+\t\t}\n+\t\telse if(index > 6) {\n+\t\t\targ1 = local;\n+\t\t\targ2 = shortname;\n+\t\t\targ3 = origin;\n+\t\t}\n+\t\telse {\n+\t\t\targ1 = local;\n+\t\t\targ2 = shortname;\n+\t\t}\n+\n+\t\tif(!arg3) {\n+\t\t\tprintf_ln(print_list[index],arg1,arg2);\n+\t\t}\n+\t\telse {\n+\t\t\tprintf_ln(print_list[index],arg1,arg2,arg3);\n+\t\t}\n+\n+\t\tfree(print_list);\n+\n+\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@@ -100,6 +150,7 @@ void install_branch_config(int flag, const char *local, const char *origin, cons\n \t\telse\n \t\t\tdie(\"BUG: impossible combination of %d and %p\",\n \t\t\t remote_is_branch, origin);\n+*/\n \t}\n }\n \n-- \n1.8.3.2\n"},{"id":"236567","messageId":"CAPig+cQu7D3AUghOSUOZBwf5+iHCPkxPbY1WuQmPJk1muCk7tQ@mail.gmail.com","threadId":"36131","inReplyTo":"1394596049-8767-1-git-send-email-zhaox383@umn.edu","subject":"Re: [PATCH] SoC 2014 MicroProject No.8:change multiple if-else statement to table-driven approach","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-12T09:21:06Z","receivedAt":"2014-03-12T09:21:06Z","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 Tue, Mar 11, 2014 at 11:47 PM, Yao Zhao wrote:\n> Subject: SoC 2014 MicroProject No.8:change multiple if-else statement to table-driven approach\n\nThe email subject is extracted automatically by \"git am\" as the first\nline of the patch's commit message so it should contain only text\nwhich is relevant to the commit message. In this case, everything\nbefore \"changes\" is merely commentary for readers of the email, and\nnot relevant to the commit message.\n\nIt is indeed a good idea to let reviewers know that this submission is\nfor GSoC, and you can indicate this as such:\n\n Subject: [PATCH GSoC] change multiple if-else statements to be table-driven\n\n> Signed-off-by: Yao Zhao \n> ---\n\nThe additional information that this is GSoC microproject #8 would go\nin the \"commentary\" area right here after the \"---\" following your\nsign-off.\n\n> branch.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++--\n> 1 file changed, 53 insertions(+), 2 deletions(-)\n\nThe patch is rife with style violations. I'll point out the first\ninstance of each violation, but do be sure to fix all remaining ones\nwhen you resubmit. See Documentation/CodingGuidelines for details.\n\n> diff --git a/branch.c b/branch.c\n> index 723a36b..6432e27 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -53,7 +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> -\n> + char** print_list = malloc(8 * sizeof(char*));\n\nStyle: char **print_list\n\nWhy allocate 'print_list' on the heap? An automatic variable 'char\nconst *print_list[]' would be more idiomatic and less likely to be\nleaked.\n\nIn fact, your heap-allocated 'print_list' _is_ being leaked a few\nlines down when the function returns early after warning that a branch\ncan not be its own upstream.\n\n> + char* arg1=NULL;\n> + char* arg2=NULL;\n> + char* arg3=NULL;\n\nStyle: char *var\nStyle: whitespace: var = NULL\n\n> + int index=0;\n> +\n> + print_list[7] = _(\"Branch %s set up to track remote branch %s from %s by rebasing.\");\n> + print_list[6] = _(\"Branch %s set up to track remote branch %s from %s.\");\n> + print_list[5] = _(\"Branch %s set up to track local branch %s by rebasing.\");\n> + print_list[4] = _(\"Branch %s set up to track local branch %s.\");\n> + print_list[3] = _(\"Branch %s set up to track remote ref %s by rebasing.\");\n> + print_list[2] = _(\"Branch %s set up to track remote ref %s.\");\n> + print_list[1] = _(\"Branch %s set up to track local ref %s by rebasing.\");\n> + print_list[0] = _(\"Branch %s set up to track local ref %s.\");\n\nIf you make print_list[] an automatic variable, then you can declare\nand populate it via a simple initializer. No need for this manual\napproach.\n\n> if (remote_is_branch\n> && !strcmp(local, shortname)\n> && !origin) {\n> @@ -77,7 +90,44 @@ 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> + if(remote_is_branch)\n\nStyle: whitespace: if (...)\n\n> + index += 4;\n> + if(origin)\n> + index += 2;\n> + if(rebasing)\n> + index += 1;\n> +\n> + if(index < 0 || index > 7)\n> + {\n> + die(\"BUG: impossible combination of %d and %p\",\n> + remote_is_branch, origin);\n> + }\n> +\n> + if(index <= 4) {\n> + arg1 = local;\n> + arg2 = remote;\n> + }\n> + else if(index > 6) {\n\nStyle: } else if (...) {\n\n> + arg1 = local;\n> + arg2 = shortname;\n> + arg3 = origin;\n> + }\n> + else {\n> + arg1 = local;\n> + arg2 = shortname;\n> + }\n> +\n> + if(!arg3) {\n> + printf_ln(print_list[index],arg1,arg2);\n\nStyle: whitespace: printf_ln(x, y, z)\n\n> + }\n> + else {\n> + printf_ln(print_list[index],arg1,arg2,arg3);\n> + }\n\nUnfortunately, this is quite a bit more verbose and complex than the\noriginal code, and all the magic numbers (4, 2, 1, 0, 7, 4, 6) place a\nhigher cognitive load on the reader, so this change probably is a net\nloss as far as clarity is concerned.\n\nTake a step back and consider again the GSoC miniproject: It talks\nabout making the code table-driven. Certainly, you have moved the\nstrings into a table, but all the complex logic is still in the code,\nand not in a table, hence it's not table-driven. To make this\ntable-driven, you should try to figure out how most or all of this\nlogic can be moved into a table.\n\n> + free(print_list);\n> +\n> +\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> @@ -100,6 +150,7 @@ void install_branch_config(int flag, const char *local, const char *origin, cons\n> else\n> die(\"BUG: impossible combination of %d and %p\",\n> remote_is_branch, origin);\n> +*/\n\nThe code you wrote is meant to replace the old code, so your patch\nshould actually remove the old code, not just comment it out.\n\n> }\n> }\n>\n> --\n> 1.8.3.2\n"},{"id":"236686","messageId":"1394742059-7300-1-git-send-email-zhaox383@umn.edu","threadId":"36131","inReplyTo":"CAPig+cQu7D3AUghOSUOZBwf5+iHCPkxPbY1WuQmPJk1muCk7tQ@mail.gmail.com","subject":"[PATCH] GSoC Change multiple if-else statements to be table-driven","fromName":"Yao Zhao","fromEmail":"zhaox383@umn.edu","sentAt":"2014-03-13T20:20:59Z","receivedAt":"2014-03-13T20:20:59Z","isPatch":true,"sender":{"key":"zhaox383@umn.edu","avatar":null},"body":"Signed-off-by: Yao Zhao \n---\nGSoC_MicroProject_#8\n\nHello Eric,\n\nThanks for reviewing my code. I implemented table-driven method this time and correct the style\nproblems indicated in review.\n\nThank you,\n\nYao\n\n branch.c | 72 ++++++++++++++++++++++++++++++++++++++++++++--------------------\n 1 file changed, 50 insertions(+), 22 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 723a36b..6451c99 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -49,10 +49,43 @@ static int should_setup_rebase(const char *origin)\n \n void install_branch_config(int flag, const char *local, const char *origin, const char *remote)\n {\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 size=8, i;\n+\n+\ttypedef struct PRINT_LIST {\n+\t\tconst char *print_str;\n+\t\tconst char *arg2; \n+\t\t//arg1 is always local, so I only add arg2 and arg3 in struct\n+\t\tconst char *arg3;\n+\t\tint b_rebasing;\n+\t int b_remote_is_branch;\n+\t\tint b_origin;\n+\t} PRINT_LIST;\n+\n+\tPRINT_LIST print_list[] = {\n+\t\t{.print_str = _(\"Branch %s set up to track remote branch %s from %s by rebasing.\"), \n+\t\t\t\t.arg2 = shortname, .arg3 = origin,\n+\t\t\t\t\t .b_rebasing = 1, .b_remote_is_branch = 1, .b_origin = 1},\n+\t {.print_str = _(\"Branch %s set up to track remote branch %s from %s.\"), \n+\t\t\t\t.arg2 = shortname, .arg3 = origin,\n+\t\t\t\t\t .b_rebasing = 0, .b_remote_is_branch = 1, .b_origin = 1},\n+ {.print_str = _(\"Branch %s set up to track local branch %s by rebasing.\"), \n+\t\t\t\t.arg2 = shortname, .b_rebasing = 1, .b_remote_is_branch = 1, .b_origin = 0},\n+\t\t{.print_str = _(\"Branch %s set up to track local branch %s.\"), \n+\t\t\t\t.arg2 = shortname, .b_rebasing = 0, .b_remote_is_branch = 1, .b_origin = 0},\n+\t\t{.print_str = _(\"Branch %s set up to track remote ref %s by rebasing.\"), \n+\t\t\t\t.arg2 = remote, .b_rebasing = 1, .b_remote_is_branch = 0, .b_origin = 1},\n+\t\t{.print_str = _(\"Branch %s set up to track remote ref %s.\"), \n+\t\t\t\t.arg2 = remote, .b_rebasing = 0, .b_remote_is_branch = 0, .b_origin = 1},\n+\t\t{.print_str = _(\"Branch %s set up to track local ref %s by rebasing.\"), \n+\t\t\t\t.arg2 = remote, .b_rebasing = 1, .b_remote_is_branch = 0, .b_origin = 0},\n+\t\t{.print_str = _(\"Branch %s set up to track local ref %s.\"), \n+\t\t\t\t.arg2 = remote, .b_rebasing = 0, .b_remote_is_branch = 0, .b_origin = 0},\n+};\nI am confused here: I use struct initializer and I am not sure if it's ok\nbecause it is only supported by ANSI \n\n \tif (remote_is_branch\n \t && !strcmp(local, shortname)\n@@ -75,31 +108,26 @@ void install_branch_config(int flag, const char *local, const char *origin, cons\n \t\tgit_config_set(key.buf, \"true\");\n \t}\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\tfor (i=0;i wrote:\n> Subject: [PATCH] GSoC Change multiple if-else statements to be table-driven\n\nIt's a good idea to let reviewers know that this is attempt 2. Do so\nby saying [PATCH v2]. Your next one will be [PATCH v3]. The -v option\nfor \"git format-email\" can help.\n\nWhen your patch is applied via \"git am\", text inside [...] gets\nstripped automatically. The \"GSoC\" tells email readers what this\nsubmission is about, but isn't relevant to the actual commit message.\nIt should be placed inside [...]. For instance: [PATCH/GSoC v2].\n\n> Signed-off-by: Yao Zhao \n> ---\n> GSoC_MicroProject_#8\n>\n> Hello Eric,\n>\n> Thanks for reviewing my code. I implemented table-driven method this time and correct the style\n> problems indicated in review.\n\nExplaining what you changed since the last version is indeed good\netiquette. Thanks. For bonus points, also provide a link to the\nprevious version, like this [1].\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/243919\n\n> branch.c | 72 ++++++++++++++++++++++++++++++++++++++++++++--------------------\n> 1 file changed, 50 insertions(+), 22 deletions(-)\n>\n> diff --git a/branch.c b/branch.c\n> index 723a36b..6451c99 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -49,10 +49,43 @@ static int should_setup_rebase(const char *origin)\n>\n> void install_branch_config(int flag, const char *local, const char *origin, const char *remote)\n> {\n> +\n\nUnnecessary insertion of blank line.\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 size=8, i;\n\nStyle: whitespace: size = 8\n\nYou can use ARRAY_SIZE(print_list) to avoid hardcoding 8 (and then you\ndon't need the variable 'size').\n\n> + typedef struct PRINT_LIST {\n> + const char *print_str;\n> + const char *arg2;\n> + //arg1 is always local, so I only add arg2 and arg3 in struct\n\nThis commentary should be placed below the \"---\" under your sign-off\n(or dropped altogether since it's pretty obvious).\n\nAlso, in this project avoid //-style comments.\n\n> + const char *arg3;\n> + int b_rebasing;\n> + int b_remote_is_branch;\n\nStrange indentation. Use tabs for indentation, and set your editor so\ntabs have width 8.\n\n> + int b_origin;\n> + } PRINT_LIST;\n\nRead below for some commentary about b_rebasing, b_remote_is_branch, b_origin.\n\n> + PRINT_LIST print_list[] = {\n> + {.print_str = _(\"Branch %s set up to track remote branch %s from %s by rebasing.\"),\n> + .arg2 = shortname, .arg3 = origin,\n> + .b_rebasing = 1, .b_remote_is_branch = 1, .b_origin = 1},\n> + {.print_str = _(\"Branch %s set up to track remote branch %s from %s.\"),\n> + .arg2 = shortname, .arg3 = origin,\n> + .b_rebasing = 0, .b_remote_is_branch = 1, .b_origin = 1},\n> + {.print_str = _(\"Branch %s set up to track local branch %s by rebasing.\"),\n> + .arg2 = shortname, .b_rebasing = 1, .b_remote_is_branch = 1, .b_origin = 0},\n> + {.print_str = _(\"Branch %s set up to track local branch %s.\"),\n> + .arg2 = shortname, .b_rebasing = 0, .b_remote_is_branch = 1, .b_origin = 0},\n> + {.print_str = _(\"Branch %s set up to track remote ref %s by rebasing.\"),\n> + .arg2 = remote, .b_rebasing = 1, .b_remote_is_branch = 0, .b_origin = 1},\n> + {.print_str = _(\"Branch %s set up to track remote ref %s.\"),\n> + .arg2 = remote, .b_rebasing = 0, .b_remote_is_branch = 0, .b_origin = 1},\n> + {.print_str = _(\"Branch %s set up to track local ref %s by rebasing.\"),\n> + .arg2 = remote, .b_rebasing = 1, .b_remote_is_branch = 0, .b_origin = 0},\n> + {.print_str = _(\"Branch %s set up to track local ref %s.\"),\n> + .arg2 = remote, .b_rebasing = 0, .b_remote_is_branch = 0, .b_origin = 0},\n> +};\n> I am confused here: I use struct initializer and I am not sure if it's ok\n> because it is only supported by ANSI\n\nThis commentary should be placed below \"---\" after your sign-off.\n\nIndeed, you want to avoid named field initializers in this project and\ninstead use positional initializers.\n\nTranslatable strings in an initializer should be wrapped with N_()\ninstead of _(). You will still need to use _() later on when you\nreference the string from the table. See section 4.7 [2] of the GNU\ngettext manual for details.\n\n[2]: http://www.gnu.org/software/gettext/manual/gettext.html#Special-cases\n\n> if (remote_is_branch\n> && !strcmp(local, shortname)\n> @@ -75,31 +108,26 @@ void install_branch_config(int flag, const char *local, const char *origin, cons\n> git_config_set(key.buf, \"true\");\n> }\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> + for (i=0;i + {\n> + if (print_list[i].b_rebasing == (rebasing? 1 : 0) &&\n> + print_list[i].b_remote_is_branch == (remote_is_branch? 1 : 0) &&\n> + print_list[i].b_origin == (origin? 1 : 0))\n\nStyle: whitespace: (x ? 1 : 0)\n\nAssigning &print_list[i] to a variable would allow you to reduce the\nnoise of this expression a bit.\n\nOn this project it is more idiomatic to say !!rebasing,\n!!remote_is_branch, !!origin to constrain these values to 0 or 1.\n\nAn alternate approach might be to use a multi-dimensional array, where\nthe boolean values of rebasing, remote_is_branch, and origin are keys\ninto the array. This would allow you to pick out the correct\nPRINT_LIST entry directly (no looping), thus eliminating the need for\nthose b_rebasing, b_remote_is_branch, and b_origin members.\n\n> + {\n> + if (print_list[i].arg3 == NULL)\n> + printf_ln (print_list[i].print_str, local, print_list[i].arg2);\n\nStyle: whitespace: printf_ln(...)\n\nReminder: wrap _() around print_list[i].print_str\n\n> + else\n> + printf_ln (print_list[i].print_str, local,\n> + print_list[i].arg2, print_list[i].arg3);\n\nThis logic can be simplified. Hint: It is not an error to pass more\narguments than there are %s's in the format string.\n\n> +\n> + break;\n> + }\n> + }\n> + if (i == size)\n> die(\"BUG: impossible combination of %d and %p\",\n> remote_is_branch, origin);\n> +\n\nUnnecessary insertion of blank line.\n\n> }\n> }\n>\n> --\n> 1.8.3.2\n"},{"id":"236732","messageId":"xmqqiorghgaj.fsf@gitster.dls.corp.google.com","threadId":"36131","inReplyTo":"CAPig+cRF_eQiGugR8TSks5ki375y-5wiQ7HWKyKRudJ5apd4cg@mail.gmail.com","subject":"Re: [PATCH] GSoC Change multiple if-else statements to be table-driven","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-14T16:54:28Z","receivedAt":"2014-03-14T16:54:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Thanks for the resubmission. Comments below.\n\nThanks, Eric, for helping so many micro exercises.\n\n> On Thu, Mar 13, 2014 at 4:20 PM, Yao Zhao wrote:\n>> Subject: [PATCH] GSoC Change multiple if-else statements to be table-driven\n>\n> It's a good idea to let reviewers know that this is attempt 2. Do so\n> by saying [PATCH v2]. Your next one will be [PATCH v3]. The -v option\n> for \"git format-email\" can help.\n\nYao, I think Eric meant \"git format-patch\".\n\n> When your patch is applied via \"git am\", text inside [...] gets\n> stripped automatically. The \"GSoC\" tells email readers what this\n> submission is about, but isn't relevant to the actual commit message.\n> It should be placed inside [...]. For instance: [PATCH/GSoC v2].\n\nSo in short,\n\n\tSubject: [PATCH/GSoC v2] branch.c: turn nested if-else logic to table-driven\n\nor something.\n\n>> + typedef struct PRINT_LIST {\n> ...\n>> + int b_origin;\n>> + } PRINT_LIST;\n\nWe do not do ALL_CAPS names and tend not to introduce one-off\ntypedefs for struct. Instead we would just use \"struct print_list\"\nthroughout (if we were to indeed use such a new struct, that is).\n\n>> + PRINT_LIST print_list[] = {\n>> + {.print_str = _(\"Branch %s set up to track remote branch %s from %s by rebasing.\"),\n>> + .arg2 = shortname, .arg3 = origin,\n>> + .b_rebasing = 1, .b_remote_is_branch = 1, .b_origin = 1},\n\n>> I am confused here: I use struct initializer and I am not sure if it's ok\n>> because it is only supported by ANSI\n> ...\n> Indeed, you want to avoid named field initializers in this project and\n> instead use positional initializers.\n\nCorrect.\n\n> Translatable strings in an initializer should be wrapped with N_()\n> instead of _(). You will still need to use _() later on when you\n> reference the string from the table. See section 4.7 [2] of the GNU\n> gettext manual for details.\n\nCorrect.\n\n> An alternate approach might be to use a multi-dimensional array,\n> where the boolean values of rebasing, remote_is_branch, and origin\n> are keys into the array. This would allow you to pick out the\n> correct PRINT_LIST entry directly (no looping), thus eliminating\n> the need for those b_rebasing, b_remote_is_branch, and b_origin\n> members.\n\nCorrect.\n\nAfter seeing so many \"table driven\" submissions, I however tend to\nagree with your earlier comment on another thread on this same\nmicro, where you said an nested if-else cascade that was rewritten\nin a clearer way (sorry, I do not remember whose submission it was\noffhand) may be the best answer to the \"Would it make sense to make\nthe code table-driven?\" question, even though I tentatively queued\nd7ea7894 (install_branch_config(): simplify verbose messages logic,\n2014-03-13) from Paweł on 'pu'.\n\nThanks for a review.\n"},{"id":"236816","messageId":"1394950103-2264-1-git-send-email-zhaox383@umn.edu","threadId":"36131","inReplyTo":"CAPig+cRF_eQiGugR8TSks5ki375y-5wiQ7HWKyKRudJ5apd4cg@mail.gmail.com","subject":"[PATCH/GSoC_v3] branch.c: turn nested if-else logic to table-driven","fromName":"Yao Zhao","fromEmail":"zhaox383@umn.edu","sentAt":"2014-03-16T06:08:23Z","receivedAt":"2014-03-16T06:08:23Z","isPatch":true,"sender":{"key":"zhaox383@umn.edu","avatar":null},"body":"Signed-off-by: Yao Zhao \n---\n branch.c | 53 +++++++++++++++++++++++++++++------------------------\n 1 file changed, 29 insertions(+), 24 deletions(-)\nHello Eric,\n\nThank you and Junio for reviewing my code. It is really helpful to improve my code quality.\n\nThis is version 3 of patch. Previous address : http://thread.gmane.org/gmane.comp.version-control.git/243919. I do not use positional initializer because it is not allowed to use variable in it. I don't know if it's ok to use this redundant way to initialize \"list\".\n\nI cannot find -v flag in documentation you indicated in last email so I use set-prefix to add it into prefix.\n\t\nNow I am working on writing proposal for git project. I am really interested in last one, about improve git_config. I know it's important to get known about git_config first and have read documentation about it. But I am really confused about how to understand code of git_config. When user type in git config in terminal, what is the execute order of functions? How git config influence other git command? Does program read config file every time when they execuate config-related command?\n\nThank you,\n\nYao\n\ndiff --git a/branch.c b/branch.c\nindex 723a36b..1df30c7 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -53,7 +53,33 @@ 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-\n+\tstruct print_list {\n+\t\tconst char *print_str;\n+\t\tconst char *arg2; \n+\t\tconst char *arg3;\n+\t} ; \n+\tstruct print_list target;\n+\n+\tstruct print_list list[2][2][2];\n+\tlist[0][0][0].print_str = N_(\"Branch %s set up to track local ref %s.\");\n+\tlist[0][0][0].arg2 = remote;\n+\tlist[0][0][1].print_str = N_(\"Branch %s set up to track local ref %s by rebasing.\");\n+\tlist[0][0][1].arg2 = remote;\n+\tlist[0][1][0].print_str = N_(\"Branch %s set up to track remote ref %s.\");\n+\tlist[0][1][0].arg2 = remote;\n+\tlist[0][1][1].print_str = N_(\"Branch %s set up to track remote ref %s by rebasing.\");\n+\tlist[0][1][1].arg2 = remote;\n+\tlist[1][0][0].print_str = N_(\"Branch %s set up to track local branch %s.\");\n+\tlist[1][0][0].arg2 =shortname;\n+\tlist[1][0][1].print_str = N_(\"Branch %s set up to track local branch %s by rebasing.\");\n+\tlist[1][0][1].arg2 = shortname;\n+\tlist[1][1][0].print_str = N_(\"Branch %s set up to track remote branch %s from %s.\");\n+\tlist[1][1][0].arg2 = shortname;\n+\tlist[1][1][0].arg3 = origin;\n+\tlist[1][1][1].print_str = N_(\"Branch %s set up to track remote branch %s from %s by rebasing.\");\n+\tlist[1][1][1].arg2 = shortname;\n+\tlist[1][1][1].arg3 = origin;\n+ \n \tif (remote_is_branch\n \t && !strcmp(local, shortname)\n \t && !origin) {\n@@ -77,29 +103,8 @@ 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\t\ttarget = list[!!remote_is_branch][!!origin][!!rebasing];\n+\t\t\tprintf_ln (_(target.print_str), local, target.arg2, target.arg3);\n \t}\n }\n \n-- \n1.8.3.2\n"},{"id":"236852","messageId":"CAPig+cSG0YsdTGWWyxWrg+s235qDe7Xr-jXT2mOkpvA6OaGWyg@mail.gmail.com","threadId":"36131","inReplyTo":"xmqqiorghgaj.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] GSoC Change multiple if-else statements to be table-driven","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-17T06:29:10Z","receivedAt":"2014-03-17T06:29:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 14, 2014 at 12:54 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>\n>> On Thu, Mar 13, 2014 at 4:20 PM, Yao Zhao wrote:\n>>> Subject: [PATCH] GSoC Change multiple if-else statements to be table-driven\n>>\n>> It's a good idea to let reviewers know that this is attempt 2. Do so\n>> by saying [PATCH v2]. Your next one will be [PATCH v3]. The -v option\n>> for \"git format-email\" can help.\n>\n> Yao, I think Eric meant \"git format-patch\".\n\nCorrect. Sorry for the confusion.\n\n>> An alternate approach might be to use a multi-dimensional array,\n>> where the boolean values of rebasing, remote_is_branch, and origin\n>> are keys into the array. This would allow you to pick out the\n>> correct PRINT_LIST entry directly (no looping), thus eliminating\n>> the need for those b_rebasing, b_remote_is_branch, and b_origin\n>> members.\n>\n> Correct.\n>\n> After seeing so many \"table driven\" submissions, I however tend to\n> agree with your earlier comment on another thread on this same\n> micro, where you said an nested if-else cascade that was rewritten\n> in a clearer way (sorry, I do not remember whose submission it was\n\nIt was Adam NoLastName [1].\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/243704\n\n> offhand) may be the best answer to the \"Would it make sense to make\n> the code table-driven?\" question, even though I tentatively queued\n> d7ea7894 (install_branch_config(): simplify verbose messages logic,\n> 2014-03-13) from Paweł on 'pu'.\n\nPerhaps it is time to mark this microproject as \"taken\" on the GSoC\npage [2], along a fews others for which we have received multiple\nsubmissions.\n\n[2]: https://github.com/git/git.github.io/blob/master/SoC-2014-Microprojects.md\n"},{"id":"236858","messageId":"7v38ihuvq7.fsf@alter.siamese.dyndns.org","threadId":"36131","inReplyTo":"CAPig+cSG0YsdTGWWyxWrg+s235qDe7Xr-jXT2mOkpvA6OaGWyg@mail.gmail.com","subject":"Re: [PATCH] GSoC Change multiple if-else statements to be table-driven","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-17T07:31:44Z","receivedAt":"2014-03-17T07:31:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Perhaps it is time to mark this microproject as \"taken\" on the GSoC\n> page [2], along a fews others for which we have received multiple\n> submissions.\n>\n> [2]: https://github.com/git/git.github.io/blob/master/SoC-2014-Microprojects.md\n\nI actually have been of multiple minds on this.\n\n * After seeing that many candidates tried to solve the same micro,\n apparently without checking answers by other people, and seeing\n how they responded to the reviews given to them, I found that we\n had as equally good interactions with them to judge their skills,\n both techincal and social, as we would have had if each of them\n solved different micros.\n\n * Many reviewers may have gotten tired of seeing many novice\n attempts on the same micro over and over again, giving gentle\n suggestions for improvements. Because the _sole_ purpose of these\n micros were to help candidates get their toes wet in the process,\n duplicated efforts on the candidates' side are not wasted---they\n each hopefully learned how to interact with this community.\n\n But it is true that, if we were wishing to also get some trivial\n clean-ups in the codebase as a side effect of these micros, we\n have wasted reviewer bandwidth by not having enough micros, and\n reviewers may have had some feeling that their efforts did not\n fully contribute to improving our codebase, which may have been\n discouraging.\n\n Big thanks go to all reviewers who participated despite this. If\n we had more micros, the apparent wastage of the reviewer efforts\n might have been avoided.\n\n * Many candidates did not even bother to check if others are\n working on the same micro, however, which would be a bad sign by\n itself. Concentrating on one's own topic, without paying any\n attention to what others are working on the same software, is\n never a good discipline.\n\n Some may argue that it would be unfair to blame the candidates on\n this---they may have picked up micros that haven't been solved if\n we had more---but after seeing that many candidates' submissions\n apparently did not take into account the reviews given to others'\n submissions on the same micro and/or had many exactly the same\n issues like log message styles as submissions on other micros\n that have already been reviewed, I personally do not think they\n are blameless.\n\nSo in short, yes it would have been nicer if we had more micros than\ncandidates, but I do not think it was detrimental for the purpose of\nthese micro exercises that multiple candidates ended up attempting\nthe same micro.\n\nAgain, Big Thanks to Michael for the excellent \"micro\" idea, and all\nreviewers who participated.\n"},{"id":"236861","messageId":"CAPig+cRWAr+p=npTh-MKpNy+A6wjLBtN9JB=u=UtVudGLU6TuA@mail.gmail.com","threadId":"36131","inReplyTo":"1394950103-2264-1-git-send-email-zhaox383@umn.edu","subject":"Re: [PATCH/GSoC_v3] branch.c: turn nested if-else logic to table-driven","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-17T07:54:40Z","receivedAt":"2014-03-17T07:54:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 16, 2014 at 2:08 AM, Yao Zhao wrote:\n> Signed-off-by: Yao Zhao \n> ---\n> branch.c | 53 +++++++++++++++++++++++++++++------------------------\n> 1 file changed, 29 insertions(+), 24 deletions(-)\n> Hello Eric,\n>\n> Thank you and Junio for reviewing my code. It is really helpful to improve my code quality.\n\nWrap commit message (and preferably commentary) to 65-70 characters.\n\n> This is version 3 of patch. Previous address : http://thread.gmane.org/gmane.comp.version-control.git/243919. I do not use positional initializer because it is not allowed to use variable in it. I don't know if it's ok to use this redundant way to initialize \"list\".\n\nThanks for the resubmission. A few comments below, but I don't think\nyou need to resubmit. What is important is that you have had a taste\nof the review process on this project, and the GSoC mentors have had a\nchance to observe your abilities and reviewer interaction. A better\nuse of your time now would be to make your GSoC proposal and converse\nwith the mentors.\n\n> I cannot find -v flag in documentation you indicated in last email so I use set-prefix to add it into prefix.\n\nSorry for the confusion. As Junio pointed out [1], I meant the -v\noption of \"git format-patch\", not \"git format-email\" (which doesn't\nexist).\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/243919/focus=244095\n\n> Now I am working on writing proposal for git project. I am really interested in last one, about improve git_config. I know it's important to get known about git_config first and have read documentation about it. But I am really confused about how to understand code of git_config. When user type in git config in terminal, what is the execute order of functions? How git config influence other git command? Does program read config file every time when they execuate config-related command?\n\nYou will get a better response if you ask these questions in a new\nemail thread rather than re-using this one. Choose a subject for your\nemail which indicates clearly that you have questions about that\nparticular GSoC project, address it to the git mailing list, and cc:\nthe mentors of that project.\n\n> Thank you,\n>\n> Yao\n>\n> diff --git a/branch.c b/branch.c\n> index 723a36b..1df30c7 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -53,7 +53,33 @@ 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> -\n> + struct print_list {\n> + const char *print_str;\n> + const char *arg2;\n> + const char *arg3;\n> + } ;\n> + struct print_list target;\n\nThis should be 'const struct print_list *target'. There is no need to\ncopy the entire structure (below) just to access its members.\n\n> + struct print_list list[2][2][2];\n> + list[0][0][0].print_str = N_(\"Branch %s set up to track local ref %s.\");\n> + list[0][0][0].arg2 = remote;\n> + list[0][0][1].print_str = N_(\"Branch %s set up to track local ref %s by rebasing.\");\n> + list[0][0][1].arg2 = remote;\n> + list[0][1][0].print_str = N_(\"Branch %s set up to track remote ref %s.\");\n> + list[0][1][0].arg2 = remote;\n> + list[0][1][1].print_str = N_(\"Branch %s set up to track remote ref %s by rebasing.\");\n> + list[0][1][1].arg2 = remote;\n> + list[1][0][0].print_str = N_(\"Branch %s set up to track local branch %s.\");\n> + list[1][0][0].arg2 =shortname;\n> + list[1][0][1].print_str = N_(\"Branch %s set up to track local branch %s by rebasing.\");\n> + list[1][0][1].arg2 = shortname;\n> + list[1][1][0].print_str = N_(\"Branch %s set up to track remote branch %s from %s.\");\n> + list[1][1][0].arg2 = shortname;\n> + list[1][1][0].arg3 = origin;\n> + list[1][1][1].print_str = N_(\"Branch %s set up to track remote branch %s from %s by rebasing.\");\n> + list[1][1][1].arg2 = shortname;\n> + list[1][1][1].arg3 = origin;\n\nSince this is not in an initializer table, you would normally use _()\nrather than N_() (and you don't have to use _() in the printf_ln()\ninvocation).\n\nIt disturbs me to see that .arg3 does not get initialized in most of\nthese cases, yet it is accessed below by printf_ln(). Code analysis\ntools are likely to complain about accessing an uninitialized value.\n\nOne way you could put this into the initializer would be to use a\nconstant expression to indicate which variable to pass to printf_ln().\nSomething like this:\n\n struct print_list {\n const char *print_str;\n int use_shortname;\n int use_origin;\n } print_list[][2][2] = {\n N_(\"Branch %s ...\"), 0, 0,\n ...\n N_(\"Branch %s ...\"), 1, 0,\n ...\n }\n\n printf_ln(_(target->print_str), local,\n target->use_shortname ? shortname : remote,\n target->use_origin ? origin : NULL);\n\nTo make it clearer, use named constants rather than 0 and 1.\n\nNOTE: I am *not* suggesting that you resubmit with this change. It's\ngetting ugly and bordering on being too complex and difficult to read.\nAs noted above, your time is probably better spent working on your\nGSoC proposal.\n\n> if (remote_is_branch\n> && !strcmp(local, shortname)\n> && !origin) {\n> @@ -77,29 +103,8 @@ 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> + target = list[!!remote_is_branch][!!origin][!!rebasing];\n\nWith the suggestion above of making 'target' a pointer to the struct,\nthis would become:\n\n target = &list[...][...][...];\n\n> + printf_ln (_(target.print_str), local, target.arg2, target.arg3);\n\nStyle: whitespace: printf_ln(...)\n\nAnd this would become:\n\n printf_ln(_(target->print_str), local, target->arg2, target->arg3);\n\n> }\n> }\n>\n> --\n> 1.8.3.2\n"},{"id":"236883","messageId":"5327027D.6090500@alum.mit.edu","threadId":"36131","inReplyTo":"7v38ihuvq7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] GSoC Change multiple if-else statements to be table-driven","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-17T14:11:09Z","receivedAt":"2014-03-17T14:11:09Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/17/2014 08:31 AM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n> \n>> Perhaps it is time to mark this microproject as \"taken\" on the GSoC\n>> page [2], along a fews others for which we have received multiple\n>> submissions.\n\nI just marked #8 as taken, as it's been beaten to death.\n\nI haven't been keeping careful track of which other microprojects have\nbeen overused. If you have suggestions for the chopping block, let me know.\n\n>> [2]: https://github.com/git/git.github.io/blob/master/SoC-2014-Microprojects.md\n> \n> I actually have been of multiple minds on this.\n> \n> * After seeing that many candidates tried to solve the same micro,\n> apparently without checking answers by other people, and seeing\n> how they responded to the reviews given to them, I found that we\n> had as equally good interactions with them to judge their skills,\n> both techincal and social, as we would have had if each of them\n> solved different micros.\n> \n> * Many reviewers may have gotten tired of seeing many novice\n> attempts on the same micro over and over again, giving gentle\n> suggestions for improvements. Because the _sole_ purpose of these\n> micros were to help candidates get their toes wet in the process,\n> duplicated efforts on the candidates' side are not wasted---they\n> each hopefully learned how to interact with this community.\n> \n> But it is true that, if we were wishing to also get some trivial\n> clean-ups in the codebase as a side effect of these micros, we\n> have wasted reviewer bandwidth by not having enough micros, and\n> reviewers may have had some feeling that their efforts did not\n> fully contribute to improving our codebase, which may have been\n> discouraging.\n> \n> Big thanks go to all reviewers who participated despite this. If\n> we had more micros, the apparent wastage of the reviewer efforts\n> might have been avoided.\n> \n> * Many candidates did not even bother to check if others are\n> working on the same micro, however, which would be a bad sign by\n> itself. Concentrating on one's own topic, without paying any\n> attention to what others are working on the same software, is\n> never a good discipline.\n> \n> Some may argue that it would be unfair to blame the candidates on\n> this---they may have picked up micros that haven't been solved if\n> we had more---but after seeing that many candidates' submissions\n> apparently did not take into account the reviews given to others'\n> submissions on the same micro and/or had many exactly the same\n> issues like log message styles as submissions on other micros\n> that have already been reviewed, I personally do not think they\n> are blameless.\n> \n> So in short, yes it would have been nicer if we had more micros than\n> candidates, but I do not think it was detrimental for the purpose of\n> these micro exercises that multiple candidates ended up attempting\n> the same micro.\n\nI wish I had had time to invent more microprojects. I think we were\nlucky: since students didn't seem to learn from each other's attempts,\nthe fact that many people tried the same microprojects wasn't much of a\nproblem. But if a student had arrived with a \"perfect\" solution to a\nwell-trodden microproject on his/her first attempt, I would hate to have\nto be suspicious that the student plagiarized from somebody else's\nanswer plus all of the review comments about the earlier answer,\nespecially since a perfect solution would itself reduce the amount of\ninteraction between the cheating student and the mailing list compared\nto a student who required several iterations. I also hope that students\ndidn't feel unwelcomed during the times when the list of untaken\nmicroprojects was nearly empty.\n\nIf we really wanted to make this system cheat-proof, there would have to\nbe not only enough microprojects to go around, but also some mechanism\nto ensure that student B didn't start work on a microproject that\nstudent A was just about to submit a solution to. Maybe students would\nhave to claim a microproject when they started work on them, or request\na microproject to work on as opposed to picking one from a list on a web\npage. But realistically, I think that this hypothetical improved system\nwould be more work than we would have been able to invest.\n\nMaybe in the future our microprojects should have two parts:\n\na. Fix ...\nb. Invent a microproject for the next student.\n\n(Just kidding.)\n\n> Again, Big Thanks to Michael for the excellent \"micro\" idea, and all\n> reviewers who participated.\n\nI'd really like to thank the reviewers, who put in enormous effort\nreviewing submissions promptly, and showed superhuman patience when\ndealing with the same issues over an over again. Without such great\nreviewers the whole idea would have just resulted in frustration for the\nstudents; with them, I think that even the students who don't get a GSoC\nspot will have learned something valuable from their participation.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"236899","messageId":"xmqqeh20ej1a.fsf@gitster.dls.corp.google.com","threadId":"36131","inReplyTo":"5327027D.6090500@alum.mit.edu","subject":"Re: [PATCH] GSoC Change multiple if-else statements to be table-driven","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-17T19:12:33Z","receivedAt":"2014-03-17T19:12:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> On 03/17/2014 08:31 AM, Junio C Hamano wrote:\n>\n>> So in short, yes it would have been nicer if we had more micros than\n>> candidates, but I do not think it was detrimental for the purpose of\n>> these micro exercises that multiple candidates ended up attempting\n>> the same micro.\n>\n> I wish I had had time to invent more microprojects. I think we were\n> lucky: since students didn't seem to learn from each other's attempts,\n> the fact that many people tried the same microprojects wasn't much of a\n> problem. But if a student had arrived with a \"perfect\" solution to a\n> well-trodden microproject on his/her first attempt, I would hate to have\n> to be suspicious that the student plagiarized from somebody else's\n> answer plus all of the review comments about the earlier answer,\n> especially since a perfect solution would itself reduce the amount of\n> interaction between the cheating student and the mailing list compared\n> to a student who required several iterations.\n\nIt may probably be not a huge problem. If anything, a cheater would\nhave also learned how the mailing interaction goes when trying to\nplagiarise. And not really having interacted us, a cheater would\nhave left us no impression on his or her communication skills, so it\nwould probably have been detrimental for his or her own chance to be\nchosen as a student, compared to the ones who started from their own\nsolution and polished it by responding to reviews.\n\n> I also hope that students\n> didn't feel unwelcomed during the times when the list of untaken\n> microprojects was nearly empty.\n\nYeah, that is why I said I was of multiple minds. I wasn't enthused\nby seeing the ones that somebody is attempting marked as \"taken\" in\nthe first place. Solving one that others attempted in his or her\nown way and interacting with reviewers seemed to have just as much\ninformation on the candidate, at least to me.\n\n> If we really wanted to make this system cheat-proof, there would have to\n> be not only enough microprojects to go around, but also some mechanism\n> to ensure that student B didn't start work on a microproject that\n> student A was just about to submit a solution to.\n\nNah, I do not think there is any need to go there. It is over for\nthis year anyway, but I do not think such a provision is necessary\nfor future years, if Git project will participate in future GSoCs;\nsee above.\n\n> Maybe in the future our microprojects should have two parts:\n>\n> a. Fix ...\n> b. Invent a microproject for the next student.\n>\n> (Just kidding.)\n\n;-)\n"},{"id":"236903","messageId":"CAPig+cQ07mqZDsHQ7qgWARx6eZ2mD+-w0YF3pJXwiJxufj-peg@mail.gmail.com","threadId":"36131","inReplyTo":"5327027D.6090500@alum.mit.edu","subject":"Re: [PATCH] GSoC Change multiple if-else statements to be table-driven","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-17T19:27:23Z","receivedAt":"2014-03-17T19:27:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 17, 2014 at 10:11 AM, Michael Haggerty wrote:\n> On 03/17/2014 08:31 AM, Junio C Hamano wrote:\n>> Eric Sunshine writes:\n>>\n>>> Perhaps it is time to mark this microproject as \"taken\" on the GSoC\n>>> page [2], along a fews others for which we have received multiple\n>>> submissions.\n>\n> I just marked #8 as taken, as it's been beaten to death.\n>\n> I haven't been keeping careful track of which other microprojects have\n> been overused. If you have suggestions for the chopping block, let me know.\n\nA quick (perhaps inaccurate) search of the mailing list shows that, of\nthe remaining \"untaken\" items, #10, 11, 12, 15, 16, and 18 have had\njust one submission, and #13 had two, so we're okay. (When I wrote the\nabove, I was probably thinking of some of the earlier items on the\nlist which we saw submitted repeatedly.)\n"},{"id":"236912","messageId":"CALs4jVEqAVXEjioHniu8Wf8c+nb_JQXG5gn__AZ+urc5tbx=Ag@mail.gmail.com","threadId":"36131","inReplyTo":"CAPig+cQ07mqZDsHQ7qgWARx6eZ2mD+-w0YF3pJXwiJxufj-peg@mail.gmail.com","subject":"Re: [PATCH] GSoC Change multiple if-else statements to be table-driven","fromName":"Quint Guvernator","fromEmail":"quintus.public@gmail.com","sentAt":"2014-03-17T20:39:34Z","receivedAt":"2014-03-17T20:39:34Z","isPatch":true,"sender":{"key":"quintus.public@gmail.com","avatar":"https://gravatar.com/avatar/0805695585d6191e00badd5057b262b501047d9fb2b22dd2495009ffb25d85fd?d=mp&s=160"},"body":"2014-03-17 15:27 GMT-04:00 Eric Sunshine :\n> A quick (perhaps inaccurate) search of the mailing list shows that, of\n> the remaining \"untaken\" items, #10, 11, 12, 15, 16, and 18 have had\n> just one submission, and #13 had two, so we're okay.\n\nI am still working on #14: \"Change fetch-pack.c:filter_refs() to use\nstarts_with() instead of memcmp(). Try to find other sites that could\nbe rewritten similarly.\"\nAnother version of the patch should be on this list within the hour.\n\nQuint\n"}]}