Re: [GSoC][PATCH 1/3] dir-iterator: add pedantic option to dir_iterator_begin

[Christian Couder <christian.couder@gmail.com>]

On Sat, Feb 23, 2019 at 10:37 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
>
> On 02/23, Matheus Tavares wrote:
> > Add the pedantic option to dir-iterator's initialization function,
> > dir_iterator_begin. When this option is set to true,
> > dir_iterator_advance will immediately return ITER_ERROR when failing to
> > fetch the next entry. When set to false, dir_iterator_advance will emit
> > a warning and keep looking for the next entry.
> >
> > Also adjust refs/files-backend.c to the new dir_iterator_begin
> > signature.
>
> Thanks, this makes sense to me.  This commit message describes what we
> are doing in this patch, but not why we are doing it.  From previously
> reviewing this series, I know this is going to be used in a subsequent
> patch, but it is nice to give reviewers and future readers of this
> patch that context as well.  Just something like "This behaviour is
> going to be used in a subsequent patch." should be enough here.

I agree that it's a good idea to add just that.

> >  int dir_iterator_advance(struct dir_iterator *dir_iterator)
> > @@ -71,6 +78,8 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
> >
> >                       level->dir = opendir(iter->base.path.buf);
> >                       if (!level->dir && errno != ENOENT) {
> > +                             if (iter->pedantic)
> > +                                     goto error_out;
>
> I think we should also print an error here.  The caller doesn't have
> any context on what went wrong, and will probably just 'die()' if an
> error is encountered.  I think it would make sense to call
> 'error(...)' here before 'goto error_out;' to give a useful error
> message here.

If we start to give error messages, then we might as well give error
messages all the times when we error out. This will avoid the callers
wondering if they need to give an error message or not.

I am not sure it's necessary here though. And I think if it's useful,
it can be added in another patch or another patch series.

> >                               warning("error opening directory %s: %s",
> >                                       iter->base.path.buf, strerror(errno));
> >                               /* Popping the level is handled below */

> > -struct dir_iterator *dir_iterator_begin(const char *path)
> > +struct dir_iterator *dir_iterator_begin(const char *path, int pedantic)
>
> Thinking about the future evolution of this interface, it might make
> more sense to have that second parameter be a "struct
> dir_iterator_opts".  For now it would just have one member "pedantic",
> but in the future we could add additional options there instead of
> adding additional parameters.

I think it's ok with `int pedantic` for now as improvements can be
done when they are really needed. And we will perhaps find out that
it's better to just change `int pedantic` to `unsigned flags` instead
of `struct dir_iterator_opts`.

Thanks,
Christian.