Re: [pacman-dev] [PATCH v2 1/2] makepkg: implement error codes

2017-09-17 Thread Ivy Foster
Dave Reisner  wrote:
> On Sun, Sep 17, 2017 at 05:34:15PM +1000, Allan McRae wrote:
> > On 16/09/17 06:54, Dave Reisner wrote:
> >>> +Errors
> >>> +--
> >>> +On exit, makepkg will return one of the following error codes.
> >>> +
> >>> +**E_OK**=0::
> >> I don't see the need to document internal details of how we refer to the
> >> error codes through named variables if we aren't making these public API.
> > 
> > To clarify - you are saying to remove "E_OK" etc from the documentation,
> > but keep the number and description? That seems fair enough to me.

> Yep, that's correct. Similar to how curl(1) documents its exit codes.

Ah, yes, I see what you mean.

iff


Re: [pacman-dev] [PATCH v2 1/2] makepkg: implement error codes

2017-09-17 Thread Dave Reisner
On Sun, Sep 17, 2017 at 05:34:15PM +1000, Allan McRae wrote:
> On 16/09/17 06:54, Dave Reisner wrote:
> >> +Errors
> >> +--
> >> +On exit, makepkg will return one of the following error codes.
> >> +
> >> +**E_OK**=0::
> > I don't see the need to document internal details of how we refer to the
> > error codes through named variables if we aren't making these public API.
> > 
> 
> To clarify - you are saying to remove "E_OK" etc from the documentation,
> but keep the number and description? That seems fair enough to me.
> 
> Allan

Yep, that's correct. Similar to how curl(1) documents its exit codes.


Re: [pacman-dev] [PATCH v2 2/2] makepkg: clarify error when user passes -F

2017-09-17 Thread Ivy Foster
Florian Pritz via pacman-dev  wrote:
> On 15.09.2017 20:30, ivy.fos...@gmail.com wrote:
> > -   error "$(gettext "Do not use the %s option. \
> > This option is only for use by %s.")" "'-F'" "makepkg"
> > +   error "$(gettext "Do not use the %s option. \
> > This option is only for internal use by %s.")" "'-F'" "makepkg"
> 
> This seems like a very straight forward change. It's generally better if
> you commit less invasive/more likely to be merged changes first so that
> they can be merged more easily.

That makes a lot of sense.

> In case you've never done this before: [snip]

That's very handy! Thanks, Florian.

iff


Re: [pacman-dev] [PATCH] Fix CVE-2016-5434 (DoS/loop and out of boundary read)

2017-09-17 Thread Florian Pritz via pacman-dev
On 16.09.2017 22:21, Nils Freydank wrote:
> [...]
>
> (Feedback is very appreciated / this is an updated patch.)

This line shouldn't be part of the commit. You can add it after the
"---" marker (see below) when you use `git send-email --annotate` or if
you save the patch with `git format-patch` and then edit the file before
sending with send-email.

You can also include a list of changes from the previous version there.


> 
> [1] Original patch:
> https://lists.archlinux.org/pipermail/pacman-dev/2016-June/021148.html
> CVE request (and assignment):
> http://seclists.org/oss-sec/2016/q2/526
> ---

This marker here ^

>  lib/libalpm/signing.c | 60 
> ---
>  1 file changed, 57 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/libalpm/signing.c b/lib/libalpm/signing.c
> index 95cb3280..33438140 100644
> --- a/lib/libalpm/signing.c
> +++ b/lib/libalpm/signing.c
> @@ -986,6 +986,19 @@ int SYMEXPORT alpm_siglist_cleanup(alpm_siglist_t 
> *siglist)
>   return 0;





signature.asc
Description: OpenPGP digital signature


Re: [pacman-dev] [PATCH v2 2/2] makepkg: clarify error when user passes -F

2017-09-17 Thread Florian Pritz via pacman-dev
On 15.09.2017 20:30, ivy.fos...@gmail.com wrote:
> - error "$(gettext "Do not use the %s option. This option is only 
> for use by %s.")" "'-F'" "makepkg"
> + error "$(gettext "Do not use the %s option. This option is only 
> for internal use by %s.")" "'-F'" "makepkg"

This seems like a very straight forward change. It's generally better if
you commit less invasive/more likely to be merged changes first so that
they can be merged more easily. Right now this patch should raise a
merge conflict if merged without the first one because of the changed
exit line. You also won't have to carry it along each time you make a
change to the more complex patch.

In case you've never done this before: Committing like this can be done
rather easily if you have all the changes in your working directory and
then use `git add -pi`. When it asks you to stage the hunk, you can edit
the diff and take out the exit code change and then stage only the error
message. Then commit and make a second commit for the rest.

Obviously you could also just commit it afterwards, perform an
interactive rebase to reorder the history and then fix the conflicts,
but I generally follow the above approach when I have small fixes that
come up during creation of a larger patch. It also ensures that `git
diff` is usable during development and I don't have to track the
possibility of performing that change afterwards so I can't forget it.
Also what's done is done.

Florian



signature.asc
Description: OpenPGP digital signature


Re: [pacman-dev] [PATCH v2 1/2] makepkg: implement error codes

2017-09-17 Thread Allan McRae
On 16/09/17 06:54, Dave Reisner wrote:
>> +Errors
>> +--
>> +On exit, makepkg will return one of the following error codes.
>> +
>> +**E_OK**=0::
> I don't see the need to document internal details of how we refer to the
> error codes through named variables if we aren't making these public API.
> 

To clarify - you are saying to remove "E_OK" etc from the documentation,
but keep the number and description? That seems fair enough to me.

Allan