Re:Re: [PATCH] Rewrite the diff-no-index.c
thank you for your comments
At 2014-03-17 17:13:38,"Eric Sunshine" wrote:
>Thanks for the resubmission. Comments below...
>
>On Sun, Mar 16, 2014 at 8:44 AM, wrote:
>> From: 沈承恩
>> Subject: [PATCH] Rewrite the diff-no-index.c
>
>This is your second version of the patch, so you should say [PATCH v2].
>
>Most patches rewrite something, so "rewrite" in the subject does not
>convey much. Better would be to explain what the patch does. For
>instance:
>
>Subject: diff-no-index: replace manual "." & ".." check with
>is_dot_or_dotdot()
>
>> I am sorry for that I send this agian.Last patch I have some error.(Maybe
>> this time will like the previous).It is apply for GSOC
>
>This commentary is relevant to the ongoing email conversation but does
>not belong in the commit message, so you should place it below the
>"---" line after your sign-off.
>
>> Signed-off-by: 沈承恩
>> ---
>
>This is where you would place commentary. It is also good etiquette to
>tell reviewers what changed in this version of the patch and to
>provide a link to the previous version, like this [1].
>
>[1]: http://thread.gmane.org/gmane.comp.version-control.git/244093
>
>> diff-no-index.c |5 +++--
>> dir.h |3 ++-
>> 2 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/diff-no-index.c b/diff-no-index.c
>> index 8e10bff..1fb0c0f 100644
>> --- a/diff-no-index.c
>> +++ b/diff-no-index.c
>> @@ -3,13 +3,14 @@
>> * Copyright (c) 2007 by Johannes Schindelin
>> * Copyright (c) 2008 by Junio C Hamano
>> */
>> -
>> +#define EXIT
>
>This change is non-obvious and should be explained in the commit
>message, otherwise reviewers will not understand its purpose.
>
>In fact, you are doing this because you want to omit the declaration
>of read_directory() from dir.h when it is included in this file to
>avoid conflict with the (different) read_directory() implementation in
>this file. This is an ugly way to solve the problem. Renaming
>read_directory() in this file would be a much cleaner solution (but
>should be done as a separate preparatory patch).
>
>> #include "cache.h"
>> #include "color.h"
>> #include "commit.h"
>> #include "blob.h"
>> #include "tag.h"
>> #include "diff.h"
>> +#include "dir.h"
>> #include "diffcore.h"
>> #include "revision.h"
>> #include "log-tree.h"
>> @@ -25,7 +26,7 @@ static int read_directory(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))
>
>This logic is backward. Keep in mind the return value of strcmp() and
>then think carefully about the expression 'strcmp(...) &&
>strcmp(...)'.
>
>> string_list_insert(list, e->d_name);
>>
>> closedir(dir);
>> diff --git a/dir.h b/dir.h
>> index 55e5345..c0e45c8 100644
>> --- a/dir.h
>> +++ b/dir.h
>> @@ -138,8 +138,9 @@ extern int match_pathspec(const struct pathspec
>> *pathspec,
>> extern int within_depth(const char *name, int namelen, int depth, int
>> max_depth);
>>
>> extern int fill_directory(struct dir_struct *dir, const struct pathspec
>> *pathspec);
>> +#ifndef EXIT
>> extern int read_directory(struct dir_struct *, const char *path, int len,
>> const struct pathspec *pathspec);
>> -
>> +#endif
>
>See above.
>
>> extern int is_excluded_from_list(const char *pathname, int pathlen, const
>> char *basename,
>> int *dtype, struct exclude_list *el);
>> struct dir_entry *dir_add_ignored(struct dir_struct *dir, const char
>> *pathname, int len);
>> --
>> 1.7.9.5
Re:Re: [PATCH] Rewrite the diff-no-index.c
Thank you for your comments.If I rename some function when I work on a large
project like git,whether it will cause other error that I can not solve .So I
use the ugly way for this reason.
At 2014-03-17 17:13:38,"Eric Sunshine" wrote:
>Thanks for the resubmission. Comments below...
>
>On Sun, Mar 16, 2014 at 8:44 AM, wrote:
>> From: 沈承恩
>> Subject: [PATCH] Rewrite the diff-no-index.c
>
>This is your second version of the patch, so you should say [PATCH v2].
>
>Most patches rewrite something, so "rewrite" in the subject does not
>convey much. Better would be to explain what the patch does. For
>instance:
>
>Subject: diff-no-index: replace manual "." & ".." check with
>is_dot_or_dotdot()
>
>> I am sorry for that I send this agian.Last patch I have some error.(Maybe
>> this time will like the previous).It is apply for GSOC
>
>This commentary is relevant to the ongoing email conversation but does
>not belong in the commit message, so you should place it below the
>"---" line after your sign-off.
>
>> Signed-off-by: 沈承恩
>> ---
>
>This is where you would place commentary. It is also good etiquette to
>tell reviewers what changed in this version of the patch and to
>provide a link to the previous version, like this [1].
>
>[1]: http://thread.gmane.org/gmane.comp.version-control.git/244093
>
>> diff-no-index.c |5 +++--
>> dir.h |3 ++-
>> 2 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/diff-no-index.c b/diff-no-index.c
>> index 8e10bff..1fb0c0f 100644
>> --- a/diff-no-index.c
>> +++ b/diff-no-index.c
>> @@ -3,13 +3,14 @@
>> * Copyright (c) 2007 by Johannes Schindelin
>> * Copyright (c) 2008 by Junio C Hamano
>> */
>> -
>> +#define EXIT
>
>This change is non-obvious and should be explained in the commit
>message, otherwise reviewers will not understand its purpose.
>
>In fact, you are doing this because you want to omit the declaration
>of read_directory() from dir.h when it is included in this file to
>avoid conflict with the (different) read_directory() implementation in
>this file. This is an ugly way to solve the problem. Renaming
>read_directory() in this file would be a much cleaner solution (but
>should be done as a separate preparatory patch).
>
>> #include "cache.h"
>> #include "color.h"
>> #include "commit.h"
>> #include "blob.h"
>> #include "tag.h"
>> #include "diff.h"
>> +#include "dir.h"
>> #include "diffcore.h"
>> #include "revision.h"
>> #include "log-tree.h"
>> @@ -25,7 +26,7 @@ static int read_directory(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))
>
>This logic is backward. Keep in mind the return value of strcmp() and
>then think carefully about the expression 'strcmp(...) &&
>strcmp(...)'.
>
>> string_list_insert(list, e->d_name);
>>
>> closedir(dir);
>> diff --git a/dir.h b/dir.h
>> index 55e5345..c0e45c8 100644
>> --- a/dir.h
>> +++ b/dir.h
>> @@ -138,8 +138,9 @@ extern int match_pathspec(const struct pathspec
>> *pathspec,
>> extern int within_depth(const char *name, int namelen, int depth, int
>> max_depth);
>>
>> extern int fill_directory(struct dir_struct *dir, const struct pathspec
>> *pathspec);
>> +#ifndef EXIT
>> extern int read_directory(struct dir_struct *, const char *path, int len,
>> const struct pathspec *pathspec);
>> -
>> +#endif
>
>See above.
>
>> extern int is_excluded_from_list(const char *pathname, int pathlen, const
>> char *basename,
>> int *dtype, struct exclude_list *el);
>> struct dir_entry *dir_add_ignored(struct dir_struct *dir, const char
>> *pathname, int len);
>> --
>> 1.7.9.5
Re:Re: [PATCH] microproject for GSOC
Thank you for your comments.I will amend those issues .As a Chinese
student,what name should I use?My Chinese name is ok?
At 2014-03-15 01:10:57,"Matthieu Moy" wrote:
>Hi,
>
>Welcome to the Git community, and welcome to the GSOC program. Below are
>some comments to give you a taste of what a review looks like on this
>list. Do take the comments seriously (they should be addressed), but
>don't take them badly: critic is meant to be constructive.
>
>ubuntu733 writes:
>^
>
>Please, use a real name when you contribute to Git.
>
>> Apply for GSOC.The microprojects is rewriter diff-index.c
>
>This part of your message will become the commit message (i.e. cast in
>stone forever in git.git's history). The point is not that you want to
>apply for GSOC, but what the patch does and more importantly why it does
>it.
>
>> +#define REMOVE 1
>
>If the code is to be removed, then remove it. That's why we use a
>version control system ;-).
>
>> -while ((e = readdir(dir)))
>> -if (strcmp(".", e->d_name) && strcmp("..", e->d_name))
>> -string_list_insert(list, e->d_name);
>> -
>> +while ((e = readdir(dir))) {
>> +while(is_dot_or_dotdot(e->d_name))
>
>Missing space between "while" and "(".
>
>> + break;
>
>Broken indentation (indent with space).
>
>This while (...) break; seems really weird to me: if the condition is
>false, then you exit the loop because it's a while loop, and if the
>condition is true, you exit the loop because of the break. Isn't that a
>no-op?
>
>> +string_list_insert(list, e->d_name);
>> + }
>
>Broken indentation (misplaced }).
>
>--
>Matthieu Moy
>http://www-verimag.imag.fr/~moy/
Re:Re: [PATCH] microproject for GSOC
Thank you for your comments.I will amend those issues .As a Chinese
student,what name should I use?My Chinese name is ok?
I am very interest in Open source.What can I do to increase my chance?
At 2014-03-15 01:10:57,"Matthieu Moy" wrote:
>Hi,
>
>Welcome to the Git community, and welcome to the GSOC program. Below are
>some comments to give you a taste of what a review looks like on this
>list. Do take the comments seriously (they should be addressed), but
>don't take them badly: critic is meant to be constructive.
>
>ubuntu733 writes:
>^
>
>Please, use a real name when you contribute to Git.
>
>> Apply for GSOC.The microprojects is rewriter diff-index.c
>
>This part of your message will become the commit message (i.e. cast in
>stone forever in git.git's history). The point is not that you want to
>apply for GSOC, but what the patch does and more importantly why it does
>it.
>
>> +#define REMOVE 1
>
>If the code is to be removed, then remove it. That's why we use a
>version control system ;-).
>
>> -while ((e = readdir(dir)))
>> -if (strcmp(".", e->d_name) && strcmp("..", e->d_name))
>> -string_list_insert(list, e->d_name);
>> -
>> +while ((e = readdir(dir))) {
>> +while(is_dot_or_dotdot(e->d_name))
>
>Missing space between "while" and "(".
>
>> + break;
>
>Broken indentation (indent with space).
>
>This while (...) break; seems really weird to me: if the condition is
>false, then you exit the loop because it's a while loop, and if the
>condition is true, you exit the loop because of the break. Isn't that a
>no-op?
>
>> +string_list_insert(list, e->d_name);
>> + }
>
>Broken indentation (misplaced }).
>
>--
>Matthieu Moy
>http://www-verimag.imag.fr/~moy/
