From: Phillip Wood <phillip.wood123@gmail.com>
To: Plato Kiorpelidis <kioplato@gmail.com>, git@vger.kernel.org
Cc: matheus.bernardino@usp.br, mhagger@alum.mit.edu
Subject: Re: [RFC PATCH 0/6][GSoC] iterate dirs before or after their contents
Date: Mon, 11 Apr 2022 14:37:00 +0100 [thread overview]
Message-ID: <df287d4f-e9da-4ce0-d7e9-1b1fe7671aab@gmail.com> (raw)
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:
> 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(-)
>
next prev parent reply other threads:[~2022-04-11 13:37 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-10 11:18 [RFC PATCH 0/6][GSoC] iterate dirs before or after their contents Plato Kiorpelidis
2022-04-10 11:18 ` [RFC PATCH 1/6] t0066: improve readablity of dir-iterator tests Plato Kiorpelidis
2022-04-11 13:16 ` Phillip Wood
2022-04-24 19:25 ` Plato Kiorpelidis
2022-04-10 11:18 ` [RFC PATCH 2/6] t0066: better test coverage for dir-iterator Plato Kiorpelidis
2022-04-10 11:18 ` [RFC PATCH 3/6] dir-iterator: refactor dir_iterator_advance() Plato Kiorpelidis
2022-04-11 11:11 ` Ævar Arnfjörð Bjarmason
2022-04-11 13:40 ` Phillip Wood
2022-04-27 15:45 ` Plato Kiorpelidis
2022-04-11 13:26 ` Phillip Wood
2022-04-27 14:32 ` Plato Kiorpelidis
2022-04-10 11:18 ` [RFC PATCH 4/6] dir-iterator: iterate dirs before or after their contents Plato Kiorpelidis
2022-04-11 13:31 ` Phillip Wood
2022-04-27 14:57 ` Plato Kiorpelidis
2022-04-10 11:18 ` [RFC PATCH 5/6] t0066: remove redundant tests Plato Kiorpelidis
2022-04-11 11:10 ` Ævar Arnfjörð Bjarmason
2022-04-27 16:00 ` Plato Kiorpelidis
2022-04-10 11:18 ` [RFC PATCH 6/6] test-dir-iterator: handle EACCES errno by dir-iterator Plato Kiorpelidis
2022-04-11 11:04 ` Ævar Arnfjörð Bjarmason
2022-04-27 17:30 ` Plato Kiorpelidis
2022-04-11 13:37 ` Phillip Wood [this message]
2022-04-19 13:06 ` [RFC PATCH 0/6][GSoC] iterate dirs before or after their contents Plato Kiorpelidis
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=df287d4f-e9da-4ce0-d7e9-1b1fe7671aab@gmail.com \
--to=phillip.wood123@gmail.com \
--cc=git@vger.kernel.org \
--cc=kioplato@gmail.com \
--cc=matheus.bernardino@usp.br \
--cc=mhagger@alum.mit.edu \
--cc=phillip.wood@dunelm.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).