Re: [RFC PATCH 3/6] dir-iterator: refactor dir_iterator_advance()

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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@gmail.com>
> ---
>  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.