* [PATCH v2 1/3][GSOC] diff: rename read_directory() to get_path_list()
@ 2014-03-19 11:23 Hiroyuki Sano
2014-03-19 11:23 ` [PATCH v2 2/3][GSOC] diff: use is_dot_or_dotdot() instead of strcmp() Hiroyuki Sano
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Hiroyuki Sano @ 2014-03-19 11:23 UTC (permalink / raw)
To: git; +Cc: Hiroyuki Sano, Junio C Hamano, Eric Sunshine
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/3][GSOC] diff: use is_dot_or_dotdot() instead of strcmp()
2014-03-19 11:23 [PATCH v2 1/3][GSOC] diff: rename read_directory() to get_path_list() Hiroyuki Sano
@ 2014-03-19 11:23 ` Hiroyuki Sano
2014-03-19 21:15 ` Eric Sunshine
2014-03-19 11:23 ` [PATCH v2 3/3][GSOC] fsck: replace if-statements to logical expressions Hiroyuki Sano
2014-03-19 21:15 ` [PATCH v2 1/3][GSOC] diff: rename read_directory() to get_path_list() Eric Sunshine
2 siblings, 1 reply; 6+ messages in thread
From: Hiroyuki Sano @ 2014-03-19 11:23 UTC (permalink / raw)
To: git; +Cc: Hiroyuki Sano, Junio C Hamano, Eric Sunshine
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 3/3][GSOC] fsck: replace if-statements to logical expressions
2014-03-19 11:23 [PATCH v2 1/3][GSOC] diff: rename read_directory() to get_path_list() Hiroyuki Sano
2014-03-19 11:23 ` [PATCH v2 2/3][GSOC] diff: use is_dot_or_dotdot() instead of strcmp() Hiroyuki Sano
@ 2014-03-19 11:23 ` Hiroyuki Sano
2014-03-19 18:29 ` Junio C Hamano
2014-03-19 21:15 ` [PATCH v2 1/3][GSOC] diff: rename read_directory() to get_path_list() Eric Sunshine
2 siblings, 1 reply; 6+ messages in thread
From: Hiroyuki Sano @ 2014-03-19 11:23 UTC (permalink / raw)
To: git; +Cc: Hiroyuki Sano, Junio C Hamano, Eric Sunshine
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 3/3][GSOC] fsck: replace if-statements to logical expressions
2014-03-19 11:23 ` [PATCH v2 3/3][GSOC] fsck: replace if-statements to logical expressions Hiroyuki Sano
@ 2014-03-19 18:29 ` Junio C Hamano
0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2014-03-19 18:29 UTC (permalink / raw)
To: Hiroyuki Sano; +Cc: git, Eric Sunshine
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);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/3][GSOC] diff: rename read_directory() to get_path_list()
2014-03-19 11:23 [PATCH v2 1/3][GSOC] diff: rename read_directory() to get_path_list() Hiroyuki Sano
2014-03-19 11:23 ` [PATCH v2 2/3][GSOC] diff: use is_dot_or_dotdot() instead of strcmp() Hiroyuki Sano
2014-03-19 11:23 ` [PATCH v2 3/3][GSOC] fsck: replace if-statements to logical expressions Hiroyuki Sano
@ 2014-03-19 21:15 ` Eric Sunshine
2 siblings, 0 replies; 6+ messages in thread
From: Eric Sunshine @ 2014-03-19 21:15 UTC (permalink / raw)
To: Hiroyuki Sano; +Cc: Git List, Junio C Hamano
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/3][GSOC] diff: use is_dot_or_dotdot() instead of strcmp()
2014-03-19 11:23 ` [PATCH v2 2/3][GSOC] diff: use is_dot_or_dotdot() instead of strcmp() Hiroyuki Sano
@ 2014-03-19 21:15 ` Eric Sunshine
0 siblings, 0 replies; 6+ messages in thread
From: Eric Sunshine @ 2014-03-19 21:15 UTC (permalink / raw)
To: Hiroyuki Sano; +Cc: Git List, Junio C Hamano
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-03-19 21:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-03-19 11:23 [PATCH v2 1/3][GSOC] diff: rename read_directory() to get_path_list() Hiroyuki Sano
2014-03-19 11:23 ` [PATCH v2 2/3][GSOC] diff: use is_dot_or_dotdot() instead of strcmp() Hiroyuki Sano
2014-03-19 21:15 ` Eric Sunshine
2014-03-19 11:23 ` [PATCH v2 3/3][GSOC] fsck: replace if-statements to logical expressions Hiroyuki Sano
2014-03-19 18:29 ` Junio C Hamano
2014-03-19 21:15 ` [PATCH v2 1/3][GSOC] diff: rename read_directory() to get_path_list() Eric Sunshine
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).