Re: [PATCH v2 3/3][GSOC] fsck: replace if-statements to logical expressions
Hiroyuki Sano <sh19910711@gmail.com> writes:
Show 5 quoted lines
> 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...
Show 45 quoted lines
> 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);
Hiroyuki Sano
