Re: [PATCH] microproject for GSOC

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxx> 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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]