[pacman-dev] [PATCH] libalpm: fix documentation for alpm_pkg_mtree_next

[Andrew Gregory]

On 06/21/19 at 03:24pm, Morgan Adamiec wrote:
> On Thu, 20 Jun 2019 at 05:46, Allan McRae <allan at archlinux.org> wrote:
> >
> > On 14/6/19 11:26 pm, Dave Reisner wrote:
> > > On Fri, Jun 14, 2019 at 02:19:52PM +0100, Morgan Adamiec wrote:
> > >> On Fri, 14 Jun 2019 at 14:09, Dave Reisner <d at falconindy.com> wrote:
> > >>>
> > >>> On Fri, Jun 14, 2019 at 02:51:14AM +0100, morganamilo wrote:
> > >>>> libarchive uses 1 for EOF, not 0. Instead of using the actual ints, use
> > >>>> libarchive's error codes.
> > >>>> ---
> > >>>>
> > >>>> By the way, not familiar with doxygen. Is my wording fine or is there
> > >>>> some built in "see also" functionality?
> > >>>>
> > >>>> diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
> > >>>> index ffb2ad96..ece894cf 100644
> > >>>> --- a/lib/libalpm/alpm.h
> > >>>> +++ b/lib/libalpm/alpm.h
> > >>>> @@ -1326,7 +1326,8 @@ struct archive *alpm_pkg_mtree_open(alpm_pkg_t *pkg);
> > >>>>   * @param pkg the package that the mtree file is being read from
> > >>>>   * @param archive the archive structure reading from the mtree file
> > >>>>   * @param entry an archive_entry to store the entry header information
> > >>>> - * @return 0 if end of archive is reached, non-zero otherwise.
> > >>>> + * @return ARCHIVE_OK on success, ARCHIVE_EOF if end of archive is reached,
> > >>>> + * otherwise an error occured (see archive.h).
> > >>>
> > >>> Please, no. Let's not leak details from libarchive in our own API.
> > >>>
> > >>>>   */
> > >>>>  int alpm_pkg_mtree_next(const alpm_pkg_t *pkg, struct archive *archive,
> > >>>>               struct archive_entry **entry);
> > >>>> --
> > >>>> 2.21.0
> > >>
> > >> Why not? The return value is exactly that. If libarchive's return
> > >> codes suddenly changed then so would libalpms's. Plus pacman itself
> > >> already uses ARCHIVE_OK to check the return code. And finally if we
> > >> did not depend on magic numbers then the doc wouldn't be wrong in the
> > >> first place.
> > >
> > > Because users of libalpm should only need to understand libalpm and not
> > > concern themselves with details of libarchive. Exposing ARCHIVE_* in
> > > libalpm is a leaky abstraction.
> > >
> > > If the code is broken (and it sounds like it is), then it should be
> > > fixed along with the documentation.
> > >
> >
> > Agreed.   Not this is the only place in pacman we use an ARCHIVE_*
> > value, so this is broken.
> >
> > src/pacman/check.c:     while(alpm_pkg_mtree_next(pkg, mtree, &entry) ==
> > ARCHIVE_OK) {
> 
> Would is then also make sense to do `typedef struct archive
> alpm_mtree_t` or something similar?

I already said this on IRC, but, for the record, I don't see a need for a full
alpm mtree implementation.  I wouldn't reject it if somebody really wanted to
put in that effort, but it seems like a lot of work to reimplement libarchive's
entire archive_entry_* API for little gain.  Personally, I'd just delete this
function and alpm_pkg_mtree_close.