Re: [GSOC][PATCH 1/2] modified dir-iterator.c

[Thomas Gummerer <t.gummerer@gmail.com>]

Hi,

thanks for the patch!

Subject: [GSOC][PATCH 1/2] modified dir-iterator.c

Commit messages should be written in the imperative mood, e.g. "modify
dir-iterator.c", see also "Documentation/SubmittingPatches.  Also they
are generally in the format "<area>: <description>".  So the subject
here could be something like "dir-iterator: replace closedir with
cast".  Note that the description briefly describes what the patch
does, in more detail than just "modify".  Every patch modifies
something, so that word by itself is somewhat meaningless.

Also you are using 1/2 in the subject, but I can't find a second patch
on the mailing list.  Did you forget to send that?

On 03/08, sushmaunnibhavi wrote:
> ---
> Some places in git use raw API opendir/readdir/closedir to traverse a directory recursively, which usually involves function recursion. Now that we have struct dir_iterator,we have to convert these to use the dir-iterator to simplify the code.

This is a plain copy from the microprojects page, but doesn't explain
what this change is about.  The commit message should explain why a
certain change is made, so reviewers can easily understand why the
patch would be worth including.

Note that this explaination should also be part of the commit message
(before the --- marker above), so it is included in the project
history.  The space after the --- marker can be useful for giving
additional information to reviewers, that should not be included in
the projects commit history.

> Signed-off-by: Sushma Unnibhavi <sushmaunnibhavi425@gmail.com>

The Signed-off-by: line should be part of the commit message (before
the ---).  Also it should match the author name used in the commit.
Right now you are using two different names.  We generally prefer real
names over nicknames, so the author of the commit should be updated to
be "Sushma Unnibhavi".

>  dir-iterator.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/dir-iterator.c b/dir-iterator.c
> index f2dcd82fde..a3b5b8929c 100644
> --- a/dir-iterator.c
> +++ b/dir-iterator.c
> @@ -169,7 +169,7 @@ int dir_iterator_abort(struct dir_iterator *dir_iterator)
>  		struct dir_iterator_level *level =
>  			&iter->levels[iter->levels_nr - 1];
>  
> -		if (level->dir && closedir(level->dir)) {
> +		if (level->dir && (struct dir_iterator_int *)(level->dir)) {//changed closedir to (struct dir_iterator_int *)

The 'closedir' call is changed to a cast here, which will always
evaluate to true as long as level->dir is not a NULL pointer, which we
already check in the first condition.

After this change we no longer close the directory in
'dir_iterator_abort', which we should still do.  Also the warning
message below is now no longer correct, if this would be the change
that is really desired.

The comment that's added here tells us what has been done in this
particular commit, but is not very useful in the longer term (someone
reading this line tomorrow or in a year would not care that this has
been changed in some previous commit).  The actual change is already
easily understandable from the diff, so the comment adds no additional
benefit.  If it would add some benefit to the reader, it would be
better to describe that in the commit message rather than in a
comment.

Additionally we don't use C++ style comments in this project, but
rather prefer C style comments using /* */ to wrap it.

>  			strbuf_setlen(&iter->base.path, level->prefix_len);
>  			warning("error closing directory %s: %s",
>  				iter->base.path.buf, strerror(errno));
> -- 
> 2.17.1
>