search

adrianlopezroche / fdupes

FDUPES is a program for identifying or deleting duplicate files residing within specified directories.
2.43k stars 187 forks source link

make summary compatible with other options #94

Open gritsulyak opened 6 years ago

gritsulyak commented 6 years ago

Lets consider case of call fdupes -1 -r -m -S -m (--summary) option before the proposed pull request leads to printing just summary, even in the case when such options as -1 specified. However, the documentation states, that when -1 and -S is specified, than sizes of files are shown, and files are displayed on the same line.

After the proposed fix, not only summary, but also duplicate files list is displayed, when summary flag is specified.

ferdnyc commented 6 years ago

If I'm reading the change correctly, this means that if someone were to run fdupes -m ${dir} after this patch, it would still print all of the matches, though... correct? In other words, there's no way to get the "only print a summary" behavior of current fdupes?

I personally like the idea of being able to include a summary with the output, but I suppose there might be others who desire only a summary (emailed cron jobs, perhaps?), and would still want some way to access that behavior.

gritsulyak commented 5 years ago

It is up to you. It looks like options processing can be redone, as options behaviour currently different in different contexts. And you are right at your point, that it can lead to regression of some complex case where external software relies on current behaviour.

ferdnyc commented 5 years ago

@gritsulyak

Oh, it's definitely not up to me, as I'm just another user. I suppose it's up to @adrianlopezroche . :grin:

Rereading the code, I see what you mean about the output being context-dependent. Also, I notice that currently the code restricts -m and --delete as incompatible, which after this change may not be necessary anymore. https://github.com/adrianlopezroche/fdupes/blob/5c05785dfe81a4f878206d49998e905bc2ce3f3e/fdupes.c#L1213-L1216

As far as the option thing, as I said it's not my call, but two possibilities occur to me:

  1. Add another, separate flag to preserve the only-summary functionality. Perhaps -M. (Analogous to the existing -r and -R, somewhat.) It would require that users of the current -m change their scripts to use -M instead, but it at least gives them some way to preserve the old functionality.

  2. Like I said, the summary is nice to have on output... in fact, so nice that maybe it just makes sense to always include it by default? Then -m could remain the flag for "only output the summary", like it is now. (And maybe -M could mean "suppress the summary in the default output", if that was desired.)

gritsulyak commented 5 years ago

I had no hope and intention that my PR will be merged. I created it just for my own purposes only and to demonstrate the issue with options processing. I think all current options processing in fdupes is an issue at all. And this issue can't be fixed without breaking backword compatibility. Adding additional options will make options processing even more trashy (duplicate options with a similar meaning is an additional cognitive load for the user and fdupes code reader).

rightaway commented 3 years ago

It is useful to have the summary printed with the file list, hope it can be merged.