1556088 - If you end up with a commit whose only changes to a file are from "clang-format", then it's really hard to get those changes out of your patch

Status: NEW

(Developer Infrastructure :: Lint and Formatting, task, P3)

Description

[Daniel Holbert [:dholbert] (PTO until 2/24)]

cmarlow just ran into some trouble with clang-format causing him trouble over in https://phabricator.services.mozilla.com/D33233

Here's roughly what happened:

• nsTextFrame had a latent formatting issue.
• cmarlow had a patch which initially touched nsTextFrame.cpp, so our clang-format commit hook added the formatting changes to his patch.
• He later removed his own changes to nsTextFrame.cpp, so his commit was left with just the formatting changes as the only adjustment to nsTextFrame.cpp, which is a bit messy (since he's not even touching that file meaningfully anymore)
• He tried to revert the changes by doing e.g.

 hg revert -r [parent] layout/generic/nsTextFrame.cpp
 hg commit --amend

...but the clang-format commit hook ran and treated nsTextFrame.cpp as a modified file and auto-reformatted it again as part of the commit.

So, there was no easy way to get this unrelated change out of his patch. Is there a recommended way to address this?

(He ended up working around the issue by creating a dedicated dummy commit to do the nsTextFrame.cpp reformatting and rebasing his "real" patch on top of it. This successfully removed the formatting changes from his main commit, as part of the rebase operation. This felt kinda broken, though.)

Comment 1

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(Note: I fixed the nsTextFrame formatting issue in https://hg.mozilla.org/integration/mozilla-inbound/rev/55447a3ae13a73424b8abd301e28291fcedf5680 , so that particular file/indentation issue won't cause trouble again, but other latent formatting issues easily could cause trouble, and it'd be nice to have some Best Practices and a known-good workaround available, and for it to be hopefully less of a hassle than what we ended up doing in this case.

In a perfect world, it'd be great if the clang-format "is this file a candidate for automatic reformatting" selection step would compare whether the file in question differs between the current working copy & the parent-revision copy. If that was the logic that we used here, then cmarlow's initial attempt to address this (reverting + recommitting) would've worked.