Hi, Thomas
Sorry for the late reply, but now that I submitted my GSoC proposal I
can finally come back to this series.
On Sun, Mar 31, 2019 at 3:12 PM Thomas Gummerer <t.gummerer@xxxxxxxxx> wrote:
>
> On 03/30, Matheus Tavares wrote:
> > Add the possibility of giving flags to dir_iterator_begin to initialize
> > a dir-iterator with special options.
> >
> > Currently possible flags are DIR_ITERATOR_PEDANTIC, which makes
> > dir_iterator_advance abort immediately in the case of an error while
> > trying to fetch next entry; and DIR_ITERATOR_FOLLOW_SYMLINKS, which
> > makes the iteration follow symlinks to directories and include its
> > contents in the iteration. These new flags will be used in a subsequent
> > patch.
> >
> > Also adjust refs/files-backend.c to the new dir_iterator_begin
> > signature.
> >
> > Signed-off-by: Matheus Tavares <matheus.bernardino@xxxxxx>
> > ---
> > dir-iterator.c | 28 +++++++++++++++++++++++++---
> > dir-iterator.h | 39 +++++++++++++++++++++++++++++++++------
> > refs/files-backend.c | 2 +-
> > 3 files changed, 59 insertions(+), 10 deletions(-)
> >
> > diff --git a/dir-iterator.c b/dir-iterator.c
> > index f2dcd82fde..17aca8ea41 100644
> > --- a/dir-iterator.c
> > +++ b/dir-iterator.c
> > @@ -48,12 +48,16 @@ struct dir_iterator_int {
> > * that will be included in this iteration.
> > */
> > struct dir_iterator_level *levels;
> > +
> > + /* Combination of flags for this dir-iterator */
> > + unsigned flags;
> > };
> >
> > int dir_iterator_advance(struct dir_iterator *dir_iterator)
> > {
> > struct dir_iterator_int *iter =
> > (struct dir_iterator_int *)dir_iterator;
> > + int ret;
>
> Minor nit: I'd define this variable closer to where it is actually
> used, inside the second 'while(1)' loop in this function. That would
> make it clearer that it's only used there and not in other places in
> the function as well, which I had first expected when I read this.
Right, thanks.
> > diff --git a/dir-iterator.h b/dir-iterator.h
> > index 970793d07a..93646c3bea 100644
> > --- a/dir-iterator.h
> > +++ b/dir-iterator.h
> > @@ -19,7 +19,7 @@
> > * A typical iteration looks like this:
> > *
> > * int ok;
> > - * struct iterator *iter = dir_iterator_begin(path);
> > + * struct iterator *iter = dir_iterator_begin(path, 0);
>
> Outside of this context, we already mentione errorhandling when
> 'ok != ITER_DONE' in his example. This still can't happen with the
> way the dir iterator is used here, but it serves as a reminder if
> people are using the DIR_ITERATOR_PEDANTIC flag. Good.
This made me think again about the documentation saying that
dir_iterator_abort() and dir_iterator_advance() may return ITER_ERROR,
but the implementation does not containing these possibilities.
(Besides when the pedantic flag is used). Maybe the idea was to make
API-users implement the check for an ITER_ERROR in case dir-iterator
needs to start returning it in the future.
But do you think such a change in dir-iterator is likely to happen?
Maybe we could just make dir_iterator_abort() be void and remove this
section from documentation. Then, for dir_iterator_advance() users
would only need to check for ITER_ERROR if the pedantic flag was given
at dir-iterator creation...
Also CC-ed Michael in case he has some input
