git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [RFC PATCH 0/6][GSoC] iterate dirs before or after their contents

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Apr 11, 2022, 13:37 UTC
Message-ID
<df287d4f-e9da-4ce0-d7e9-1b1fe7671aab@gmail.com>
In-Reply-To
<20220410111852.2097418-1-kioplato@gmail.com>
Hi Plato

I think this would be a useful addition to git, thanks for working on it. I've left some comments on the individual patches. In general I think this series would benefit from trying to work with the existing code and change one aspect at a time in stages rather than rewriting large chunks all at once.

Best Wishes
Phillip
On 10/04/2022 12:18, Plato Kiorpelidis wrote:
Show 77 quoted lines
> Hello. Some time ago I worked on converting opendir/readdir calls to use
> dir-iterator [1], however this cleanup required to iterate directory paths after
> their contents. Matheus pointed me out to a previous patch series[2] that
> attempted to implement such functionality in dir-iterator. From it, I mainly
> used Michael's feedback and feature requests and tried to include it in my work.
> 
> My fork: https://github.com/kioplato/git/tree/dir-iterator
> CI: https://github.com/kioplato/git/actions/runs/2141288008
> There are some memleaks, I'll track them down in v2.
> 
> I aim to implement more functionality in dir-iterator, my goal being to simplify
> the codebase by introducing an abstraction layer for iterating directories.
> I would like to eventually simplify read_directory_recursive(). I wanted to
> check in with you to make sure I'm heading in the right direction with what I've
> implemented.
>    * Are my tests overly exhaustive?
>    * As of now we can't thoroughly test dir-iterator on directories with complex
>    structure since readdir produce dirents with undefined order in a directory.
>    I thought about introducing a tool for generating permutations with stable
>    parts in test-lib. Is there a need to something like this for other tests?
>    Or maybe should I sort each level iterated by dir-iterator inside
>    test-dir-iterator before printing to stdout? In these patches I did enumerate
>    the path permutations for some tests by hand, but that's not viable really.
>    * We also don't test for deleted entries between dir_iterator_advance() calls.
>    * Are my comments too much? Throughout git, .c files don't have many comments,
>    should I remove mine as well? I think they provide better context when reading
>    through the source code.
> 
> I do understand that it probably is too early to worry about most of these.
> However I wanted to communicate my thoughts and setup for the following
> versions.
> 
> While I wait for review, I'll implement/fix:
>    * DIR_ITERATOR_RECURSE proposed here[3], but with finer control. Instead of a
>    flag I'll introduce a new integer parameter in dir_iterator_begin(), which
>    will specify the maximum iteration depth.
>      * Supplying 0 will have the same behavior as DIR_ITERATOR_RECURSE i.e. it
>      will iterate over the entries of the root directory.
>      * Supplying -1 will iterate to maximum depth. This is the default right now.
>    * DIR_ITERATOR_DIRS_ONLY proposed here[4]. Enabling this, dir-iterator will
>    show only directories. Failing to enable DIR_ITERATOR_DIRS_BEFORE and/or
>    DIR_ITERATOR_DIRS_AFTER will result in dir_iterator_begin() returning NULL.
>    Is this a good way to encode "show only directories" in the flags?
> 
> I'll include them along with feedback and suggestions from this version in the
> next one.
> 
> I didn't refactor entry.c to use dir-iterator. It's a good first issue for
> someone else to become introduced to the community. I applied my patch[1] and it
> does not pass t/, as it used to, because of 15th test in t1092. Should I work on
> entry.c in my next version or leave it alone for a newcomer?
> 
> This serves as my microproject for GSoC. Could my future work on dir-iterator
> and cleanup of read_directory_recursive() and other customers of dir-iterator
> become a seperate GSoC project I could undertake?
> 
> [1]: https://public-inbox.org/git/20191208180439.19018-1-otalpster@gmail.com/
> [2]: https://public-inbox.org/git/1493226219-33423-1-git-send-email-bnmvco@gmail.com/
> [3]: https://public-inbox.org/git/CACsJy8DBa-oH3i+5P=iVr9NhJwsicZ43DO89WmvpYEQu90RrMw@mail.gmail.com/
> [4]: https://public-inbox.org/git/xmqqmvc265hk.fsf@gitster.mtv.corp.google.com/
> 
> Plato Kiorpelidis (6):
>    t0066: improve readablity of dir-iterator tests
>    t0066: better test coverage for dir-iterator
>    dir-iterator: refactor dir_iterator_advance()
>    dir-iterator: iterate dirs before or after their contents
>    t0066: remove redundant tests
>    test-dir-iterator: handle EACCES errno by dir-iterator
> 
>   builtin/clone.c              |    4 +-
>   dir-iterator.c               |  302 ++++++---
>   dir-iterator.h               |   34 +-
>   refs/files-backend.c         |    2 +-
>   t/helper/test-dir-iterator.c |   23 +-
>   t/t0066-dir-iterator.sh      | 1202 +++++++++++++++++++++++++++++++---
>   6 files changed, 1371 insertions(+), 196 deletions(-)
> 
Previous: Plato KiorpelidisNext: Plato Kiorpelidis
Message 21 of 22 in “[GSoC] iterate dirs before or after their contents”
  1. 0/6 [GSoC] iterate dirs before or after their contentsPlato Kiorpelidis, Apr 10, 2022
  2. 1/6 t0066: improve readablity of dir-iterator testsPlato Kiorpelidis, Apr 10, 2022
  3. Phillip WoodApr 11, 2022
  4. Plato KiorpelidisApr 24, 2022
  5. 2/6 t0066: better test coverage for dir-iteratorPlato Kiorpelidis, Apr 10, 2022
  6. 3/6 dir-iterator: refactor dir_iterator_advance()Plato Kiorpelidis, Apr 10, 2022
  7. Ævar Arnfjörð BjarmasonApr 11, 2022
  8. Phillip WoodApr 11, 2022
  9. Plato KiorpelidisApr 27, 2022
  10. Phillip WoodApr 11, 2022
  11. Plato KiorpelidisApr 27, 2022
  12. 5/6 t0066: remove redundant testsPlato Kiorpelidis, Apr 10, 2022
  13. Ævar Arnfjörð BjarmasonApr 11, 2022
  14. Plato KiorpelidisApr 27, 2022
  15. 4/6 dir-iterator: iterate dirs before or after their contentsPlato Kiorpelidis, Apr 10, 2022
  16. Phillip WoodApr 11, 2022
  17. Plato KiorpelidisApr 27, 2022
  18. 6/6 test-dir-iterator: handle EACCES errno by dir-iteratorPlato Kiorpelidis, Apr 10, 2022
  19. Ævar Arnfjörð BjarmasonApr 11, 2022
  20. Plato KiorpelidisApr 27, 2022
  21. Phillip WoodApr 11, 2022
  22. Plato KiorpelidisApr 19, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.