{"thread":{"id":"50694","subject":"[GSOC][PATCH 1/2] modified dir-iterator.c","startedAt":"2019-03-08T14:03:17Z","lastAt":"2019-03-09T16:12:42Z","messageCount":2,"participants":["sushmaunnibhavi","Thomas Gummerer"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"370987","messageId":"20190308140307.GA22661@hacker-queen","threadId":"50694","inReplyTo":null,"subject":"[GSOC][PATCH 1/2] modified dir-iterator.c","fromName":"sushmaunnibhavi","fromEmail":"sushmaunnibhavi425@gmail.com","sentAt":"2019-03-08T14:03:07Z","receivedAt":"2019-03-08T14:03:17Z","isPatch":true,"sender":{"key":"sushmaunnibhavi425@gmail.com","avatar":null},"body":"---\nSome 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. \n\nSigned-off-by: Sushma Unnibhavi <sushmaunnibhavi425@gmail.com>\n dir-iterator.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/dir-iterator.c b/dir-iterator.c\nindex f2dcd82fde..a3b5b8929c 100644\n--- a/dir-iterator.c\n+++ b/dir-iterator.c\n@@ -169,7 +169,7 @@ int dir_iterator_abort(struct dir_iterator *dir_iterator)\n \t\tstruct dir_iterator_level *level =\n \t\t\t&iter->levels[iter->levels_nr - 1];\n \n-\t\tif (level->dir && closedir(level->dir)) {\n+\t\tif (level->dir && (struct dir_iterator_int *)(level->dir)) {//changed closedir to (struct dir_iterator_int *)\n \t\t\tstrbuf_setlen(&iter->base.path, level->prefix_len);\n \t\t\twarning(\"error closing directory %s: %s\",\n \t\t\t\titer->base.path.buf, strerror(errno));\n-- \n2.17.1\n\n"},{"id":"371033","messageId":"20190309161226.GA31533@hank.intra.tgummerer.com","threadId":"50694","inReplyTo":"20190308140307.GA22661@hacker-queen","subject":"Re: [GSOC][PATCH 1/2] modified dir-iterator.c","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-03-09T16:12:26Z","receivedAt":"2019-03-09T16:12:42Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Hi,\n\nthanks for the patch!\n\nSubject: [GSOC][PATCH 1/2] modified dir-iterator.c\n\nCommit messages should be written in the imperative mood, e.g. \"modify\ndir-iterator.c\", see also \"Documentation/SubmittingPatches.  Also they\nare generally in the format \"<area>: <description>\".  So the subject\nhere could be something like \"dir-iterator: replace closedir with\ncast\".  Note that the description briefly describes what the patch\ndoes, in more detail than just \"modify\".  Every patch modifies\nsomething, so that word by itself is somewhat meaningless.\n\nAlso you are using 1/2 in the subject, but I can't find a second patch\non the mailing list.  Did you forget to send that?\n\nOn 03/08, sushmaunnibhavi wrote:\n> ---\n> 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.\n\nThis is a plain copy from the microprojects page, but doesn't explain\nwhat this change is about.  The commit message should explain why a\ncertain change is made, so reviewers can easily understand why the\npatch would be worth including.\n\nNote that this explaination should also be part of the commit message\n(before the --- marker above), so it is included in the project\nhistory.  The space after the --- marker can be useful for giving\nadditional information to reviewers, that should not be included in\nthe projects commit history.\n\n> Signed-off-by: Sushma Unnibhavi <sushmaunnibhavi425@gmail.com>\n\nThe Signed-off-by: line should be part of the commit message (before\nthe ---).  Also it should match the author name used in the commit.\nRight now you are using two different names.  We generally prefer real\nnames over nicknames, so the author of the commit should be updated to\nbe \"Sushma Unnibhavi\".\n\n>  dir-iterator.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/dir-iterator.c b/dir-iterator.c\n> index f2dcd82fde..a3b5b8929c 100644\n> --- a/dir-iterator.c\n> +++ b/dir-iterator.c\n> @@ -169,7 +169,7 @@ int dir_iterator_abort(struct dir_iterator *dir_iterator)\n>  \t\tstruct dir_iterator_level *level =\n>  \t\t\t&iter->levels[iter->levels_nr - 1];\n>  \n> -\t\tif (level->dir && closedir(level->dir)) {\n> +\t\tif (level->dir && (struct dir_iterator_int *)(level->dir)) {//changed closedir to (struct dir_iterator_int *)\n\nThe 'closedir' call is changed to a cast here, which will always\nevaluate to true as long as level->dir is not a NULL pointer, which we\nalready check in the first condition.\n\nAfter this change we no longer close the directory in\n'dir_iterator_abort', which we should still do.  Also the warning\nmessage below is now no longer correct, if this would be the change\nthat is really desired.\n\nThe comment that's added here tells us what has been done in this\nparticular commit, but is not very useful in the longer term (someone\nreading this line tomorrow or in a year would not care that this has\nbeen changed in some previous commit).  The actual change is already\neasily understandable from the diff, so the comment adds no additional\nbenefit.  If it would add some benefit to the reader, it would be\nbetter to describe that in the commit message rather than in a\ncomment.\n\nAdditionally we don't use C++ style comments in this project, but\nrather prefer C style comments using /* */ to wrap it.\n\n>  \t\t\tstrbuf_setlen(&iter->base.path, level->prefix_len);\n>  \t\t\twarning(\"error closing directory %s: %s\",\n>  \t\t\t\titer->base.path.buf, strerror(errno));\n> -- \n> 2.17.1\n> \n"}]}