Page MenuHomePhabricator

`arc patch` removes trailing whitespace
Closed, ResolvedPublic

Description

I think that this might be an issue with my git configuration, but we should be able to detect (and maybe override) this behavior.

I created a diff on this install (see https://secure.phabricator.com/differential/diff/35819/) which contains trailing whitespace. When I patch this diff, the trailing whitespace is removed:

> arc patch --diff 35819 --nocommit  --trace
libphutil loaded from '/home/josh/workspace/github.com/phacility/libphutil/src'.
arcanist loaded from '/home/josh/workspace/github.com/phacility/arcanist/src'.
Config: Reading user configuration file "/home/josh/.arcrc"...
Config: Did not find system configuration at "/etc/arcconfig".
Working Copy: Reading .arcconfig from "/home/josh/workspace/github.com/phacility/arcanist/.arcconfig".
Working Copy: Path "/home/josh/workspace/github.com/phacility/arcanist" is part of `git` working copy "/home/josh/workspace/github.com/phacility/arcanist".
Working Copy: Project root is at "/home/josh/workspace/github.com/phacility/arcanist".
Config: Did not find local configuration at "/home/josh/workspace/github.com/phacility/arcanist/.git/arc/config".
Loading phutil library from '/home/josh/workspace/github.com/phacility/arcanist/src'...
>>> [0] <conduit> differential.querydiffs() <bytes = 68>
>>> [1] <http> https://secure.phabricator.com/api/differential.querydiffs
<<< [1] <http> 783,401 us
<<< [0] <conduit> 786,391 us
>>> [2] <conduit> user.whoami() <bytes = 117>
>>> [3] <http> https://secure.phabricator.com/api/user.whoami
<<< [3] <http> 232,025 us
<<< [2] <conduit> 232,383 us
>>> [4] <conduit> differential.querydiffs() <bytes = 149>
>>> [5] <http> https://secure.phabricator.com/api/differential.querydiffs
<<< [5] <http> 238,775 us
<<< [4] <conduit> 239,124 us
>>> [6] <exec> $ git symbolic-ref --quiet HEAD
<<< [6] <exec> 3,289 us
>>> [7] <exec> $ git rev-parse --symbolic-full-name 'arcpatch'@{upstream}
<<< [7] <exec> 11,203 us
>>> [8] <exec> $ git --version
<<< [8] <exec> 5,465 us
>>> [9] <exec> $ git ls-remote --get-url 'origin'
<<< [9] <exec> 3,147 us
>>> [10] <conduit> repository.query() <bytes = 197>
>>> [11] <http> https://secure.phabricator.com/api/repository.query
<<< [11] <http> 237,881 us
<<< [10] <conduit> 238,333 us
>>> [12] <exec> $ git diff --no-ext-diff --no-textconv --raw 'HEAD' --
>>> [13] <exec> $ git ls-files --others --exclude-standard
<<< [13] <exec> 7,126 us
<<< [12] <exec> 8,305 us
>>> [14] <exec> $ git diff-files --name-only
<<< [14] <exec> 4,429 us
>>> [15] <exec> $ git show -s --format='%H' 'ae3c2cb0e17296ed29ef9d09d02a9f5c0daa6791' --
<<< [15] <exec> 9,562 us
>>> [16] <exec> $ git symbolic-ref --quiet HEAD
<<< [16] <exec> 5,581 us
>>> [17] <exec> $ git rev-parse --verify 'arcpatch'
<<< [17] <exec> 3,322 us
Branch name arcpatch already exists; trying a new name.
>>> [18] <exec> $ git rev-parse --verify 'arcpatch_1'
<<< [18] <exec> 3,659 us
>>> [19] <exec> $ git show -s --format='%H' 'ae3c2cb0e17296ed29ef9d09d02a9f5c0daa6791' --
<<< [19] <exec> 5,795 us
>>> [20] <exec> $ git checkout -b 'arcpatch_1' 'ae3c2cb0e17296ed29ef9d09d02a9f5c0daa6791'
<<< [20] <exec> 13,053 us
Created and checked out branch arcpatch_1.
>>> [21] <exec> $ git show -s --format='%H' 'ae3c2cb0e17296ed29ef9d09d02a9f5c0daa6791' --
<<< [21] <exec> 17,559 us
>>> [22] <exec> $ git apply --index --reject -- '/tmp/etd1kzwrc1s0g84c/17786-n22g5d'
/tmp/etd1kzwrc1s0g84c/17786-n22g5d:6: trailing whitespace.
test 
Checking patch src/lint/linter/xhpast/rules/__tests__/test.lint-test...
Applied patch src/lint/linter/xhpast/rules/__tests__/test.lint-test cleanly.
warning: 1 line applied after fixing whitespace errors.
<<< [22] <exec> 4,771 us
>>> [23] <exec> $ git submodule update --init --recursive
<<< [23] <exec> 46,321 us
 OKAY  Successfully applied patch.

Event Timeline

warning: 1 line applied after fixing whitespace errors.

It looks like core.whitespace impacts this and --whitespace=nowarn might make it respect trailing whitespace.

(Confusingly, the help suggests that --whitespace=warn is the default and that warn does not fix errors, but maybe that's a version thing or maybe there's more config.)

epriestley triaged this task as Low priority.