{"thread":{"id":"36257","subject":"[GSoc PATCH 1/3] diff-no-index.c: rename read_directory()","startedAt":"2014-03-20T19:52:44Z","lastAt":"2014-03-20T23:53:13Z","messageCount":2,"participants":["Andrei Dinu","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"237201","messageId":"1395345164-5239-1-git-send-email-mandrei.dinu@gmail.com","threadId":"36257","inReplyTo":null,"subject":"[GSoc PATCH 1/3] diff-no-index.c: rename read_directory()","fromName":"Andrei Dinu","fromEmail":"mandrei.dinu@gmail.com","sentAt":"2014-03-20T19:52:44Z","receivedAt":"2014-03-20T19:52:44Z","isPatch":true,"sender":{"key":"mandrei.dinu@gmail.com","avatar":null},"body":"\nAvoid the conflict between read_directory() from diff-no-index.c and \nread_directory() from \"dir.h\"\n\nSigned-off-by: Andrei Dinu \n\n---\n\n I plan on applying to GSOC 2014\n\n Submit again on the list for an older bug that I solved, to show you that \n I received your feedback and I reviewed my code, numbering and partitioning \n patches style. Thank you!\n\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..5e4a76c 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 read_directory_path(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 && read_directory_path(name1, &p1))\n \t\t\treturn -1;\n-\t\tif (name2 && read_directory(name2, &p2)) {\n+\t\tif (name2 && read_directory_path(name2, &p2)) {\n \t\t\tstring_list_clear(&p1, 0);\n \t\t\treturn -1;\n \t\t}\n-- \n1.7.9.5\n\n\n>From d54129eacb45b307dacf2b7afebd1da40df17047 Mon Sep 17 00:00:00 2001\nFrom: Andrei Dinu \nDate: Wed, 19 Mar 2014 17:42:08 +0200\nSubject: [GSoc PATCH 2/3] diff-no-index.c: read_directory_path() use\n is_dot_or_dotdot().\n\nImplement code so read_directory_path() use is_dot_or_dotdot() from \"dir.h\"\ninstead of strcmp().\n\nSigned-off-by: Andrei Dinu \n\n---\n\n I plan on applying to GSOC 2014\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 5e4a76c..2d1165f 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -15,6 +15,7 @@\n #include \"log-tree.h\"\n #include \"builtin.h\"\n #include \"string-list.h\"\n+#include \"dir.h\"\n \n static int read_directory_path(const char *path, struct string_list *list)\n {\n@@ -25,7 +26,7 @@ static int read_directory_path(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.7.9.5\n\n\n>From 4843ad23675047163211c68434517c097435ebb7 Mon Sep 17 00:00:00 2001\nFrom: Andrei Dinu \nDate: Wed, 19 Mar 2014 18:01:55 +0200\nSubject: [GSoc PATCH 3/3] fsck.c: fsck_tree() now use is_dot_or_dotdot().\n\nRewrite fsck_tree() to use is_dot_or_dotdot() from \"dir.h\" instead \nof calling twice strcmp().\n\nSigned-off-by: Andrei Dinu \n\n---\n\n I plan on applying to GSOC 2014.\n\n fsck.c | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 64bf279..82a55ab 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@@ -171,10 +172,12 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)\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 (is_dot_or_dotdot(name))\n+\t\t\tif (name[1] == '\\0')\n+\t\t\t\thas_dot = 1;\n+\t\t\telse\n+\t\t\t\thas_dotdot = 1;\n \t\tif (!strcmp(name, \".git\"))\n \t\t\thas_dotgit = 1;\n \t\thas_zero_pad |= *(char *)desc.buffer == '0';\n-- \n1.7.9.5\n"},{"id":"237244","messageId":"CAPig+cS4ScFQhMEkWigay1oi6X=6H7JC7LoxW6U4g9AWyR37=g@mail.gmail.com","threadId":"36257","inReplyTo":"1395345164-5239-1-git-send-email-mandrei.dinu@gmail.com","subject":"Re: [GSoc PATCH 1/3] diff-no-index.c: rename read_directory()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-20T23:53:13Z","receivedAt":"2014-03-20T23:53:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 20, 2014 at 3:52 PM, Andrei Dinu wrote:\n> Avoid the conflict between read_directory() from diff-no-index.c and\n> read_directory() from \"dir.h\"\n\nThis message is lacking a bit of information. Since diff-no-index.c\ndoes not presently include dir.h, the reader is left to wonder why you\nneed to resolve a non-existent conflict. You should explain that\ndiff-no-index.c will soon be including dir.h, which is why the\nfunction must be renamed. You might say something like:\n\n A subsequent patch will include dir.h in diff-no-index.c to\n access is_dot_or_dotdot(), however, dir.h declares a\n read_directory() which conflicts with a (different)\n read_directory() defined in diff-no-index.c. Rename\n diff-no-index.c's read_directory() to avoid the conflict.\n\n> Signed-off-by: Andrei Dinu \n>\n> ---\n>\n> I plan on applying to GSOC 2014\n>\n> Submit again on the list for an older bug that I solved, to show you that\n> I received your feedback and I reviewed my code, numbering and partitioning\n> patches style. Thank you!\n\nUnfortunately, all three patches arrived in a single email message,\nwhich is undesirable. It's a good idea to try sending patches to\nyourself first (using \"git send-email\") to make sure that everything\ncomes through okay; then send to the list.\n\n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index 8e10bff..5e4a76c 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 read_directory_path(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 && read_directory_path(name1, &p1))\n> return -1;\n> - if (name2 && read_directory(name2, &p2)) {\n> + if (name2 && read_directory_path(name2, &p2)) {\n\nThe patch looks reasonable. The name is a bit odd, but not terribly important.\n\n> string_list_clear(&p1, 0);\n> return -1;\n> }\n> --\n> 1.7.9.5\n>\n>\n> From d54129eacb45b307dacf2b7afebd1da40df17047 Mon Sep 17 00:00:00 2001\n> From: Andrei Dinu \n> Date: Wed, 19 Mar 2014 17:42:08 +0200\n> Subject: [GSoc PATCH 2/3] diff-no-index.c: read_directory_path() use\n> is_dot_or_dotdot().\n\nYou can drop the \".\" from the end of this line.\n\n> Implement code so read_directory_path() use is_dot_or_dotdot() from \"dir.h\"\n> instead of strcmp().\n\nYou're not really implementing any code, so this is a bit misleading,\nand you don't need to explain that it's from dir.h since that's\nobvious from the fact that you are including dir.h You might say\ninstead:\n\n Use is_dot_or_dotdot() instead of manually checking against \".\"\n or \"..\".\n\n> Signed-off-by: Andrei Dinu \n>\n> ---\n>\n> I plan on applying to GSOC 2014\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 5e4a76c..2d1165f 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -15,6 +15,7 @@\n> #include \"log-tree.h\"\n> #include \"builtin.h\"\n> #include \"string-list.h\"\n> +#include \"dir.h\"\n>\n> static int read_directory_path(const char *path, struct string_list *list)\n> {\n> @@ -25,7 +26,7 @@ static int read_directory_path(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\nLooks fine.\n\n> string_list_insert(list, e->d_name);\n>\n> closedir(dir);\n> --\n> 1.7.9.5\n>\n>\n> From 4843ad23675047163211c68434517c097435ebb7 Mon Sep 17 00:00:00 2001\n> From: Andrei Dinu \n> Date: Wed, 19 Mar 2014 18:01:55 +0200\n> Subject: [GSoc PATCH 3/3] fsck.c: fsck_tree() now use is_dot_or_dotdot().\n\nDrop \"now\" and the \".\".\n\n> Rewrite fsck_tree() to use is_dot_or_dotdot() from \"dir.h\" instead\n> of calling twice strcmp().\n\nNo need to explain that it is from dir.h.\n\n> Signed-off-by: Andrei Dinu \n>\n> ---\n>\n> I plan on applying to GSOC 2014.\n>\n> fsck.c | 11 +++++++----\n> 1 file changed, 7 insertions(+), 4 deletions(-)\n>\n> diff --git a/fsck.c b/fsck.c\n> index 64bf279..82a55ab 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> @@ -171,10 +172,12 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)\n> has_full_path = 1;\n> if (!*name)\n> has_empty_name = 1;\n> - if (!strcmp(name, \".\"))\n> - has_dot = 1;\n> - if (!strcmp(name, \"..\"))\n> - has_dotdot = 1;\n> + if (is_dot_or_dotdot(name))\n> + if (name[1] == '\\0')\n\nOn this project it probably would be more idiomatic to say !name[1].\n\n> + has_dot = 1;\n> + else\n> + has_dotdot = 1;\n\nOverall, however, this change does not really make the code clearer.\nIt probably would be best to drop this patch from your submission.\n\n> if (!strcmp(name, \".git\"))\n> has_dotgit = 1;\n> has_zero_pad |= *(char *)desc.buffer == '0';\n> --\n> 1.7.9.5\n"}]}