[PATCH v2 1/3][GSOC] diff: rename read_directory() to get_path_list()

[PATCH v2 1/3][GSOC] diff: rename read_directory() to get_path_list()

[Hiroyuki Sano]

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..20b6a8a 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_path_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_path_list(name1, &p1))
 			return -1;
-		if (name2 && read_directory(name2, &p2)) {
+		if (name2 && get_path_list(name2, &p2)) {
 			string_list_clear(&p1, 0);
 			return -1;
 		}
-- 
1.9.0

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

[Hiroyuki Sano]

The is_dot_or_dotdot() is used to check if the string is either "." or "..".
Include the "dir.h" header file to use is_dot_or_dotdot().

Signed-off-by: Hiroyuki Sano <sh19910711@gmail.com>
---
 diff-no-index.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/diff-no-index.c b/diff-no-index.c
index 20b6a8a..8e642b3 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_path_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

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

[Eric Sunshine]

On Wed, Mar 19, 2014 at 7:23 AM, Hiroyuki Sano <sh19910711@gmail.com> wrote:
> Subject: diff: use is_dot_or_dotdot() instead of strcmp()

You probably meant 'diff-no-index' rather than 'diff'.

You could make the subject a bit more explanatory by saying:

    use is_dot_or_dotdot() instead of a manual "."/".." check

> The is_dot_or_dotdot() is used to check if the string is either "." or "..".

It's pretty obvious what this function does, so it's not necessary to
explain it.

> Include the "dir.h" header file to use is_dot_or_dotdot().

Including dir.h is a obvious requirement of using is_dot_or_dotdot(),
thus also does not require explanation.

Otherwise, the patch looks fine.

> Signed-off-by: Hiroyuki Sano <sh19910711@gmail.com>
> ---
>  diff-no-index.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/diff-no-index.c b/diff-no-index.c
> index 20b6a8a..8e642b3 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_path_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
>

[PATCH v2 3/3][GSOC] fsck: replace if-statements to logical expressions

[Hiroyuki Sano]

There were two different ways to check flag values,
one way is using if-statement, and the other way is
using logical expression.

To make sensible, replace if-statements to logical expressions
in fsck_tree().

When checking "has_dot" and "has_dotdot", use is_dot_or_dotdot()
instead of strcmp() to avoid hard coding.

The is_dot_or_dotdot() is used to check if the string is
either "." or "..".
Include the "dir.h" header file to use is_dot_or_dotdot().

Signed-off-by: Hiroyuki Sano <sh19910711@gmail.com>
---
 fsck.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/fsck.c b/fsck.c
index b3022ad..08f613d 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)
 {
@@ -165,18 +166,12 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)
 
 		sha1 = tree_entry_extract(&desc, &name, &mode);
 
-		if (is_null_sha1(sha1))
-			has_null_sha1 = 1;
-		if (strchr(name, '/'))
-			has_full_path = 1;
-		if (!*name)
-			has_empty_name = 1;
-		if (!strcmp(name, "."))
-			has_dot = 1;
-		if (!strcmp(name, ".."))
-			has_dotdot = 1;
-		if (!strcmp(name, ".git"))
-			has_dotgit = 1;
+		has_null_sha1 |= is_null_sha1(sha1);
+		has_full_path |= !!strchr(name, '/');
+		has_empty_name |= !*name;
+		has_dot |= is_dot_or_dotdot(name) && !name[1];
+		has_dotdot |= is_dot_or_dotdot(name) && name[1];
+		has_dotgit |= !strcmp(name, ".git");
 		has_zero_pad |= *(char *)desc.buffer == '0';
 		update_tree_entry(&desc);
 
-- 
1.9.0

Re: [PATCH v2 3/3][GSOC] fsck: replace if-statements to logical expressions

[Junio C Hamano]

Hiroyuki Sano <sh19910711@gmail.com> writes:

