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

[Plato Kiorpelidis <kioplato@gmail.com>]

On 22/04/11 01:11PM, Ævar Arnfjörð Bjarmason wrote:
> 
> 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>
> > ---
> >
> > [...]
> >
> > @@ -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?

In this patch I also fixed a problem that's subtle. Previously, when we called
dir_iterator_begin() and the specified path is invalid the call won't return
NULL. Instead the call succeeds and the first call to dir_iterator_advance()
fails. I believe that's unexpected. The expected behavior would be to return
NULL from dir_iterator_begin() if the specified path is invalid. Successful call
to dir_iterator_begin() suggests that the root path is valid.

To deal with that I introduced two states for the most recent directory. The
first is just pushed into the levels stack and the other pushed and activated.
This way we can "activate" the root directory in dir_iterator_begin(), but we
also need to reorder the calls to push_level() and activate_level() as a result.
We need to push directories after we read them and activate the most recent one
when dir_iterator_advance() is called.

I'll document that change in the related commit in v2.

Do you have any objection about this change?

> > +/*
> > + * 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.

Yes, I agree. It was bothering me while I was writing these parts. I'm happy you
suggested a way to make this cleaner by using an enum.

> >  		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.
> 
> > [...]
> >
> > -		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.
> 
> [...]
> 
> > +		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.

I'll fix the coding style in these parts in v2. I'm sorry about that.