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

[Matthew White]

On Mon, 01 Aug 2016 16:32:30 +0200
Tim Ruehsen <address@hidden> wrote:

> 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

Thanks Tim!

I really needed your guidance. I sent the modified patches to address@hidden

I believe there are more things to fix.

Consider the following after applying the attached patch:
* src/metalink.c (retrieve_from_metalink): line 124: If the download is 
interrupted (output_stream isn't NULL), and there are more urls for the same 
file (we are still in the download loop), switching to the next url should 
resume the download (instead than start it over).

In this patch I added a fix to rename/remove the fully downloaded file 
(output_stream is NULL), but there was an error (probably checksum) and we are 
still in the download loop (more urls for the same file). But as said before, 
switching to the next url should continue the download:
* src/metalink.c (retrieve_from_metalink): line 131: Rename/remove fully 
downloaded file on error

I still have to investigate the problem...

Regards,
Matthew

-- 
Matthew White <address@hidden>

0001-Add-new-option-keep-badhash-to-keep-Metalink-s-files.patch
Description: Text Data

0002-Add-link-and-unlink-to-bootstrap.conf.patch
Description: Text Data