threads / patch / 36201

patch, 3 parts[GSOC] diff: rename read_directory() to get_directory_list()

Subject: [PATCH 1/3][GSOC] diff: rename read_directory() to get_directory_list()

## tl;dr

8 messages between Mar 17, 2014 and Mar 19, 2014. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Hiroyuki Sano· Mar 17, 2014, 21:30 UTC · lore

Including "dir.h" in "diff-no-index.c", it causes a compile error, because the same name function read_directory() is declared globally in "dir.h".

This change is to avoid conflicts as above.
Signed-off-by: Hiroyuki Sano <sh19910711@gmail.com>
---
 diff-no-index.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Show changes to diff-no-index.c +3 −3
diff --git a/diff-no-index.c b/diff-no-index.c
index 8e10bff..1ed5c9d 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -16,7 +16,7 @@
 #include "builtin.h"
 #include "string-list.h"
 
-static int read_directory(const char *path, struct string_list *list)
+static int get_directory_list(const char *path, struct string_list *list)
 {
 	DIR *dir;
 	struct dirent *e;
@@ -107,9 +107,9 @@ static int queue_diff(struct diff_options *o,
 		int i1, i2, ret = 0;
 		size_t len1 = 0, len2 = 0;
 
-		if (name1 && read_directory(name1, &p1))
+		if (name1 && get_directory_list(name1, &p1))
 			return -1;
-		if (name2 && read_directory(name2, &p2)) {
+		if (name2 && get_directory_list(name2, &p2)) {
 			string_list_clear(&p1, 0);
 			return -1;
 		}

-- 
1.9.0
Hiroyuki Sano· Mar 17, 2014, 21:30 UTC · re: Hiroyuki Sano · lore

[PATCH 2/3][GSOC] diff: use is_dot_or_dotdot() instead of strcmp()

The is_dot_or_dotdot() is used to check if the string is either "." or "..".
Signed-off-by: Hiroyuki Sano <sh19910711@gmail.com>
---
 diff-no-index.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to diff-no-index.c +2 −1
diff --git a/diff-no-index.c b/diff-no-index.c
index 1ed5c9d..ccd9270 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -11,6 +11,7 @@
 #include "tag.h"
 #include "diff.h"
 #include "diffcore.h"
+#include "dir.h"
 #include "revision.h"
 #include "log-tree.h"
 #include "builtin.h"
@@ -25,7 +26,7 @@ static int get_directory_list(const char *path, struct string_list *list)
 		return error("Could not open directory %s", path);
 
 	while ((e = readdir(dir)))
-		if (strcmp(".", e->d_name) && strcmp("..", e->d_name))
+		if (!is_dot_or_dotdot(e->d_name))
 			string_list_insert(list, e->d_name);
 
 	closedir(dir);

-- 
1.9.0
Hiroyuki Sano· Mar 17, 2014, 21:30 UTC · re: Hiroyuki Sano · lore

[PATCH 3/3][GSOC] fsck: use is_dot_or_dotdot() instead of strcmp()

The is_dot_or_dotdot() is used to check if the string is either "." or "..".
Signed-off-by: Hiroyuki Sano <sh19910711@gmail.com>
---
 fsck.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
Show changes to fsck.c +7 −4
diff --git a/fsck.c b/fsck.c
index b3022ad..c9d7784 100644
--- a/fsck.c
+++ b/fsck.c
@@ -6,6 +6,7 @@
 #include "commit.h"
 #include "tag.h"
 #include "fsck.h"
+#include "dir.h"
 
 static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)
 {
@@ -171,10 +172,12 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)
 			has_full_path = 1;
 		if (!*name)
 			has_empty_name = 1;
-		if (!strcmp(name, "."))
-			has_dot = 1;
-		if (!strcmp(name, ".."))
-			has_dotdot = 1;
+		if (is_dot_or_dotdot(name)) {
+			if (!name[1])
+				has_dot = 1;
+			else
+				has_dotdot = 1;
+		}
 		if (!strcmp(name, ".git"))
 			has_dotgit = 1;
 		has_zero_pad |= *(char *)desc.buffer == '0';

-- 
1.9.0
Junio C Hamano· Mar 17, 2014, 22:39 UTC · re: Hiroyuki Sano · lore

Re: [PATCH 3/3][GSOC] fsck: use is_dot_or_dotdot() instead of strcmp()

Hiroyuki Sano <sh19910711@gmail.com> writes:
Show 33 quoted lines
> The is_dot_or_dotdot() is used to check if the string is either "." or "..".
>
> Signed-off-by: Hiroyuki Sano <sh19910711@gmail.com>
> ---
>  fsck.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/fsck.c b/fsck.c
> index b3022ad..c9d7784 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -6,6 +6,7 @@
>  #include "commit.h"
>  #include "tag.h"
>  #include "fsck.h"
> +#include "dir.h"
>  
>  static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)
>  {
> @@ -171,10 +172,12 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)
>  			has_full_path = 1;
>  		if (!*name)
>  			has_empty_name = 1;
> -		if (!strcmp(name, "."))
> -			has_dot = 1;
> -		if (!strcmp(name, ".."))
> -			has_dotdot = 1;
> +		if (is_dot_or_dotdot(name)) {
> +			if (!name[1])
> +				has_dot = 1;
> +			else
> +				has_dotdot = 1;
> +		}
In what way is this an improvement?

This looks like "because I was told to", not "because the resulting code is better" to me.

The other patch on diff-no-index looked sensible, though.
>  		if (!strcmp(name, ".git"))
>  			has_dotgit = 1;
>  		has_zero_pad |= *(char *)desc.buffer == '0';
Junio C Hamano· Mar 17, 2014, 22:37 UTC · re: Hiroyuki Sano · lore

Re: [PATCH 1/3][GSOC] diff: rename read_directory() to get_directory_list()

Hiroyuki Sano <sh19910711@gmail.com> writes:
Show 20 quoted lines
> Including "dir.h" in "diff-no-index.c", it causes a compile error, because
> the same name function read_directory() is declared globally in "dir.h".
>
> This change is to avoid conflicts as above.
>
> Signed-off-by: Hiroyuki Sano <sh19910711@gmail.com>
> ---
>  diff-no-index.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/diff-no-index.c b/diff-no-index.c
> index 8e10bff..1ed5c9d 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -16,7 +16,7 @@
>  #include "builtin.h"
>  #include "string-list.h"
>  
> -static int read_directory(const char *path, struct string_list *list)
> +static int get_directory_list(const char *path, struct string_list *list)

Renaming is a good idea but the new name sounds like you are grabbing the names of directories, ignoring all the files, no?

Show 15 quoted lines
>  {
>  	DIR *dir;
>  	struct dirent *e;
> @@ -107,9 +107,9 @@ static int queue_diff(struct diff_options *o,
>  		int i1, i2, ret = 0;
>  		size_t len1 = 0, len2 = 0;
>  
> -		if (name1 && read_directory(name1, &p1))
> +		if (name1 && get_directory_list(name1, &p1))
>  			return -1;
> -		if (name2 && read_directory(name2, &p2)) {
> +		if (name2 && get_directory_list(name2, &p2)) {
>  			string_list_clear(&p1, 0);
>  			return -1;
>  		}
Junio C Hamano· Mar 19, 2014, 17:22 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/3][GSOC] diff: rename read_directory() to get_directory_list()

Junio C Hamano <gitster@pobox.com> writes:
Show 25 quoted lines
> Hiroyuki Sano <sh19910711@gmail.com> writes:
>
>> Including "dir.h" in "diff-no-index.c", it causes a compile error, because
>> the same name function read_directory() is declared globally in "dir.h".
>>
>> This change is to avoid conflicts as above.
>>
>> Signed-off-by: Hiroyuki Sano <sh19910711@gmail.com>
>> ---
>>  diff-no-index.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/diff-no-index.c b/diff-no-index.c
>> index 8e10bff..1ed5c9d 100644
>> --- a/diff-no-index.c
>> +++ b/diff-no-index.c
>> @@ -16,7 +16,7 @@
>>  #include "builtin.h"
>>  #include "string-list.h"
>>  
>> -static int read_directory(const char *path, struct string_list *list)
>> +static int get_directory_list(const char *path, struct string_list *list)
>
> Renaming is a good idea but the new name sounds like you are
> grabbing the names of directories, ignoring all the files, no?

I am tempted to suggest, because this is an internal implementation detail only visible to this narrow corner of the system, calling this just 

	static int ls(const char *path, struct string_list *result)
;-)
Junio C Hamano· Mar 19, 2014, 18:48 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/3][GSOC] diff: rename read_directory() to get_directory_list()

Junio C Hamano <gitster@pobox.com> writes:
Show 13 quoted lines
>>> -static int read_directory(const char *path, struct string_list *list)
>>> +static int get_directory_list(const char *path, struct string_list *list)
>>
>> Renaming is a good idea but the new name sounds like you are
>> grabbing the names of directories, ignoring all the files, no?
>
> I am tempted to suggest, because this is an internal implementation
> detail only visible to this narrow corner of the system, calling
> this just 
>
> 	static int ls(const char *path, struct string_list *result)
>
> ;-)
Scratch that one.

