On Sun, Apr 10 2022, Plato Kiorpelidis wrote:
> Simplify dir_iterator_advance by switch from iterative to recursive
> implementation. In each recursive step one action is performed.
>
> This makes dir-iterator easier to work with, understand and introduce
> new functionality.
>
> Signed-off-by: Plato Kiorpelidis <kioplato@xxxxxxxxx>
> ---
> dir-iterator.c | 225 ++++++++++++++++++++++++++--------------
> t/t0066-dir-iterator.sh | 4 +-
> 2 files changed, 148 insertions(+), 81 deletions(-)
>
> diff --git a/dir-iterator.c b/dir-iterator.c
> index b17e9f970a..0f9ed95958 100644
> --- a/dir-iterator.c
> +++ b/dir-iterator.c
> @@ -7,8 +7,7 @@ struct dir_iterator_level {
> DIR *dir;
>
> /*
> - * The length of the directory part of path at this level
> - * (including a trailing '/'):
> + * The length of the directory part of path at this level.
> */
> size_t prefix_len;
> };
> @@ -34,8 +33,9 @@ struct dir_iterator_int {
> size_t levels_alloc;
>
> /*
> - * A stack of levels. levels[0] is the uppermost directory
> - * that will be included in this iteration.
> + * A stack of levels. levels[0] is the root directory.
> + * It won't be included in the iteration, but iteration will happen
> + * inside it's subdirectories.
> */
> struct dir_iterator_level *levels;
>
> @@ -45,34 +45,53 @@ struct dir_iterator_int {
>
> /*
> * Push a level in the iter stack and initialize it with information from
> - * the directory pointed by iter->base->path. It is assumed that this
> - * strbuf points to a valid directory path. Return 0 on success and -1
> - * otherwise, setting errno accordingly and leaving the stack unchanged.
> + * the directory pointed by iter->base->path. Don't open the directory.
> + *
> + * Return 1 on success.
> + * Return 0 when `iter->base->path` isn't a directory.
> */
> static int push_level(struct dir_iterator_int *iter)
> {
> struct dir_iterator_level *level;
>
> + if (!S_ISDIR(iter->base.st.st_mode)) return 0;
style: missing \n before "return".
Also, the one existing caller before this patch is:
if (S_ISDIR(iter->base.st.st_mode) && push_level(iter))
Why not continue to do that?
> +/*
> + * Activate most recent pushed level.
> + *
> + * Return 1 on success.
> + * Return -1 on failure when errno == ENOENT, leaving the stack unchanged.
> + * Return -2 on failure when errno != ENOENT, leaving the stack unchanged.
> + */
> +static int activate_level(struct dir_iterator_int *iter)
> +{
> + struct dir_iterator_level *level = &iter->levels[iter->levels_nr - 1];
> + int saved_errno;
> +
> + if (level->dir)
> + return 1;
> +
> + if ((level->dir = opendir(iter->base.path.buf)) != NULL)
> + return 1;
> +
> + saved_errno = errno;
> + if (errno != ENOENT) {
> + warning_errno("error opening directory '%s'", iter->base.path.buf);
> errno = saved_errno;
> - return -1;
> + return -2;
Perhaps we should just add an enum for these return values instead of
adding more negative/positive values here. That makes your later calls
of activate_level() more idiomaic. E.g. !activate_level() instead of
activate_level() == 1.
> warning_errno("failed to stat '%s'", iter->base.path.buf);
> + return -2; // Stat failed not with ENOENT.
Don't use // comments, use /* .. */
> + } else if (stat_err && errno == ENOENT)
> + return -1; // Stat failed with ENOENT.
Missing {} here for the else if.
> + struct dir_iterator_int *iter = (struct dir_iterator_int *)dir_iterator;
> + struct dir_iterator_level *level = &iter->levels[iter->levels_nr - 1];
> +
> + struct dirent *dir_entry = NULL;
> +
> + int expose_err, activate_err;
> +
> + /* For shorter code width-wise, more readable */
> + unsigned int PEDANTIC = iter->flags & DIR_ITERATOR_PEDANTIC;
We usually don't add \n\n in the middle of variable decls.
> +
> + /*
> + * Attempt to open the directory of the last level if not opened yet.
> + *
> + * Remember that we ignore ENOENT errors so that the user of this API
> + * can remove entries between calls to `dir_iterator_advance()`.
> + * We care for errors other than ENOENT only when PEDANTIC is enabled.
> + */
> +
> + activate_err = activate_level(iter);
> +
> + if (activate_err == -2 && PEDANTIC)
> + goto error_out;
> + else if (activate_err == -2 || activate_err == -1) {
> + /*
> + * We activate the root level at `dir_iterator_begin()`.
> + * Therefore, there isn't any case to run out of levels.
> + */
> +
> + --iter->levels_nr;
> +
> + return dir_iterator_advance(dir_iterator);
> }
>
> - /* Loop until we find an entry that we can give back to the caller. */
> - while (1) {
> - struct dirent *de;
> - struct dir_iterator_level *level =
> - &iter->levels[iter->levels_nr - 1];
> + strbuf_setlen(&iter->base.path, level->prefix_len);
> +
> + errno = 0;
> + dir_entry = readdir(level->dir);
>
> - strbuf_setlen(&iter->base.path, level->prefix_len);
> - errno = 0;
> - de = readdir(level->dir);
> -
> - if (!de) {
> - if (errno) {
> - warning_errno("error reading directory '%s'",
> - iter->base.path.buf);
> - if (iter->flags & DIR_ITERATOR_PEDANTIC)
> - goto error_out;
> - } else if (pop_level(iter) == 0) {
> + if (dir_entry == NULL) {
Don't compare against NULL, use !dir_entry.
Also: Manually resetting "errno" before isn't needed. It will be (re)set
if readdir() returns NULL().
> + if (errno) {
> + warning_errno("errno reading dir '%s'", iter->base.path.buf);
> + if (iter->flags & DIR_ITERATOR_PEDANTIC) goto error_out;
more missing \n before goto/return.
