Re: fetch extension - use local filename from content-dispositionheader (new diff)

On Mon, 2006-01-09 at 11:36 -0500, Martin Cracauer wrote:
> All right, I hope we all calmed down a little.
> 
> To make people a little happier, I changed the meaning of the -O
> flag.
> 
> Previously giving this non-argument -O flag would use the
> Content-Disposition header after a basic safey check.
> 
> Now this flag takes an expected filename as an argument.  If the
> argument is given, the server-supplied name will only be used if it
> matches the expected filename.  If it doesn't the transport is aborted
> after reading the header.
> 
> This will be useful when ports distfiles are distributed from servers
> using this mechanism, which is bound to happen (if it doesn't
> already).  This way a port can say "download this URL", where the URL
> is some random php script as a filename, such as
> http://foo.com/download?fileid=3682, and download and save it only if the
> server gave the name of the expected distfile.  So we would catch it
> if we make a mistake in the URL or if the server changes the mapping.
> Note that this will abort before actually downloading the file.
> 
> For the rest of us who need this for attachments in Bugzilla and
> forums without knowing the expected filename you can give "." which
> means use the server-supplied name as is, after the previously
> mentioned safety checks.

I'm not sure I like the choice of ".", but can't think of a better
choice.

A couple of issues with the patch:

+               for (i = 0; s[i] != '"' && i < length - 1; i++) {
+                       if (s[i] < ' ') {
+                               name_altered++;
+                               target[i] = '_';
+                       } else {

Maybe better off using iscntrl(3) rather than "(s[i] < ' ')"?

+                               return NULL;

These should be "return (NULL);" per style(9)

-               if (us.size == -1)
-                       printf("Unknown\n");
-               else
-                       printf("%jd\n", (intmax_t)us.size);
+               if (O_flag) {
+                       if (us.content_disposition[0])
+                               printf("%s\n", us.content_disposition);
+                       else
+                               printf("No_filename_supplied\n");
+               } else {
+                       if (us.size == -1)
+                               printf("Unknown_size\n");
+                       else
+                               printf("%jd\n", (intmax_t)us.size);
+               }

I don't like the underscores in this output.  You're also changing the
output in the case of "-s" on it's own, which could break scripts (the
most likely user for the -s option).
There's also some overlap with the -Q option, I'm not sure it's needed
at all.  Why not, in the -O case, always print the filename unless -q is
specified?

+       /* start the transfer */

You move this block of code to before the checks for "-r" and subsequent
stat(2) of the local file - I haven't tested it, but I suspect this will
break the -r option's ability to resume a partially downloaded file.  

+                               exit(34);

Why 34?  The man page says fetch will return 1 on failure.  I wonder if
this should be replaced with a "goto failure;" instead?

Other than those points, the patch seems good.  I can see a definite
need for this functionality.

Gavin