Re: [pacman-dev] [PATCH] libalpm: fix documentation for alpm_pkg_mtree_next
On 06/21/19 at 03:24pm, Morgan Adamiec wrote:
> On Thu, 20 Jun 2019 at 05:46, Allan McRae 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 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, ) ==
> > 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.
Re: [pacman-dev] [PATCH] libalpm: fix documentation for alpm_pkg_mtree_next
On Thu, 20 Jun 2019 at 05:46, Allan McRae 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 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, ) ==
> ARCHIVE_OK) {
Would is then also make sense to do `typedef struct archive
alpm_mtree_t` or something similar?
Re: [pacman-dev] [PATCH] libalpm: fix documentation for alpm_pkg_mtree_next
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 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, ) ==
ARCHIVE_OK) {
Re: [pacman-dev] [PATCH] libalpm: fix documentation for alpm_pkg_mtree_next
On Fri, Jun 14, 2019 at 02:19:52PM +0100, Morgan Adamiec wrote: > On Fri, 14 Jun 2019 at 14:09, Dave Reisner 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.
Re: [pacman-dev] [PATCH] libalpm: fix documentation for alpm_pkg_mtree_next
On Fri, 14 Jun 2019 at 14:09, Dave Reisner 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.
Re: [pacman-dev] [PATCH] libalpm: fix documentation for alpm_pkg_mtree_next
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
[pacman-dev] [PATCH] libalpm: fix documentation for alpm_pkg_mtree_next
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). */ int alpm_pkg_mtree_next(const alpm_pkg_t *pkg, struct archive *archive, struct archive_entry **entry); -- 2.21.0
