[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#851937: RFS: farbfeld/2.20170109-1 ITP



control: tag -1 -moreinfo

On 2017-07-15 02:28, Sean Whitton wrote:
> 
> Here is a review of 534d41f:

Thank you for reviewing it.

> - I think we should list Dmitry in the Uploaders: field

Done

> - your git history does not really give credit to Dmitry for his work.
>   I'd like to suggest starting again, and doing it like this:

This is because I didn't start using dgit from the beginning, and I
rebuilt debian/ in a separate working tree. Now I better understand
dgit's philosophy and I started over. The repository is still at

https://anonscm.debian.org/cgit/users/paride-guest/farbfeld.git/

but it's a new one, which retains the full history.

> - I also think it would be good to state in debian/changelog that most
>   of the Debianisation is due to Dmitry

Done

> - your changes to the patch header do not make sense: the '3..' will not
>   yield "the changes made by the Debian maintainer in the first upload
>   of upstream version 3".

Should be fixed now. The 'debian/3-1' tag is still missing, I guess the
right time to tag it is after the package is accepted.

> - I disagree with you about Dmitry's `convert` patch.  It just doesn't
>   seem likely to me that there would be difficult merge conflicts with
>   new upstream versions, and it is indeed useful to inform the user that
>   convert is not available.  But I will defer to your judgement -- if
>   you're sure about dropping the patch, maybe imagemagick should be
>   moved to a hard dependency?

I still believe this patch belongs to upstream, and even if it's trivial
to maintain it already prevented a clean 'git merge' of upstream version
3. The package works fine even without imagemagick, it just can't handle
all those image formats. Imagemagick itself is not very kind when a
helper binary is missing:

$ convert test.jpg test.webp
convert-im6.q16: delegate failed `'cwebp' -quiet %Q '%i' -o '%o'' @
error/delegate.c/InvokeDelegate/1919.

(cwebp is provided by the webp package, which is not even suggested by
imagemagick).

In general I believe it's better not to apply patches whenever possible,
for several reasons. I understand this one is trivial, but the issue it
addresses is, in my opinion, even more trivial.

This said, I left it in. I explained my general thought on the topic,
but I value your feedback and while I think there is no strong reason to
include this patch, I see there is no strong reason to oppose it.

I would be against a hard dependency on imagemagick.


Thanks again,

Paride


Reply to: