Re: [WIP RFC PATCH v2 1/5] dir-iterator: add flags parameter to dir_iterator_begin

[Duy Nguyen <pclouds@gmail.com>]

On Tue, Feb 26, 2019 at 12:18 PM Matheus Tavares
<matheus.bernardino@usp.br> wrote:
>  int dir_iterator_advance(struct dir_iterator *dir_iterator)
>  {
>         struct dir_iterator_int *iter =
>                 (struct dir_iterator_int *)dir_iterator;
> +       int ret;
>
>         while (1) {
>                 struct dir_iterator_level *level =
> @@ -71,6 +75,8 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
>
>                         level->dir = opendir(iter->base.path.buf);
>                         if (!level->dir && errno != ENOENT) {
> +                               if (iter->flags & DIR_ITERATOR_PEDANTIC)
> +                                       goto error_out;
>                                 warning("error opening directory %s: %s",
>                                         iter->base.path.buf, strerror(errno));
>                                 /* Popping the level is handled below */
> @@ -122,6 +128,8 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
>                         if (!de) {
>                                 /* This level is exhausted; pop up a level. */
>                                 if (errno) {
> +                                       if (iter->flags & DIR_ITERATOR_PEDANTIC)
> +                                               goto error_out;
>                                         warning("error reading directory %s: %s",
>                                                 iter->base.path.buf, strerror(errno));
>                                 } else if (closedir(level->dir))
> @@ -138,11 +146,20 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
>                                 continue;
>
>                         strbuf_addstr(&iter->base.path, de->d_name);
> -                       if (lstat(iter->base.path.buf, &iter->base.st) < 0) {
> -                               if (errno != ENOENT)
> +
> +                       if (iter->flags & DIR_ITERATOR_FOLLOW_SYMLINKS)
> +                               ret = stat(iter->base.path.buf, &iter->base.st);
> +                       else
> +                               ret = lstat(iter->base.path.buf, &iter->base.st);
> +
> +                       if (ret < 0) {
> +                               if (errno != ENOENT) {
> +                                       if (iter->flags & DIR_ITERATOR_PEDANTIC)
> +                                               goto error_out;
>                                         warning("error reading path '%s': %s",
>                                                 iter->base.path.buf,
>                                                 strerror(errno));
> +                               }
>                                 continue;
>                         }
>
> @@ -159,6 +176,10 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
>                         return ITER_OK;
>                 }
>         }
> +
> +error_out:
> +       dir_iterator_abort(dir_iterator);

Should this function call this or leaveit to the caller? The
description says "free resources" which to me sounds like something
the caller should be aware of. Although if double dir_iterator_abort()
has no bad side effects then I guess we can even leave it here.

PS. files-backend.c does call dir_iterator_abort() unconditionally.
Which sounds like this double-abort pattern should be dealt with even
if that call site does not use the pedantic flag (it could later on,
who knows; don't leave traps behind).

> +       return ITER_ERROR;
>  }
>
> diff --git a/dir-iterator.h b/dir-iterator.h
> index 970793d07a..fe9eb9a04b 100644
> --- a/dir-iterator.h
> +++ b/dir-iterator.h
> @@ -6,9 +6,10 @@
>  /*
>   * Iterate over a directory tree.
>   *
> - * Iterate over a directory tree, recursively, including paths of all
> - * types and hidden paths. Skip "." and ".." entries and don't follow
> - * symlinks except for the original path.
> + * With no flags to modify behaviour, iterate over a directory tree,

Nit but I think we can just skip "With no flags to modify behavior". It's given.

> + * recursively, including paths of all types and hidden paths. Skip
> + * "." and ".." entries and don't follow symlinks except for the
> + * original path.
>   *
>   * Every time dir_iterator_advance() is called, update the members of
>   * the dir_iterator structure to reflect the next path in the
> @@ -19,7 +20,7 @@
>   * A typical iteration looks like this:
>   *
>   *     int ok;
> - *     struct iterator *iter = dir_iterator_begin(path);
> + *     struct iterator *iter = dir_iterator_begin(path, 0);
>   *
>   *     while ((ok = dir_iterator_advance(iter)) == ITER_OK) {
>   *             if (want_to_stop_iteration()) {
> @@ -40,6 +41,20 @@
>   * dir_iterator_advance() again.
>   */
>
> +/*
> + * Flags for dir_iterator_begin:
> + *
> + * - DIR_ITERATOR_PEDANTIC: override dir-iterator's default behavior
> + *   in case of an error while trying to fetch the next entry, which is
> + *   to emit a warning and keep going. With this flag, resouces are
> + *   freed and ITER_ERROR is return immediately.
> + *
> + * - DIR_ITERATOR_FOLLOW_SYMLINKS: make dir-iterator follow symlinks to
> + *   directories, i.e., iterate over linked directories' contents.

Do we really need this flag? If dir-iterator does not follow symlinks,
the caller _can_ check stat data to detect symlinks and look inside
anyway. So this flag is more about convenience (_if_ it has more than
one call site, convenience for one call site is just not worth it).

Or is there something else I'm missing?

> + */
> +#define DIR_ITERATOR_PEDANTIC (1 << 0)
> +#define DIR_ITERATOR_FOLLOW_SYMLINKS (1 << 1)
> +
-- 
Duy