Page MenuHomeFreeBSD

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

Authored by kevans on Sep 5 2019, 4:23 PM.
Tags
None
Referenced Files
Unknown Object (File)
Dec 22 2023, 10:21 PM
Unknown Object (File)
Nov 22 2023, 10:29 PM
Unknown Object (File)
Nov 5 2023, 11:29 PM
Unknown Object (File)
Nov 5 2023, 1:13 PM
Unknown Object (File)
Oct 19 2023, 8:19 PM
Unknown Object (File)
Oct 19 2023, 12:25 PM
Unknown Object (File)
Oct 13 2023, 8:28 AM
Unknown Object (File)
Oct 4 2023, 10:33 PM
Subscribers

Details

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.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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...

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 added inline comments.
usr.bin/patch/patch.c
238 ↗(On Diff #61697)

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 ↗(On Diff #61697)

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 ↗(On Diff #61697)

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 ↗(On Diff #61697)

retval != 0 indicates success? :-) Ok

usr.bin/patch/tests/unified_patch_test.sh
114–115 ↗(On Diff #61697)

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

This revision is now accepted and ready to land.Sep 5 2019, 5:02 PM

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.

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).

This revision now requires review to proceed.Oct 17 2019, 11:27 PM
kevans marked 2 inline comments as done.
  • 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.
This revision was not accepted when it landed; it landed in state Needs Review.Nov 4 2019, 3:07 AM
This revision was automatically updated to reflect the committed changes.