Page MenuHomePhabricator
Differential D14755

Preserve the exit code of the diff command in binary_safe_diff.sh
ClosedPublic
Actions

Authored by dim on Dec 12 2015, 6:58 PM.
Tags
None
Referenced Files
F13225280: D14755.id35690.diff
Sun, May 19, 2:22 PM
F13219782: D14755.id.diff
Sat, May 18, 8:46 PM
F13211099: D14755.diff
Fri, May 17, 5:29 AM
F13207058: D14755.diff
Wed, May 15, 8:36 PM
F13194944: D14755.diff
Sun, May 12, 10:00 PM
F13177680: D14755.diff
Wed, May 8, 7:53 PM
Unknown Object (File)
Tue, Apr 30, 5:50 PM
Unknown Object (File)
Mon, Apr 29, 4:19 PM
View All Files
Subscribers
eadler
Korvin
• stevenh

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Maniphest Tasks
T9970: new files are empty for arc diff --create and --update
Commits
rARC97056a3b85bb: Preserve the exit code of the diff command in binary_safe_diff.sh
Required Signatures
L28 Phacility Individual Contributor License Agreement
Summary

Some versions of Subversion (1.9 in any case, maybe others) will
duplicate diff headers, if the diff command run through --diff-cmd
returns 0.

This lead to T9970, where the addition of a new file with properties
only shows the properties themselves in the review, not the content of
the new file.

Test Plan

This is a trivial change, is a test needed at all?

Diff Detail

Repository
rARC Arcanist
Branch
master
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 9500
Build 11328: arc lint + arc unit

Event Timeline

dim updated this revision to Diff 35681.Dec 12 2015, 6:58 PM
dim retitled this revision from to Preserve the exit code of the diff command in binary_safe_diff.sh.
dim updated this object.
dim edited the test plan for this revision. (Show Details)
dim added a reviewer: epriestley.
dim added subscribers: eadler, • stevenh.
Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptDec 12 2015, 6:58 PM
Herald added a required legal document: L28 Phacility Individual Contributor License Agreement. · View Herald Transcript
Herald added a subscriber: Korvin. · View Herald Transcript
dim mentioned this in T9970: new files are empty for arc diff --create and --update .Dec 12 2015, 6:59 PM
• stevenh added inline comments.Dec 13 2015, 10:18 AM
scripts/repository/binary_safe_diff.sh
8

As exit takes an int I don't think which should be quoted

dim added inline comments.Dec 13 2015, 10:19 AM
scripts/repository/binary_safe_diff.sh
8

Me neither, but I just imitated the existing coding style, which appears to quote everything.

epriestley accepted this revision.Dec 13 2015, 10:21 AM
epriestley edited edge metadata.
This revision is now accepted and ready to land.Dec 13 2015, 10:21 AM
Closed by commit rARC97056a3b85bb: Preserve the exit code of the diff command in binary_safe_diff.sh (authored by dim, committed by epriestley). · Explain WhyDec 13 2015, 10:21 AM
This revision was automatically updated to reflect the committed changes.
epriestley added a task: T9970: new files are empty for arc diff --create and --update .Dec 13 2015, 10:22 AM
dim added a comment.Dec 13 2015, 10:24 AM

Now that I think of it, the simplest possible way to get the desired effect (return 0 if diff succeeds, 1 if it does not succeed), is to use:

diff "$@" || exit 1
epriestley added a comment.Dec 13 2015, 10:25 AM

Happy to accept that if you want to double-check that it works and send me a diff for it.

epriestley added a comment.Dec 13 2015, 10:26 AM

(The quoting is me just being excessively-cautious/grossly-incompetent with bash, not an explicit coding style choice.)

dim added a comment.Dec 13 2015, 10:35 AM
In D14755#167774, @epriestley wrote:

Happy to accept that if you want to double-check that it works and send me a diff for it.

Please see D14759.

Revision Contents
Changeset List

PathSize
scripts/
repository/
4 lines

Diff 35681

View Options

scripts/repository/binary_safe_diff.sh

Loading...
Phabricator · Built by Phacility