Re: DOS patch for wget 1.10-alpha1

On Sun, 10 Apr 2005, Mauro Tortonesi wrote:

> On Wednesday 06 April 2005 11:50 pm, Doug Kaufman wrote:
> >
> > Note that the wget function lookup_host causes a namespace conflict
> > with the same function in Watt-32. In the aclocal macros, "test -f"
> > will miss files with executable extensions, while "test -x" picks
> > those up. Also note that DJGPP has symlinks, but they only work for
> > executables in the same directory (at least for DJGPP 2.03). The change
> > in po/Makefile.in.in regarding "--add-comments" is to work around a bug
> > in the DOS port of xgettext, wherein it doesn't recognize the long form
> > "--add-comments" but does recognize the short form of the same option,
> > "-c".
> >
> > The DOS patches have never made it into the main source code in the
> > past, but it might be reasonable to once again consider integrating
> > them.
> 
> hi doug,
> 
> i was taking a look at your patch. there are some things i don't like or 
> understand:
> 
> first, is there any reason for which you:
> 
> - rename lookup_host to lookup_host_wget?

lookup_host is a function in the watt-32 library (which supplies the
TCP/IP stack) with which the DOS port has to be linked. Making it
lookup_host_wget clears the namespace conflict.

> - use -c instead of --add-comments when calling xgettext in 
>   po/Makefile.in.in?

The DOS port of xgettext has a bug in that it doesn't recognize
--add-comments, but does recognize "-c". According to the xgettext info
file, these options should behave identically.

> - check for HAVE_SYMLINK and S_IFLNK instead of just HAVE_SYMLINK?

DJGPP 2.03 has symlink, but it only works for executable files in
the same directory. So DJGPP has "HAVE_SYMLINK" but does not have
"S_IFLNK". This should allow symlinks to be used in any operating
system that has symlinks that really work. DJGPP 2.04 should have more
functional symlinks, whenever 2.04 gets released. Does the S_IFLNK
cause problems with any operating systems that have functioning
symlinks?

> - why do you have:
> 
> #if (defined(O_TEXT) != 0)
> setmode(fileno(stdout), O_TEXT);
> #endif /* O_TEXT */
> 
>   and then: 
> 
> #if (defined(O_BINARY) != 0)
>   if (!isatty(fileno(stdout)))
>   setmode(fileno(stdout), O_BINARY);
> #endif /* O_BINARY *
> 
>   twice in main.c?

I don't know how that duplication got there. Please ignore/delete the
second set.

> - why do you check S_IFLNK instead of HAVE_SYMLINK in remove_link?

HAVE_SYMLINK doesn't guarantee that lstat exists. DJGPP will define
"HAVE_SYMLINK" but doesn't have S_IFLNK or lstat. Perhaps it should be
"#if defined(HAVE_SYMLINK) && defined(S_IFLNK)".

> - why do you change the implementation of unique_name_1 for DJGPP?

DOS is limited to "8+3", 8 characters, a dot, then three more
characters. It handles longer names by truncation of each section of
the name. "unique_name_1" adds a suffix. This works on shorter names,
but the names "notunique" and "notunique3" are both seen by DOS as
"notuniqu". This leaves the possibility that when wget downloads two
different files with names that differ only after 8+3 characters, one
will overwrite the other. But this is a problem with all DOS programs
that try to interact with unix filenames. The change in unique_name_1
prepends the count so that it survives truncation at 8+3.
  
> also, IMVHO --wdebug is an ambiguous name for a command switch. probably 
> something like --watt32debug would be better.

The option only appears in the DJGPP port. The name "wdebug" has been
used for several years in the DJGPP port of lynx, so it should be
familiar to most of the users already. It has also been in the DJGPP
port of wget 1.82 since it started distribution in 2002, so this retains
backward compatibility. 

> last, i don't really like this code pattern you have used a couple of times:
> 
> 	char *fname;
> 	...
> if defined(__DJGPP__) || defined(__CYGWIN__)
>     fname=xmalloc(PATH_MAX);
>     strcpy(fname, msdosify(*hs->local_file));
>     strcpy(*hs->local_file, rename_if_dos_device_name(fname));
>     xfree(fname);
> #endif /* __DJGPP__ || __CYGWIN__ */
> 
> since i think using the stack instead of the heap in this case would be more 
> efficient and elegant:
> 
> if defined(__DJGPP__) || defined(__CYGWIN__)
> 	{
>       char fname[PATH_MAX];
>       strcpy(fname, msdosify(*hs->local_file));
>       strcpy(*hs->local_file, rename_if_dos_device_name(fname));
> 	}
> #endif /* __DJGPP__ || __CYGWIN__ */

I have no objections to this. I am not a programmer, just a user who
tries to get programs I like to work with operating systems that I use.
Any suggestions for code improvement are always appreciated.
                               Doug
-- 
Doug Kaufman
Internet: [email protected]