Re: More patch 2.5.9 hacking

[Eli Zaretskii]

[I'm not the maintainer of GNU Patch, but I did work on its port to
DJGPP, which is a free environment for developing programs for MS-DOS
and MS-Windows.  That is why I allow myself a few comments on your
suggested changes.  Please wait for Paul to give you the definitive
response.]

> From: "Stephan T. Lavavej" <address@hidden>
> Date: Wed, 22 Oct 2003 00:14:01 -0700
> 
> I have fixed the "assertion failed" problem which occurs when patch 2.5.9 is
> compiled for MinGW and used on certain files and patches without --binary.
> My fix is simple: when HAVE_SETMODE_DOS is enabled, binary mode is *forced*.
> The --binary switch is silently ignored.

Can you tell why does the assertion fail with the original code?  The
DJGPP port doesn't have this problem, although the last version I
worked with was Patch 2.5.4.  However, I don't think there were any
significant changes in this area since that version.

> I would in fact suggest that patch should work with everything in binary at
> all times on all systems.  Text mode is an abomination from the depths of
> 1970s evil and we shouldn't have to put up with it anymore.

I disagree.  To be a good Windows citizen, the ported Patch must be
able to work on files with both Unix- and DOS-style end-of-line (EOL)
format alike.  Imagine, for example, that you have a source file that
came from a Unix .tar.gz archive, and which therefore has the Unix
single-newline EOL format, and a patch file that was created by a
Windows mail client from a mail message, and thus will most probably
have a DOS-style CR-LF EOL format.  If you use --binary, the patch
will fail to apply (because there's an extra CR character at the end
of each line of context in the hunks).  Moreover, it will fail in a
mysterious way, since the user cannot easily understand what is wrong
here.

Dealing with this problem is a nuisance, and a gratuitous nuisance at
that.  If Patch works in text mode, this kind of problem goes away.

There are many situations like that, that's why (IMHO) --binary cannot
be turned on by default on DOS/Windows platforms.  Programmers who
work on those platforms expect their development tool to work well
with both styles of EOL format.  For example, the compiler is expected
to compile both Unix- and DOS-style source files, the editor is
expected to be able to edit both types of files (a test that the
infamous Notepad fails miserably), Make is expected to process
Makefiles written either on Unix or on Windows, Diff is expected to be
able to compare a Unix-style file with a DOS-style file, etc.  Why
should Patch be any different?

> C:\Temp\test\patch-2.5.9>patchOLD -p1 < patch-2.5.9-mingw.patch
> patching file dirname.h
> Assertion failed: hunk, file patch.c, line 340
> 
> This application has requested the Runtime to terminate it in an unusual
> way.

What exactly does this last sentence mean?  Presumably, Windows is
trying to tell us something important about the way Patch terminated,
but what is it?  Is it just the fact that Patch exits with a non-zero
exit code, or is there something else?

> I do look forward to a patch 2.5.10 or 2.6 that fixes some or all of these
> problems in a better way

I cannot speak for Paul, who maintains Patch, but I doubt if this will
happen unless you tell the details about why the assertion fails.  How
can we expect Paul, or anyone else, to fix the code, if the problem is
not understood?

>  # ifndef ISSLASH
> -#  define ISSLASH(C) ((C) == DIRECTORY_SEPARATOR)
> +#  ifdef __MINGW32__
> +#   define ISSLASH(C) ((C) == '/' || (C) == '\\')
> +#  else
> +#   define ISSLASH(C) ((C) == DIRECTORY_SEPARATOR)
> +#  endif

This should IMHO be handled differently: some MinGW-specific header
should define ISSLASH as suitable for Windows.  The whole point of
"#ifndef ISSLASH" in this snippet is to move ugly system-dependent
stuff out of the mainline code, so that the mainline code remains
clean and yet portable.

> -#if HAVE_SETMODE_DOS
> -             binary_transput = O_BINARY;
> -#endif

Note, btw, that this change affects _all_ platforms that have
HAVE_SETMODE_DOS defined, not only MinGW.  I, for one, would object to
this change to affect the DJGPP port, which also defines
HAVE_SETMODE_DOS.

> +#ifdef __MINGW32__
> +      if (errno == EXDEV || errno == EEXIST)
> +#else
> +      if (errno == EXDEV)
> +#endif

I think Paul already responded to this change: it sounds like the
library function `rename' fails when the target file already exists.
The proper solution should be to write a Posix-compatible version of
`rename', which doesn't fail in that case, and use it instead of the
one provided by the Windows/MinGW runtime.  (A simple strategy to
implement such a version of `rename' is to call the original `rename',
and if it fails with EEXIST or EACCES, remove the target file and try
again.)

> +#ifdef __MINGW32__
> +        mkdir (filename);
> +#else
> +        mkdir (filename,
> +           S_IRUSR|S_IWUSR|S_IXUSR
> +           |S_IRGRP|S_IWGRP|S_IXGRP
> +           |S_IROTH|S_IWOTH|S_IXOTH);
> +#endif

Same here: I suggest to write a Posix-comlpiant version of `mkdir',
which accepts (and largely ignores) the additional argument, and use
that instead of the Windows version.

HTH