Re: [PATCH] microproject for GSOC
- From
Matthieu Moy <matthieu.moy@grenoble-inp.fr>
- Date
- Mar 14, 2014, 17:10 UTC
- Message-ID
- <vpq4n30aeou.fsf@anie.imag.fr>
- In-Reply-To
- <1394815367-9706-1-git-send-email-ubuntu2012@126.com>
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 <ubuntu2012@126.com> 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 ;-).
Show 6 quoted lines
> - 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/