"./mach clang-format" add the support to reformat a file/directory

RESOLVED FIXED in Firefox 56

Status

enhancement
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: sylvestre, Assigned: sylvestre)

Tracking

(Blocks 1 bug)

unspecified
mozilla56
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
The proposed option is the following:
./mach clang-format -p dom/base/BorrowedAttrInfo.cpp  dom/script/ -s
-p to provide paths
-s to show the diff

For now, it won't do a recursive transformation on a dir, just the files. I can change that if needed.
Comment hidden (mozreview-request)
Assignee: nobody → sledru
(Assignee)

Updated

2 years ago
Attachment #8881790 - Flags: review?(gps)
Attachment #8881791 - Flags: review?(gps)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

2 years ago
I did a few improvements:
* ignore directories in listed in thirdpartypaths.txt
* recursive on directory
* only update C/C++ files

Comment 6

2 years ago
mozreview-review
Comment on attachment 8881790 [details]
Bug 1376803 - Move the clang-format diff into a specific function

https://reviewboard.mozilla.org/r/152862/#review160412

Rubber stamp r+ on a refactor.
Attachment #8881790 - Flags: review?(gps) → review+

Comment 7

2 years ago
mozreview-review
Comment on attachment 8881791 [details]
Bug 1376803 - add support of ./mach clang-format -p <file/dir>

https://reviewboard.mozilla.org/r/152864/#review160418

Nice feature. r- because new code needs to support Git (or at the very least abort if !hg - and have a bug on file to support Git).

Also, if you write version control code, consider sticking it in python/mozversioncontrol. Not a blocker. But a nice to have so we don't reinvent as many wheels.

::: tools/mach_commands.py:272
(Diff revision 2)
> -                diff_process = Popen(["filterdiff", "--include=*.h", "--include=*.cpp",
> +                diff_process = Popen(["filterdiff", "--include=*.h",
> +                                      "--include=*.cpp", "--include=*.c",
>                                        "--exclude-from-file=.clang-format-ignore"],
>                                       stdin=git_process.stdout, stdout=PIPE)

Alternatively, you can first run `git diff --name-only` to print a list of files in the diff. Then you can invoke `git diff .. -- <paths>` on only the files you care about.

Also, I'm kinda surprised git doesn't have a built-in command to filter diffs.

::: tools/mach_commands.py:309
(Diff revision 2)
> +                continue
> +
> +            if os.path.isdir(f):
> +                # Processing a directory, generate the file list
> +                for folder, subs, files in os.walk(f):
> +                    for filename in files:

If you want this to be deterministic, throw a sorted() around files. And do a `subs.sort()` for in-place sort.

::: tools/mach_commands.py:337
(Diff revision 2)
> +
> +        # Run clang-format
> +        cf_process = Popen(args)
> +        if show:
> +            # show the diff
> +            cf_process = Popen(["hg", "diff"] + path_list)

This assumes Mercurial whereas other parts of this code support Git. So this needs to support Git.
Attachment #8881791 - Flags: review?(gps) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 9

2 years ago
mozreview-review-reply
Comment on attachment 8881791 [details]
Bug 1376803 - add support of ./mach clang-format -p <file/dir>

https://reviewboard.mozilla.org/r/152864/#review160418

> Alternatively, you can first run `git diff --name-only` to print a list of files in the diff. Then you can invoke `git diff .. -- <paths>` on only the files you care about.
> 
> Also, I'm kinda surprised git doesn't have a built-in command to filter diffs.

If you don't mind, I will that into a follow up bug

Comment 10

2 years ago
mozreview-review-reply
Comment on attachment 8881791 [details]
Bug 1376803 - add support of ./mach clang-format -p <file/dir>

https://reviewboard.mozilla.org/r/152864/#review160418

> If you don't mind, I will that into a follow up bug

I almost never mind punting things to a follow-up bug: perfect is the enemy of good.

Comment 11

2 years ago
mozreview-review
Comment on attachment 8881791 [details]
Bug 1376803 - add support of ./mach clang-format -p <file/dir>

https://reviewboard.mozilla.org/r/152864/#review160764
Attachment #8881791 - Flags: review?(gps) → review+

Comment 12

2 years ago
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6f9a5dfd2259
Move the clang-format diff into a specific function r=gps
https://hg.mozilla.org/integration/autoland/rev/d034fc43e7f6
add support of ./mach clang-format -p <file/dir> r=gps

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6f9a5dfd2259
https://hg.mozilla.org/mozilla-central/rev/d034fc43e7f6
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(In reply to Gregory Szorc [:gps] from comment #7)
> Alternatively, you can first run `git diff --name-only` to print a list of
> files in the diff. Then you can invoke `git diff .. -- <paths>` on only the
> files you care about.
> 
> Also, I'm kinda surprised git doesn't have a built-in command to filter
> diffs.

Not sure about excluding files, but you can do `git diff ... -- *.cpp *.c *.h`
(Assignee)

Updated

2 years ago
Depends on: 1387035
(Assignee)

Updated

2 years ago
Depends on: 1387036

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.