null patches special treatment
ClosedPublic
Actions

Authored by kevans on Sep 5 2019, 4:23 PM.

Details

Reviewers

pfg
cem

Group Reviewers

manpages

Commits

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

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 Skipped

Unit

Tests Skipped

Build Status

Buildable 27093

Event Timeline

kevans created this revision.Sep 5 2019, 4:23 PM

Herald added a subscriber: imp. · View Herald TranscriptSep 5 2019, 4:23 PM

Harbormaster completed remote builds in B26290: Diff 61697.Sep 5 2019, 4:23 PM

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 accepted this revision.Sep 5 2019, 5:02 PM

cem added inline comments.

usr.bin/patch/patch.c
239	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
427–438	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.
1143–1151	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–116	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

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.

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

Harbormaster completed remote builds in B27093: Diff 63423.Oct 17 2019, 11:27 PM

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.

Herald added a reviewer: manpages. · View Herald TranscriptNov 1 2019, 3:43 AM

Harbormaster completed remote builds in B27305: Diff 63856.Nov 1 2019, 3:43 AM

This revision was not accepted when it landed; it landed in state Needs Review.Nov 4 2019, 3:07 AM

Closed by commit rS354328: patch(1): give /dev/null patches special treatment (authored by kevans). · Explain Why

This revision was automatically updated to reflect the committed changes.

kevans added a commit: rS354328: patch(1): give /dev/null patches special treatment.