Re: [PATCH v8 0/5] [GSoC] remove_subtree(): reimplement using iterators

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Apr 17, 2017, 06:29 UTC
Message-ID: <a60b2ed6-2b99-b134-05af-7c8492a6949c@alum.mit.edu>
In-Reply-To: <1491442767-54068-1-git-send-email-bnmvco@gmail.com>

On 04/06/2017 03:39 AM, Daniel Ferreira wrote:

Show 20 quoted lines

> 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=vjhsAh16ANYg@mail.gmail.com/T/#t
> v2: https://public-inbox.org/git/CACsJy8Dxh-QPBBLfaFWPAWUsbA9GVXA7x+mXLjEvYKhk1zOpig@mail.gmail.com/T/#t
> v3: https://public-inbox.org/git/CAGZ79kYtpmURSQWPumobA=e3JBFjKhWCdv_LPhKCd71ZRwMovA@mail.gmail.com/T/#t
> v4: https://public-inbox.org/git/1490747533-89143-1-git-send-email-bnmvco@gmail.com/T/#e437a63e0c22c00c69b5d92977c9b438ed2b9fd3a
> v5: https://public-inbox.org/git/1490844730-47634-1-git-send-email-bnmvco@gmail.com/T/#m2323f15e45de699f2e09364f40a62e17047cf453
> v6: https://public-inbox.org/git/1491107726-21504-1-git-send-email-bnmvco@gmail.com/T/#t
> v7: https://public-inbox.org/git/1491163388-41255-1-git-send-email-bnmvco@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 <mhagger@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].

Show 8 quoted lines

> 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.

Show 5 quoted lines

> 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.mhagger@alum.mit.edu/