1411004 - mach clang-format doesn't respect .clang-format-ignore

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect, P2)

Description

[Sylvestre Ledru [:Sylvestre]]

This is a recent regression, not sure what is going on.
As example:
$ ./mach clang-format -p config/
config/gcc-stl-wrapper.template.h should not be touched

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Actually, the command "./mach clang-format -p config/" is fine
but "./mach clang-format -p ./config/" is not

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1411004 - ./mach clang-format: Better handling of relative paths =mystor

For example, this is addressing the issue:
./mach clang-format -p ./mfbt/

Review commit: https://reviewboard.mozilla.org/r/194630/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194630/

Comment 3

[Sylvestre Ledru [:Sylvestre]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc33fcddaec41c72663b0e38d76a8aad1e49d53f

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8923493 [details]
Bug 1411004 - ./mach clang-format: Better handling of relative paths =mystor

https://reviewboard.mozilla.org/r/194630/#review200032

r=me, but if this doesn't work when running ./mach clang-format outside of topsrcdir I'd love a follow up to fix that :-).

::: tools/mach_commands.py:308
(Diff revision 1)
>          for line in open(pathToThirdparty):
>              # Remove comments and empty lines
>              if line.startswith('#') or len(line.strip()) == 0:
>                  continue
> -            ignored_dir.append(line.rstrip())
> +             # The regexp is to make sure we are managing relative paths
> +            ignored_dir.append("^[\./]*" + line.rstrip())

So, this doesn't perfectly manage relative paths, it only handles ./foo/bar, foo/bar. ././foo/bar, right?

I'm worried a bit about trying to run clang-format from a directory other than $topsrcdir. Do these path whitelists just stop working if we do that?

Comment 5

[Sylvestre Ledru [:Sylvestre]]

(In reply to Nika Layzell [:mystor] from comment #4)

> So, this doesn't perfectly manage relative paths, it only handles ./foo/bar,
> foo/bar. ././foo/bar, right?
It doesn't indeed. 
We discussed about that with Calixte but felt that it wasn't a comment pattern. Are we wrong?


Thanks for the review

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/31f259ee387b
/mach clang-format: Better handling of relative paths r=mystor=mystor

Comment 7

[Nika Layzell [:nika] (ni? for response)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #5)
> It doesn't indeed. 
> We discussed about that with Calixte but felt that it wasn't a comment
> pattern. Are we wrong?
> 
> 
> Thanks for the review

I don't think it's super common to run outside of topsrcdir, but it might be good to at least fail gracefully in that situation.

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing flake8 at tools/mach_commands.py:307:14 | indentation is not a multiple of four:

https://hg.mozilla.org/integration/autoland/rev/19f207add6c8ba25e25273e807a9fff22fa41b84

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=31f259ee387b61199ae3ea2f92d68fea45d88db2&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=retry&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=141073039&repo=autoland

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8923493 [details]
Bug 1411004 - ./mach clang-format: Better handling of relative paths =mystor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194630/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e1b4f786f584
/mach clang-format: Better handling of relative paths r=mystor=mystor

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e1b4f786f584