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

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[Sylvestre Ledru [:Sylvestre]]

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 1

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1376803 - Move the clang-format diff into a specific function

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

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1376803 - add support of ./mach clang-format -p <file/dir>

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

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8881790 [details]
Bug 1376803 - Move the clang-format diff into a specific function

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

Comment 4

[Sylvestre Ledru [:Sylvestre]]

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

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

Comment 5

[Sylvestre Ledru [:Sylvestre]]

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

Comment 6

[Gregory Szorc [:gps]]

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.

Comment 7

[Gregory Szorc [:gps]]

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.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

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

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

Comment 9

[Sylvestre Ledru [:Sylvestre]]

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

[Gregory Szorc [:gps]]

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

[Gregory Szorc [:gps]]

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

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6f9a5dfd2259
https://hg.mozilla.org/mozilla-central/rev/d034fc43e7f6

Comment 14

[Mike Hommey [:glandium]]

(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`