Re: [GSoC][PATCH v5 3/7] dir-iterator: add flags parameter to dir_iterator_begin

[Matheus Tavares Bernardino <matheus.bernardino@usp.br>]

On Wed, Apr 24, 2019 at 3:36 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
>
> On 04/23, Matheus Tavares Bernardino wrote:
> > On Thu, Apr 11, 2019 at 6:09 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > >
> > > On 04/10, Matheus Tavares Bernardino wrote:
> > > > > > 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.
> > >
> > > Yeah, I think that was the intention.
> > >
> > > > 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...
> > >
> > > Dunno.  In a world where we have the pedantic flag, I think only
> > > returning ITER_ERROR if that flag is given might be what we want to
> > > do.  I can't think of a reason why we would want to return ITER_ERROR
> > > without the pedantic flag in that case.
> >
> > Ok. I began doing the change, but got stuck in a specific decision.
> > What I was trying to do is:
> >
> > 1) Make dir_iterator_advance() return ITER_ERROR only when the
> > pedantic flag is given;
> > 2) Make dir_iterator_abort() be void.
> >
> > The first change is trivial. But the second is not so easy: Since the
> > [only] current API user defines other iterators on top of
> > dir-iterator, it would require a somehow big surgery on refs/* to make
> > this change. Should I proceed and make the changes at refs/* or should
> > I keep dir_iterator_abort() returning int, although it can never fail?
>
> Maybe I'm missing something, but wouldn't this change in refs.c be
> enough? (Other than actually making dir_iterator_abort not return
> anything)
>
>         diff --git a/refs/files-backend.c b/refs/files-backend.c
>         index 5848f32ef8..81863c3ee0 100644
>         --- a/refs/files-backend.c
>         +++ b/refs/files-backend.c
>         @@ -2125,13 +2125,12 @@ static int files_reflog_iterator_abort(struct ref_iterator *ref_iterator)
>          {
>                 struct files_reflog_iterator *iter =
>                         (struct files_reflog_iterator *)ref_iterator;
>         -       int ok = ITER_DONE;
>
>                 if (iter->dir_iterator)
>         -               ok = dir_iterator_abort(iter->dir_iterator);
>         +               dir_iterator_abort(iter->dir_iterator);
>
>                 base_ref_iterator_free(ref_iterator);
>         -       return ok;
>         +       return ITER_DONE;
>          }
>
>          static struct ref_iterator_vtable files_reflog_iterator_vtable = {

Yes, indeed. But I thought that since the reason for making
dir_iterator_abort() be void is that it always returns ITER_DONE, the
same change should be applied to files_reflog_iterator_abort() as it
would fall into the same case. And this, in turn, would require
changes to ref_iterator_abort() and many other functions at
refs/iterator.c and refs/files-backend.c

> Currently the only thing calling dir_iterator_abort() is
> files_reflog_iterator_abort() from what I can see, and
> dir_iterator_abort() always returns ITER_DONE.
>
> That said, I don't know if this is actually worth pursuing.  Having it
> return some value and having the caller check that makes it more
> future proof, as we won't have to change all the callers in the future
> if we want to start returning anything other than ITER_DONE.   Just
> leaving it as it is now doesn't actually hurt anybody I think, but may
> help in the future.

Ok, I understand.

> > There's also a third option: The only operation that may fail during
> > dir_iterator_abort() is closedir(). But even on
> > dir_iterator_advance(), I'm treating this error as "non-fatal" in the
> > sense that it's not caught by the pedantic flag (although a warning is
> > emitted). I did it like this because it doesn't seem like a major
> > error during dir iteration... But I could change this and make
> > DIR_ITERATOR_PEDANTIC return ITER_ERROR upon closedir() errors for
> > both dir-iterator advance() and abort() functions. What do you think?
>
> I think this might be the right way to go.  We don't really need an
> error from closedir, but at the same time if we are being pedantic,
> maybe it should be an error.  I don't have a strong opinion here
> either way, other than I think it should probably keep returning an
> int.

I know I suggested this option, but searching the code base I saw no
other place that checks closedir()'s return besides dir-iterator. So
maybe the best option would be to keep dir_iterator_abort() always
returning ITER_DONE, even upon closedir() errors. Them, I can document
that the pedantic flag only affects dir_iterator_advance() behavior
(but closedir() errors wouldn't be considered here as well).

I got stuck in this for a while, but finally this option seems good to me now...

> > > Though I think I would change the example the other way in that case,
> > > and pass DIR_ITERATOR_PEDANTIC to 'dir_iterator_begin()', as it would
> > > be easy to forget error handling otherwise, even when it is
> > > necessary.  I'd rather err on the side of showing too much error
> > > handling, than having people forget it and having users run into some
> > > odd edge cases in the wild that the tests don't cover.
> >
> > Yes, I agree.
> >
> > > > Also CC-ed Michael in case he has some input