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.

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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

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

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

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

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

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

Path

Size

scripts/

repository/

binary_safe_diff.sh

4 lines

Diff 35690

View Options

scripts/repository/binary_safe_diff.sh

Preserve the exit code of the diff command in binary_safe_diff.shClosedPublicActions