> There were two different ways to check flag values, one way is
> using if-statement, and the other way is using logical expression.
>
> To make sensible, replace if-statements to logical expressions in
> fsck_tree().

The change described by these two paragraphs makes sense to me, but
the "to make sensible" phrasing made me hiccup while reading it.

	fsck_tree() uses two different ways to set flag values, many
	with a simple if () condition that guards an assignment, and
	one with an bitwise-or assignment operator.

	Unify them to the latter, as it is shorter and easier to
	read when the condition is short and to the point, which all
	of them are.

or something?

> When checking "has_dot" and "has_dotdot", use is_dot_or_dotdot()
> instead of strcmp() to avoid hard coding.

I am not sure how this change is an improvement.  Besides being
seemingly inefficient by checking name[1] twice (which is not a huge
objection, as a sensible compiler would notice and optimize), the
caller that checks name[1] already hardcodes its knowledge on what
is_dot_or_dotdot() does, e.g. when it returns true, name[0] is never
NUL, and name[1] is NUL only when it saw a dot and not a dotdot, so
the "to avoid hard coding" does not really justify this change.

I further wonder if

	...
	if (!name[0]) {
		has_empty_name = 1;
	} else if (name[0] == '.') {
        	has_dot |= !name[1];
                has_dotdot |= name[1] == '.' && !name[2];
		has_dotgit |= !strcmp(name + 1, "git");
	}
	...

may be an improvement (this is not a suggestion--when I say I
wonder, I usually do not know the answer).  It defeats the "unify
the two styles" theme of this change, so...

> The is_dot_or_dotdot() is used to check if the string is
> either "." or "..".
> Include the "dir.h" header file to use is_dot_or_dotdot().
>
> Signed-off-by: Hiroyuki Sano <sh19910711@gmail.com>
> ---
>  fsck.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/fsck.c b/fsck.c
> index b3022ad..08f613d 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)
>  {
> @@ -165,18 +166,12 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)
>  
>  		sha1 = tree_entry_extract(&desc, &name, &mode);
>  
> -		if (is_null_sha1(sha1))
> -			has_null_sha1 = 1;
> -		if (strchr(name, '/'))
> -			has_full_path = 1;
> -		if (!*name)
> -			has_empty_name = 1;
> -		if (!strcmp(name, "."))
> -			has_dot = 1;
> -		if (!strcmp(name, ".."))
> -			has_dotdot = 1;
> -		if (!strcmp(name, ".git"))
> -			has_dotgit = 1;
> +		has_null_sha1 |= is_null_sha1(sha1);
> +		has_full_path |= !!strchr(name, '/');
> +		has_empty_name |= !*name;
> +		has_dot |= is_dot_or_dotdot(name) && !name[1];
> +		has_dotdot |= is_dot_or_dotdot(name) && name[1];
> +		has_dotgit |= !strcmp(name, ".git");
>  		has_zero_pad |= *(char *)desc.buffer == '0';
>  		update_tree_entry(&desc);

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

[Eric Sunshine]

On Wed, Mar 19, 2014 at 7:23 AM, Hiroyuki Sano <sh19910711@gmail.com> wrote:
> Subject: diff: rename read_directory() to get_path_list()

You probably mean 'diff-no-index' here rather than 'diff'.

> 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".

It might be a bit clearer to give a hint as to why dir.h will be a problem:

    A subsequent patch will include dir.h in diff-no-index.c,
    however, dir.h declares a read_directory() which is different
    from the one defined statically by diff-no-index.c.

> This change is to avoid conflicts as above.

Good explanation, but write in imperative mood:

    Rename the local read_directory() to avoid the conflict.

> 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..20b6a8a 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_path_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_path_list(name1, &p1))
>                         return -1;
> -               if (name2 && read_directory(name2, &p2)) {
> +               if (name2 && get_path_list(name2, &p2)) {
>                         string_list_clear(&p1, 0);
>                         return -1;
>                 }
> --
> 1.9.0
>