On Sun, Feb 24, 2019 at 5:35 AM Christian Couder
<christian.couder@xxxxxxxxx> wrote:
>
> On Sat, Feb 23, 2019 at 10:37 PM Thomas Gummerer <t.gummerer@xxxxxxxxx> 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.
>
Indeed! Thank you Thomas and Christian. I will be adding this in v3.
> > > 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.
To correctly handle the error, I assumed that the caller wouldn't need
to know which exact function returned an error, as long as it knew it
was a "failure to fetch the next entry" kind of error, which is the
"category" of errors caught with the 'pedantic' option. (currently, it
includes errors on lstat, opendir and readdir). Is this assumption
valid?
> > 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.
>
I could just copy the warning messages bellow each 'goto error_out'
and use then at an 'error(...)' call before the goto. But as Christian
pointed out, I think this would confuse callers wether they should
print error messages or not. On the other side, it may just be
different 'layers' of errors... I don't have any strong opinion about
this.
> > > 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`.
>
I did thought about using `unsigned flags` instead of `int pedantic`
for the same reasons Thomas pointed out, but as there would be just
one flag for now, it seemed to me that `int pedantic` would make more
sense (following the 'YAGNI' principle). But if it is already known
that more flags (or options) are coming in a very near future, I may
change this to `unsigned flags` or `struct dir_iterator_opts` in v3 if
you think it is needed. Just let me know, please.
> Thanks,
> Christian.
