{"thread":{"id":"36222","subject":"[PATCH v2 1/3][GSOC] diff: rename read_directory() to get_path_list()","startedAt":"2014-03-19T11:23:48Z","lastAt":"2014-03-19T21:15:53Z","messageCount":6,"participants":["Hiroyuki Sano","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"237061","messageId":"1395228230-10189-1-git-send-email-sh19910711@gmail.com","threadId":"36222","inReplyTo":null,"subject":"[PATCH v2 1/3][GSOC] diff: rename read_directory() to get_path_list()","fromName":"Hiroyuki Sano","fromEmail":"sh19910711@gmail.com","sentAt":"2014-03-19T11:23:48Z","receivedAt":"2014-03-19T11:23:48Z","isPatch":true,"sender":{"key":"sh19910711@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1015222?v=4"},"body":"Including \"dir.h\" in \"diff-no-index.c\", it causes a compile error, because\nthe same name function read_directory() is declared globally in \"dir.h\".\n\nThis change is to avoid conflicts as above.\n\nSigned-off-by: Hiroyuki Sano <sh19910711@gmail.com>\n---\n diff-no-index.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 8e10bff..20b6a8a 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -16,7 +16,7 @@\n #include \"builtin.h\"\n #include \"string-list.h\"\n \n-static int read_directory(const char *path, struct string_list *list)\n+static int get_path_list(const char *path, struct string_list *list)\n {\n \tDIR *dir;\n \tstruct dirent *e;\n@@ -107,9 +107,9 @@ static int queue_diff(struct diff_options *o,\n \t\tint i1, i2, ret = 0;\n \t\tsize_t len1 = 0, len2 = 0;\n \n-\t\tif (name1 && read_directory(name1, &p1))\n+\t\tif (name1 && get_path_list(name1, &p1))\n \t\t\treturn -1;\n-\t\tif (name2 && read_directory(name2, &p2)) {\n+\t\tif (name2 && get_path_list(name2, &p2)) {\n \t\t\tstring_list_clear(&p1, 0);\n \t\t\treturn -1;\n \t\t}\n-- \n1.9.0\n"},{"id":"237062","messageId":"1395228230-10189-2-git-send-email-sh19910711@gmail.com","threadId":"36222","inReplyTo":"1395228230-10189-1-git-send-email-sh19910711@gmail.com","subject":"[PATCH v2 2/3][GSOC] diff: use is_dot_or_dotdot() instead of strcmp()","fromName":"Hiroyuki Sano","fromEmail":"sh19910711@gmail.com","sentAt":"2014-03-19T11:23:49Z","receivedAt":"2014-03-19T11:23:49Z","isPatch":true,"sender":{"key":"sh19910711@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1015222?v=4"},"body":"The is_dot_or_dotdot() is used to check if the string is either \".\" or \"..\".\nInclude the \"dir.h\" header file to use is_dot_or_dotdot().\n\nSigned-off-by: Hiroyuki Sano <sh19910711@gmail.com>\n---\n diff-no-index.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 20b6a8a..8e642b3 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -11,6 +11,7 @@\n #include \"tag.h\"\n #include \"diff.h\"\n #include \"diffcore.h\"\n+#include \"dir.h\"\n #include \"revision.h\"\n #include \"log-tree.h\"\n #include \"builtin.h\"\n@@ -25,7 +26,7 @@ static int get_path_list(const char *path, struct string_list *list)\n \t\treturn error(\"Could not open directory %s\", path);\n \n \twhile ((e = readdir(dir)))\n-\t\tif (strcmp(\".\", e->d_name) && strcmp(\"..\", e->d_name))\n+\t\tif (!is_dot_or_dotdot(e->d_name))\n \t\t\tstring_list_insert(list, e->d_name);\n \n \tclosedir(dir);\n-- \n1.9.0\n"},{"id":"237063","messageId":"1395228230-10189-3-git-send-email-sh19910711@gmail.com","threadId":"36222","inReplyTo":"1395228230-10189-1-git-send-email-sh19910711@gmail.com","subject":"[PATCH v2 3/3][GSOC] fsck: replace if-statements to logical expressions","fromName":"Hiroyuki Sano","fromEmail":"sh19910711@gmail.com","sentAt":"2014-03-19T11:23:50Z","receivedAt":"2014-03-19T11:23:50Z","isPatch":true,"sender":{"key":"sh19910711@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1015222?v=4"},"body":"There were two different ways to check flag values,\none way is using if-statement, and the other way is\nusing logical expression.\n\nTo make sensible, replace if-statements to logical expressions\nin fsck_tree().\n\nWhen checking \"has_dot\" and \"has_dotdot\", use is_dot_or_dotdot()\ninstead of strcmp() to avoid hard coding.\n\nThe is_dot_or_dotdot() is used to check if the string is\neither \".\" or \"..\".\nInclude the \"dir.h\" header file to use is_dot_or_dotdot().\n\nSigned-off-by: Hiroyuki Sano <sh19910711@gmail.com>\n---\n fsck.c | 19 +++++++------------\n 1 file changed, 7 insertions(+), 12 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex b3022ad..08f613d 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -6,6 +6,7 @@\n #include \"commit.h\"\n #include \"tag.h\"\n #include \"fsck.h\"\n+#include \"dir.h\"\n \n static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)\n {\n@@ -165,18 +166,12 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)\n \n \t\tsha1 = tree_entry_extract(&desc, &name, &mode);\n \n-\t\tif (is_null_sha1(sha1))\n-\t\t\thas_null_sha1 = 1;\n-\t\tif (strchr(name, '/'))\n-\t\t\thas_full_path = 1;\n-\t\tif (!*name)\n-\t\t\thas_empty_name = 1;\n-\t\tif (!strcmp(name, \".\"))\n-\t\t\thas_dot = 1;\n-\t\tif (!strcmp(name, \"..\"))\n-\t\t\thas_dotdot = 1;\n-\t\tif (!strcmp(name, \".git\"))\n-\t\t\thas_dotgit = 1;\n+\t\thas_null_sha1 |= is_null_sha1(sha1);\n+\t\thas_full_path |= !!strchr(name, '/');\n+\t\thas_empty_name |= !*name;\n+\t\thas_dot |= is_dot_or_dotdot(name) && !name[1];\n+\t\thas_dotdot |= is_dot_or_dotdot(name) && name[1];\n+\t\thas_dotgit |= !strcmp(name, \".git\");\n \t\thas_zero_pad |= *(char *)desc.buffer == '0';\n \t\tupdate_tree_entry(&desc);\n \n-- \n1.9.0\n"},{"id":"237097","messageId":"xmqqha6u3uvl.fsf@gitster.dls.corp.google.com","threadId":"36222","inReplyTo":"1395228230-10189-3-git-send-email-sh19910711@gmail.com","subject":"Re: [PATCH v2 3/3][GSOC] fsck: replace if-statements to logical expressions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-19T18:29:02Z","receivedAt":"2014-03-19T18:29:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hiroyuki Sano <sh19910711@gmail.com> writes:\n\n> There were two different ways to check flag values, one way is\n> using if-statement, and the other way is using logical expression.\n>\n> To make sensible, replace if-statements to logical expressions in\n> fsck_tree().\n\nThe change described by these two paragraphs makes sense to me, but\nthe \"to make sensible\" phrasing made me hiccup while reading it.\n\n\tfsck_tree() uses two different ways to set flag values, many\n\twith a simple if () condition that guards an assignment, and\n\tone with an bitwise-or assignment operator.\n\n\tUnify them to the latter, as it is shorter and easier to\n\tread when the condition is short and to the point, which all\n\tof them are.\n\nor something?\n\n> When checking \"has_dot\" and \"has_dotdot\", use is_dot_or_dotdot()\n> instead of strcmp() to avoid hard coding.\n\nI am not sure how this change is an improvement.  Besides being\nseemingly inefficient by checking name[1] twice (which is not a huge\nobjection, as a sensible compiler would notice and optimize), the\ncaller that checks name[1] already hardcodes its knowledge on what\nis_dot_or_dotdot() does, e.g. when it returns true, name[0] is never\nNUL, and name[1] is NUL only when it saw a dot and not a dotdot, so\nthe \"to avoid hard coding\" does not really justify this change.\n\nI further wonder if\n\n\t...\n\tif (!name[0]) {\n\t\thas_empty_name = 1;\n\t} else if (name[0] == '.') {\n        \thas_dot |= !name[1];\n                has_dotdot |= name[1] == '.' && !name[2];\n\t\thas_dotgit |= !strcmp(name + 1, \"git\");\n\t}\n\t...\n\nmay be an improvement (this is not a suggestion--when I say I\nwonder, I usually do not know the answer).  It defeats the \"unify\nthe two styles\" theme of this change, so...\n\n> The is_dot_or_dotdot() is used to check if the string is\n> either \".\" or \"..\".\n> Include the \"dir.h\" header file to use is_dot_or_dotdot().\n>\n> Signed-off-by: Hiroyuki Sano <sh19910711@gmail.com>\n> ---\n>  fsck.c | 19 +++++++------------\n>  1 file changed, 7 insertions(+), 12 deletions(-)\n>\n> diff --git a/fsck.c b/fsck.c\n> index b3022ad..08f613d 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -6,6 +6,7 @@\n>  #include \"commit.h\"\n>  #include \"tag.h\"\n>  #include \"fsck.h\"\n> +#include \"dir.h\"\n>  \n>  static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)\n>  {\n> @@ -165,18 +166,12 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)\n>  \n>  \t\tsha1 = tree_entry_extract(&desc, &name, &mode);\n>  \n> -\t\tif (is_null_sha1(sha1))\n> -\t\t\thas_null_sha1 = 1;\n> -\t\tif (strchr(name, '/'))\n> -\t\t\thas_full_path = 1;\n> -\t\tif (!*name)\n> -\t\t\thas_empty_name = 1;\n> -\t\tif (!strcmp(name, \".\"))\n> -\t\t\thas_dot = 1;\n> -\t\tif (!strcmp(name, \"..\"))\n> -\t\t\thas_dotdot = 1;\n> -\t\tif (!strcmp(name, \".git\"))\n> -\t\t\thas_dotgit = 1;\n> +\t\thas_null_sha1 |= is_null_sha1(sha1);\n> +\t\thas_full_path |= !!strchr(name, '/');\n> +\t\thas_empty_name |= !*name;\n> +\t\thas_dot |= is_dot_or_dotdot(name) && !name[1];\n> +\t\thas_dotdot |= is_dot_or_dotdot(name) && name[1];\n> +\t\thas_dotgit |= !strcmp(name, \".git\");\n>  \t\thas_zero_pad |= *(char *)desc.buffer == '0';\n>  \t\tupdate_tree_entry(&desc);\n"},{"id":"237118","messageId":"CAPig+cRSh5XoKKBZ6iyAwEvUf90N4Ajfp_459ME3zoBGS2h23Q@mail.gmail.com","threadId":"36222","inReplyTo":"1395228230-10189-1-git-send-email-sh19910711@gmail.com","subject":"Re: [PATCH v2 1/3][GSOC] diff: rename read_directory() to get_path_list()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-19T21:15:48Z","receivedAt":"2014-03-19T21:15:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 19, 2014 at 7:23 AM, Hiroyuki Sano <sh19910711@gmail.com> wrote:\n> Subject: diff: rename read_directory() to get_path_list()\n\nYou probably mean 'diff-no-index' here rather than 'diff'.\n\n> Including \"dir.h\" in \"diff-no-index.c\", it causes a compile error, because\n> the same name function read_directory() is declared globally in \"dir.h\".\n\nIt might be a bit clearer to give a hint as to why dir.h will be a problem:\n\n    A subsequent patch will include dir.h in diff-no-index.c,\n    however, dir.h declares a read_directory() which is different\n    from the one defined statically by diff-no-index.c.\n\n> This change is to avoid conflicts as above.\n\nGood explanation, but write in imperative mood:\n\n    Rename the local read_directory() to avoid the conflict.\n\n> Signed-off-by: Hiroyuki Sano <sh19910711@gmail.com>\n> ---\n>  diff-no-index.c | 6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index 8e10bff..20b6a8a 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -16,7 +16,7 @@\n>  #include \"builtin.h\"\n>  #include \"string-list.h\"\n>\n> -static int read_directory(const char *path, struct string_list *list)\n> +static int get_path_list(const char *path, struct string_list *list)\n>  {\n>         DIR *dir;\n>         struct dirent *e;\n> @@ -107,9 +107,9 @@ static int queue_diff(struct diff_options *o,\n>                 int i1, i2, ret = 0;\n>                 size_t len1 = 0, len2 = 0;\n>\n> -               if (name1 && read_directory(name1, &p1))\n> +               if (name1 && get_path_list(name1, &p1))\n>                         return -1;\n> -               if (name2 && read_directory(name2, &p2)) {\n> +               if (name2 && get_path_list(name2, &p2)) {\n>                         string_list_clear(&p1, 0);\n>                         return -1;\n>                 }\n> --\n> 1.9.0\n>\n"},{"id":"237119","messageId":"CAPig+cT5TMHoLDeavsV89Mopgd59rUBFF8QUj68zbX6ZDNZE7g@mail.gmail.com","threadId":"36222","inReplyTo":"1395228230-10189-2-git-send-email-sh19910711@gmail.com","subject":"Re: [PATCH v2 2/3][GSOC] diff: use is_dot_or_dotdot() instead of strcmp()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-19T21:15:53Z","receivedAt":"2014-03-19T21:15:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 19, 2014 at 7:23 AM, Hiroyuki Sano <sh19910711@gmail.com> wrote:\n> Subject: diff: use is_dot_or_dotdot() instead of strcmp()\n\nYou probably meant 'diff-no-index' rather than 'diff'.\n\nYou could make the subject a bit more explanatory by saying:\n\n    use is_dot_or_dotdot() instead of a manual \".\"/\"..\" check\n\n> The is_dot_or_dotdot() is used to check if the string is either \".\" or \"..\".\n\nIt's pretty obvious what this function does, so it's not necessary to\nexplain it.\n\n> Include the \"dir.h\" header file to use is_dot_or_dotdot().\n\nIncluding dir.h is a obvious requirement of using is_dot_or_dotdot(),\nthus also does not require explanation.\n\nOtherwise, the patch looks fine.\n\n> Signed-off-by: Hiroyuki Sano <sh19910711@gmail.com>\n> ---\n>  diff-no-index.c | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index 20b6a8a..8e642b3 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -11,6 +11,7 @@\n>  #include \"tag.h\"\n>  #include \"diff.h\"\n>  #include \"diffcore.h\"\n> +#include \"dir.h\"\n>  #include \"revision.h\"\n>  #include \"log-tree.h\"\n>  #include \"builtin.h\"\n> @@ -25,7 +26,7 @@ static int get_path_list(const char *path, struct string_list *list)\n>                 return error(\"Could not open directory %s\", path);\n>\n>         while ((e = readdir(dir)))\n> -               if (strcmp(\".\", e->d_name) && strcmp(\"..\", e->d_name))\n> +               if (!is_dot_or_dotdot(e->d_name))\n>                         string_list_insert(list, e->d_name);\n>\n>         closedir(dir);\n> --\n> 1.9.0\n>\n"}]}