Closed Bug 1411004 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox Build System :: Source Code Analysis, defect, P2)

defect

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: Sylvestre, Assigned: Sylvestre)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
Blocks: clang-format
Actually, the command "./mach clang-format -p config/" is fine
but "./mach clang-format -p ./config/" is not
Priority: -- → P2
Assignee: nobody → sledru
Attachment #8923493 - Flags: review?(nika)
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?
Attachment #8923493 - Flags: review?(nika) → review+
(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
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/31f259ee387b
/mach clang-format: Better handling of relative paths r=mystor=mystor
(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.
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e1b4f786f584
/mach clang-format: Better handling of relative paths r=mystor=mystor
https://hg.mozilla.org/mozilla-central/rev/e1b4f786f584
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Flags: needinfo?(sledru)
See Also: → 1413612
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.