Re: being nice to patch(1)

[ Paul Eggert added to Cc: I'm not sure he actually maintains "patch" 
  or cares any more, but hopefully he at least knows who does ]

On Tue, 3 Jul 2007, Theodore Tso wrote:

Is there such a thing?

The latest official version of patch from GNU is 2.5.4 from 1999, I think.

I'm finding references to 2.5.9 in distributions (from 2003), but 2.5.4 is the latest I see on the GNU mirror at kernel.org, and that's also what Fedora 7 has too, it seems, so the 2.5.9 thing seems to be something unofficial or at least not widely known about..

Anyway, I tried to look at the patch sources, but I had to stop. That whole "intuit_diff_type()" function is probably designed as an initiation rite for any patch programmers, and to make sure that you have to be really serious about wanting to send patches before you can become part of the "in crowd". It's "mental hazing".

Yeah, git-apply sources aren't necessarily a thing of great beauty either, but in comparison to patch, I think it's a work of art. Of course, part of it is that it doesn't try to parse 'ed' scripts etc, but a large part of it really is that "patch" is an old program that has grown over time, and not seen a lot of cleanups, I suspect.

IOW, I tried to see how easy it would be to dismiss the code that takes care of "indent", but it wasn't totally obvious. It's set in many different places, and the logic for "skip_this_patch" is a bit confusing.

Anyway, with Paul Eggert Cc'd, maybe he can help us sort it out.

Paul - the issue here isn't actually with git at all, but the fact that Andrew Morton noticed that he cannot apply one of the series of patches he has with "patch" (well, with his scripts that are designed _around_ patch, to be exact).

The reason? Part of the patch *description* looked like this:

    [SCSI] 53c700: Amiga 4000T NCR53c710 SCSI

    New driver for the Amiga 4000T built-in NCR53c710 SCSI controller, using the
    53c700 SCSI core.

where it really *was* indented by four characters (and that's where git comes in: git indents the patch descriptions exactly so that you cannot *possibly* confuse the patch itself with the description).

It turns out that "patch" would actually think there is a patch there: the line

    53c700 SCSI core.

was determined to be an ed-script ("53c700") _despite_ the fact that it's indented.

Andrew was able to fix that particular damage by using "-u" and forcing anything but unified diffs to be ignored, but that isn't an option for all quilt users, since some projects use old-fashioned context diffs or a mixture.

Besides, the explanations can certainly contain patch fragments anyway (in the kernel, we put things like example code in them).

And it really boils down to a really simple thing: when scripting, you DO NOT WANT "patch" to make random guesses. And that whole "indentation" thing by patch is a pure guess, and should simply NOT BE DONE. And there's no way to tell patch to not do it.

So Paul, you're our only hope.

I'm personally trying to tell people not to use "patch" at all (this isn't the first time patch has done insane things by default, but it's the first time you cannot even _disable_ the insane behaviour), but Ted has a point: regardless of whether people learn to use "git-apply" to apply patches, the old "patch" binary would be better off just improved.

In this case, the improvement would be to simply ignore indented patches (preferably by default, but at least have the option to do so).

		Linus