I'll take read_directory_contents() from Brian Bourn, which essentially is the same patch.

Thanks.
-- >8 --
From: Brian Bourn <ba.bourn@gmail.com>
Date: Wed, 19 Mar 2014 11:58:21 -0400
Subject: [PATCH] diff-no-index: rename read_directory()

In the next patch, we will replace a manual checking of "." or ".." with a call to is_dot_or_dotdot() defined in dir.h. The private function read_directory() defined in this file will conflict with the global function declared there when we do so.

As a preparatory step, rename the private read_directory() to avoid the name collision.

Signed-off-by: Brian Bourn <ba.bourn@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff-no-index.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Show changes to diff-no-index.c +3 −3
diff --git a/diff-no-index.c b/diff-no-index.c
index 33e5982..3e4f47e 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -16,7 +16,7 @@
 #include "builtin.h"
 #include "string-list.h"
 
-static int read_directory(const char *path, struct string_list *list)
+static int read_directory_contents(const char *path, struct string_list *list)
 {
 	DIR *dir;
 	struct dirent *e;
@@ -107,9 +107,9 @@ static int queue_diff(struct diff_options *o,
 		int i1, i2, ret = 0;
 		size_t len1 = 0, len2 = 0;
 
-		if (name1 && read_directory(name1, &p1))
+		if (name1 && read_directory_contents(name1, &p1))
 			return -1;
-		if (name2 && read_directory(name2, &p2)) {
+		if (name2 && read_directory_contents(name2, &p2)) {
 			string_list_clear(&p1, 0);
 			return -1;
 		}

-- 
1.9.1-423-g4596e3a
Eric Sunshine· Mar 19, 2014, 10:04 UTC · re: Hiroyuki Sano · lore

Re: [PATCH 1/3][GSOC] diff: rename read_directory() to get_directory_list()

On Mon, Mar 17, 2014 at 5:30 PM, Hiroyuki Sano <sh19910711@gmail.com> wrote:
> Including "dir.h" in "diff-no-index.c", it causes a compile error, because
> the same name function read_directory() is declared globally in "dir.h".
>
> This change is to avoid conflicts as above.

This explanation is slightly lacking. Since dir.h is not currently included by this file, a person reading this patch may wonder why you need to avoid conflict where there is none. Adding a short sentence saying that a subsequent patch will include dir.h in order to access is_dot_or_dotdot() can avoid such confusion.

Show 32 quoted lines
> Signed-off-by: Hiroyuki Sano <sh19910711@gmail.com>
> ---
>  diff-no-index.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/diff-no-index.c b/diff-no-index.c
> index 8e10bff..1ed5c9d 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -16,7 +16,7 @@
>  #include "builtin.h"
>  #include "string-list.h"
>
> -static int read_directory(const char *path, struct string_list *list)
> +static int get_directory_list(const char *path, struct string_list *list)
>  {
>         DIR *dir;
>         struct dirent *e;
> @@ -107,9 +107,9 @@ static int queue_diff(struct diff_options *o,
>                 int i1, i2, ret = 0;
>                 size_t len1 = 0, len2 = 0;
>
> -               if (name1 && read_directory(name1, &p1))
> +               if (name1 && get_directory_list(name1, &p1))
>                         return -1;
> -               if (name2 && read_directory(name2, &p2)) {
> +               if (name2 && get_directory_list(name2, &p2)) {
>                         string_list_clear(&p1, 0);
>                         return -1;
>                 }
> --
> 1.9.0

← back to recent threads