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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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:
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@xxxxxxxxx/
[2]: https://public-inbox.org/git/1493226219-33423-1-git-send-email-bnmvco@xxxxxxxxx/
[3]: https://public-inbox.org/git/CACsJy8DBa-oH3i+5P=iVr9NhJwsicZ43DO89WmvpYEQu90RrMw@xxxxxxxxxxxxxx/
[4]: https://public-inbox.org/git/xmqqmvc265hk.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxx/

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(-)




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux