Re: [Bug-wget] [PATCH] Implement --keep-badhash to keep Metalink's files with a bad hash

[Tim Ruehsen]

On Saturday, July 30, 2016 9:41:48 PM CEST Matthew White wrote:
> Hello!
> 
> I think that sometimes it could help to keep downloaded Metalink's files
> which have a bad hash.
> 
> The default wget behaviour is to delete such files.
> 
> This patch provides a way to keep files which have a bad hash through the
> option --keep-badhash. It appends the suffix .badhash to the file name,
> except without overwriting existing files. In the latter case, an unique
> suffix is appended after .badhash.
> 
> I made this patch working on the following branch:
> master (latest 20cac2c5ab3d63aacfba35fb10878a2d490e2377)
> git://git.savannah.gnu.org/wget.git
> 
> What do you think?

Hi Matthew,

good work !

While your FSF assignment is underway (PM), we can continue polishing.

Didn't test your code yet,  but from what I see, there are just these peanuts:

1.
+  bhash = malloc (strlen (name) + strlen (".badhash") + 1);
+  strcat (strcpy (bhash, name), ".badhash");

Please consider using concat_strings() from util.h.
And please leave an empty line between var declaration and code - just for 
readability.

2. 
Please add 'link' and 'unlink' to bootstrap.conf. Gnulib will emulate these on 
platforms where they are not available (or have a slightly different 
behavior). I guess we simply forgot 'unlink' when we switched to gnulib.

3.
The (GNU) commit messages should ideally look like:

one line of short description
<empty line>
file changes
<empty line>
long description

For example:
Add new option --keep-badhash

* src/init.c: Add keepbadhash
* src/main.c: Add keep-badhash
* src/options.h: Add keep_badhash
* doc/wget.texi: Add docs for --keep-badhash
* src/metalink.h: Add prototypes badhash_suffix(), badhash_or_remove()
* src/metalink.c: New functions badhash_suffix(), badhash_or_remove().
  (retrieve_from_metalink): Call append .badhash()

With --keep-badhash, append .badhash to Metalink's files with checksum
mismatch, except without overwriting existing files.

Without --keep-badhash, remove downloaded files with checksum mismatch
(this conforms to the old behaviour).

[This also applies to to your other patches]

4. 
Please not only write 'keepbadhash', but also what you did (e.g. remove, 
rename, add, ...), see my example above.

Those ChangeLog entries should allow finding changes / faults / regressions 
etc. even when a versioning system is not at hand, e.g. when unpacking the 
sources from a tarball. (Not debatable that consulting commit messages 
directly is much more powerful.)

5.
You write "except without overwriting existing files", maybe you should mention 
appending a counter instead of overwriting existent files !?


Regards, Tim

signature.asc
Description: This is a digitally signed message part.