On 04/06/2017 03:39 AM, Daniel Ferreira wrote:
> This is the seventh version of a patch series that implements the GSoC
> microproject of converting a recursive call to readdir() to use dir_iterator.
>
> v1:
> https://public-inbox.org/git/CAGZ79kZwT-9mHTiOJ5CEjk2wDFkn6+NcogjX0=vjhsah16a...@mail.gmail.com/T/#t
> v2:
> https://public-inbox.org/git/cacsjy8dxh-qpbblfafwpawusba9gvxa7x+mxljevykhk1zo...@mail.gmail.com/T/#t
> v3:
> https://public-inbox.org/git/CAGZ79kYtpmURSQWPumobA=e3jbfjkhwcdv_lphkcd71zrwm...@mail.gmail.com/T/#t
> v4:
> https://public-inbox.org/git/1490747533-89143-1-git-send-email-bnm...@gmail.com/T/#e437a63e0c22c00c69b5d92977c9b438ed2b9fd3a
> v5:
> https://public-inbox.org/git/1490844730-47634-1-git-send-email-bnm...@gmail.com/T/#m2323f15e45de699f2e09364f40a62e17047cf453
> v6:
> https://public-inbox.org/git/1491107726-21504-1-git-send-email-bnm...@gmail.com/T/#t
> v7:
> https://public-inbox.org/git/1491163388-41255-1-git-send-email-bnm...@gmail.com/T/#t
>
> Travis CI build: https://travis-ci.org/theiostream/git/jobs/219111182
>
> In this version, I applied pretty much all suggestions Michael and Stefan had
> for the patch.
>
> Michael, regarding the patch you sent for parsing the arguments on the
> test-dir-iterator helper, I did not squash because it'd generate too many
> merge conflicts. I just preferred add your code manually. Let me know how I
> can properly credit you for it.
It's a small enough bit of code that you don't have to credit me.
However, technically you should add a
Signed-off-by: Michael Haggerty <mhag...@alum.mit.edu>
line to your patch that includes my code, which you are allowed to do
because I included that line in the email where I suggested the change [1].
> The only "controversial" part of this code is how I ended up caching the
> result
> of lstat() on the dir_iterator_level struct. Having to handle the case of the
> root directory ended up making set_iterator_data() way more complicated
> (having
> to handle the case of level->stat not being set by push_dir_level()), as well
> as required the introduction of st_is_set member in the level struct. This
> issue
> would be solved if we could lstat() the root dir on dir_iterator_begin() and
> possibly return an error there. Regardless, I appreciate other suggestions to
> make this less complex.
I agree that this adds a lot of code complexity. But I think your
suggestion to move the initial `lstat()` call to `dir_iterator_begin()`
is a good one. It could return NULL on error (being careful not to leak
memory of course). It should be careful to leave `errno` set
appropriately, which would allow the caller to see why it failed (i.e.,
because the directory didn't exist, or the path was not a directory, or
whatever). In that case, probably `dir_iterator_begin()` could call
`push_dir_level()`, eliminating a bit of repeated code at the same time.
This would also fix an anomaly in the current code: currently, if the
top-level "directory" is not a directory, it is nevertheless reported
back to the caller during `DIR_ITERATOR_PRE_ORDER_TRAVERSAL` and *again*
during `DIR_ITERATOR_POST_ORDER_TRAVERSAL`. Instead,
`dir_iterator_begin()` would report an error and the caller would not be
misled.
> Hey there! I'm sorry for bothering you, but any chance you might have
> overlooked this patch for a review? (I'm just not familiar with how
> long patches usually take to be reviewed, and since I always got an
> answer within two days of sending it I wondered if you may have just
> not noticed it.)
It's pretty common for response times to vary wildly in open source
(Junio being a notable exception). People often work on open source in
their spare time, and disappear and reappear without notice based on
what else is going on in their lives. For this reason, it is really
important to submit changes in small batches as early as possible
(especially in GSoC) and continue working on the next batch while
waiting for review on earlier batches. If you find yourself with nothing
else to work on, maybe spend some time reviewing *other* people's
patches, which is also a valuable contribution.
That being said, if you are waiting for a response from particular
person, after a wait it is OK to write them an email asking if they will
have time to get to it (realizing that the answer might be "no").
Probably such an email should be to the person directly, and not CCed to
the mailing list.
Michael
[1]
http://public-inbox.org/git/bfd5d9a07139dbc6eb1fd1dc82ffb38dbbefee1e.1491286711.git.mhag...@alum.mit.edu/
