search

BetterCrypto / Applied-Crypto-Hardening

Best Current Practices regarding secure online communication and configuration of services using cryptography.
https://bettercrypto.org
703 stars 99 forks source link

Optimised images #394

Closed ZephrFish closed 2 years ago

azet commented 2 years ago

See my review above this message. ^

Furthermore this Pull Request contains no information or text as to the intent of the author. Nor any comparison regarding build speed of the document or space saved on-disk (which nowadays is trivial with a few percent of compression more or less) nor the effect the post-processing has on the image quality and hence the quality of the document over all. I'm sorry, I am sure you meant to help out here, but this isn't the right way to go about it. It will just create a lot more work for two or three additional reviewers that need to go through every change you've proposed, build the document, check it against the original etc. - you may want to take a look at the Linux Kernel Documentation [0], they wrote an excellent guide on how to contribute to their project in a meaningful way. Many of the discussed topics therein are universally relevant when it comes to Free / Open Source Software Projects.

  1. Posting patches ‒ deals with the initial thoughts that go into a set of patches you propose to a project, and how your changes are presented
  2. Follow-through ‒ is about working with project maintainers on improving your submitted patches, checking them against any potential problems and their necessity as well as looking for input from other people that may be working in a similar field and often have very good suggestions. This usually goes on for a while, until everyone is happy.

Here we're quite fast compared to that. For Linux this can take 6-10 attempts until the patch set is accepted, and every single attempt had to be reworked. In the end it might need to be re-worked again due to performance issues or unforeseen bugs / opinions from Linux Developers.

I'm for halting working on this PR for the time being. Without manual review of all affected files and local builds to make sure nothing was corrupted we can't merge such a huge change on the fly. Please take care not to merge this automatically by accident.

@sebix @aaronkaplan - what do you guys think?

[0] - If that picked your interest the entire process is well documented in it's entirety over at kernel.org in the section on working with the kernel development community. Although most - but not all - F/OSS projects are similar in that regard.

aaronkaplan commented 2 years ago

I think we need to re-do the whole guide from scratch anyway, with TLS1.3+ being the focus.

sebix commented 2 years ago

What is the need for this commit? You're changing a total of 97 files. I agree that there's always better compression algorithms for images. But this was clearly autmated and unless two people check the image integrity and quality against the original for all affected files, I do not see any way to get this PR merged. I can also not think of any improvement to the PR you could do for it to be more attractive to be merged.

The size decrease is significant and there was no visible quality decrease in the samples that I had a look at. While we are not running low on disk size or bandwidth, I'm in favour of efficiency but I agree that every image needs to be compared of course.

sebix commented 2 years ago

But this was clearly autmated

Of course it was automated, who would re-compress so many images manually? And I don't think that @ZephrFish is a bot, but this PR was with created good intent and I must also say that appreciate the initiative that he has even be looking a the guide, recognized the potential for a small change and took the effort to create a PR. He could have just ignored that project because it seems outdated or alike.

azet commented 2 years ago

Look, I asked for further input because there was no PR description and gave some directions. If you other people are willing to check all changed files and see no clear negative impact I'm Ok merging it in theory. I'm general I'm more comfortable keeping picture originals, especially if they've been copied from or by third parties. We'd also need to check licenses as some licenses do not allow for modifications.

Because Pictures are one of the very few things in this project that are in encoded format it's much easier hiding data or exploits in them - I don't think this is likely the case with this PR - but there's been no reason given or description of the pull request provided for the proposed changes. Nor any in depth review been performed due to the work involved. Was metadata at least checked @sepix?

This seems like a nice gesture but I agree that there are more urgent things to work and discuss on regarding this guide. A major one being the accuracy to date and the usefulness of big portions of the document - current defaults shipped with software are usually better than our recommendations from 2013-15.

On Mon 27. Sep 2021 at 00:10, Sebastian @.***> wrote:

But this was clearly autmated

Of course it was automated, who would re-compress so many images manually? And I don't think that @ZephrFish https://github.com/ZephrFish is a bot, but this PR was with created good intent and I must also say that appreciate the initiative that he has even be looking a the guide, recognized the potential for a small change and took the effort to create a PR. He could have just ignored that project because it seems outdated or alike.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/BetterCrypto/Applied-Crypto-Hardening/pull/394#issuecomment-927380615, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGQ2SMHGFOPJ22RL3DDP7LUD6K6VANCNFSM5AMS4LNQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

sebix commented 2 years ago

Modification of licensed material is a good point, but conversions - and I consider optimisations for presentational reasons (web, print=PDF, ebook, manpage...) part of that, is covered by e.g. CC BY ND: https://creativecommons.org/faq/#when-is-my-use-considered-an-adaptation

I agree that keeping the originals in the repo and applying optimisations during build is much more transparent.

This seems like a nice gesture but I agree that there are more urgent things to work and discuss on regarding this guide. A major one being the accuracy to date and the usefulness of big portions of the document - current defaults shipped with software are usually better than our recommendations from 2013-15.

I absolutely agree but I don't want to complain but a few years ago a group invested a lot of time with the asciidoc migration to make contributions and maintenance/updates easier and later I worked on a cross-check #387 but there was literally no other help or feedback ;)