search

andygrundman / libmediascan

C library for scanning audio/video/image file metadata
GNU Lesser General Public License v3.0
8 stars 5 forks source link

Update to enable use of modern FFmpeg and GIFLIB #5

Closed fsbruva closed 5 years ago

fsbruva commented 5 years ago

This PR successfully builds against FFmpeg 4.1.1 and GIFLIB 5.1.4.

It accomplishes the following:

Additionally, several precompiler directives were implemented by @chincheta0815 to improve build support for Solaris and FreeBSD. Lastly, it removes trailing whitespace throughout the project.

andygrundman commented 5 years ago

Wow! I definitely don't have time to review this all, so I invited you as a collaborator. I think this should let you commit directly to the repo. It's great to see someone interested in working on this project. Thanks!

mherger commented 5 years ago

@fsbruva - you should avoid whitespace changes. There are dozens of files with whitespace changes only. Not only does it increase the number of changed files unnecessarily, but it can also render following changes harder. Not all diff tools/editors are handling whitespace changes in a smart way.

fsbruva commented 5 years ago

@mherger So you are saying it should be a separate commit just for whitespace corrections? Or avoid making the changes altogether?

I was planning on merging this into a new 'devel' branch, and then squash merge into public. I can try to isolate the whitespace changes that way by doing a patch commit.

mherger commented 5 years ago

Don't bother doing them. If some whitespace changes during an edit, so be it. But changing dozens of files just for the sake of getting rid of whitespace seriously harms the git history of those files. IMHO.

When I saw there were 392 changed files it was like "no way this is going to be reviewed by anyone". But get rid of whitespace changes, and that change might look much more manageable.

It's just my opinion. I bet this has been the subject of many heated discussions already :-).

fsbruva commented 5 years ago

Don't bother doing them.

To quote Legion, "You ever make soup? Cut up the meat, vegetables, add broth, cook it for a couple hours. You ever try to unmake soup?"

If some whitespace changes during an edit, so be it. But changing dozens of files just for the sake of getting rid of whitespace seriously harms the git history of those files.

Why? Is this an object lesson you've learned the hard way? Or is it fair warning so I don't try to do a whitespace cleanup on any logitech repo? :-)

When I saw there were 392 changed files it was like "no way this is going to be reviewed by anyone". But get rid of whitespace changes, and that change might look much more manageable.

Don't worry - you won't have to review it!

It's just my opinion. I bet this has been the subject of many heated discussions already :-).

Actually not really. I did some work with git macros to automatically enforce what I thought were fairly standard and commonplace coding conventions. I did it because I could, and that's usually good enough for me.

As I think about your comments a little more, I think I'll separate the whitespace fixes as a separate commit. My reasoning is that waiting to do whitespace fixes during another edit:

  1. Obfuscates the functional changes made to that file to fix a bug. (Was the whitespace the cause of the bug listed in the commit?)
  2. Will never achieve a baseline of code standards (unless every file with whitespace also has bugs, in which case we might have other issues)
mherger commented 5 years ago

I'll be happy to continue the discussion offline. It doesn't belong here. I'd just recommend not to overwhelm a reviewer with unnecessary changes.

fsbruva commented 5 years ago

I will close this PR and perform a rebase to skip the commit that contains all the trailing whitespace "fixes."

andygrundman commented 5 years ago

Thanks. I agree with Michael that too many whitespace changes make it very hard to see the real content of a patch. I apologize for leaving so much trash in these files, I hadn't yet been enlightened about the evils of trailing whitespace when we wrote this stuff. :)

mherger commented 5 years ago

@andygrundman you taught me about this! Once I committed a bunch of files in which I had accidentally changed all the line endings... 😄