patch(1): give /dev/null patches special treatment

[kevans]

Summary

We have a bad habit of duplicating contents of files that are sourced from /dev/null and applied more than once... take the more sane (in most ways) GNU route and complain if the file exists and offer reversal options.

This still falls short a little bit as selecting "don't reverse, apply anyway" will still give you duplicated file contents. There's probably other issues as well, but awareness is the first step to happiness.

[cem]

Meta-question: Does /dev/null need special treatment or should we reject on the basis of "zero matching lines of context" anyway? Like, if the added hunk starts at @@ 0,0 @@ or whatever and the prior contents are empty...

[kevans]

In D21535#469070, @cem wrote:

Meta-question: Does /dev/null need special treatment or should we reject on the basis of "zero matching lines of context" anyway? Like, if the added hunk starts at @@ 0,0 @@ or whatever and the prior contents are empty...

I had thought about this and I think I had a reason for not going that route. I don't recall now why, though, so I'll revisit.

[cem]

usr.bin/patch/patch.c

238

On the /dev/null name-creation basis, it seems like we could just bail on this input patch at this point rather than wait until we get to hunk processing

402

If I understand correctly, remove_file will only work if we successfully (reverse-) applied a patch to totally empty the contents first. But unlike remove_empty_files, it's specific to the loop iteration and not global. ok.

1107–1115

I don't grok our patch well enough to understand what each of these cases (throughout the subroutine) does, so it's difficult for me to verify. Comments might help but they might make it too wordy. Up to you.

usr.bin/patch/pch.c

418–420

retval != 0 indicates success? :-) Ok

usr.bin/patch/tests/unified_patch_test.sh

114–115

Do we not also want to check on the size of foo?

[cem]

In D21535#469071, @kevans wrote:

I had thought about this and I think I had a reason for not going that route. I don't recall now why, though, so I'll revisit.

I'm really happy just special casing devnull now since it seems straightforward. Don't waste too much time thinking about a context solution if it isn't any better in practice.

I would like to lower our default max-fuzz, if that's possible. Totally unrelated to this patch.

[kevans]

Drop in a lot more comments that should help explain the funky flow and make the decisions made (incorrect or not =-)) a little more clear. Some of the weird structure is so we can still be verbose about what we're doing without rewriting more than we need to (though a good chunk of it needs a rewrite anyways, because it feels a little unwieldy in spots).

[kevans]

• Do some sanity checks to make sure we didn't do something funky like apply the patch but throw an error exit status
• Amend the manpage to mention that file existence isn't a crisis.