Page MenuHomeFreeBSD

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

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

Details

Reviewers
pfg
cem
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
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 26290

Event Timeline

kevans created this revision.Thu, Sep 5, 4:23 PM
cem added a comment.Thu, Sep 5, 4:51 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...

kevans added a comment.Thu, Sep 5, 4:59 PM
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.Thu, Sep 5, 5:02 PM
cem added inline comments.
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?

This revision is now accepted and ready to land.Thu, Sep 5, 5:02 PM
cem added a comment.Thu, Sep 5, 5:03 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.