1387036 - ./mach clang-format - improve the git filter

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[Sylvestre Ledru [:Sylvestre]]

gps:
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.
----
glandium: Not sure about excluding files, but you can do `git diff ... -- *.cpp *.c *.h`

Comment 1

[Prithviraj Pawar]

Hey Sylvestre, I would like to work on this.  'git diff .. --*.[some_extension]*' works for excluding files based on extensions.

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Sure, please go ahead and propose a patch!

Comment 3

[Prithviraj Pawar]

I think there is a patch to be done in tools/mach_commands.py. Please confirm this Sylvestre. Is there any other file associated with this bug?

Comment 4

[Prithviraj Pawar]

Attachment: Bug 1387036 - Improved the ./mach clang-format git filter.

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

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8894768 [details]
Bug 1387036 - Improved the ./mach clang-format git filter.

https://reviewboard.mozilla.org/r/165936/#review171052

::: tools/mach_commands.py:270
(Diff revision 1)
>              diff_process = Popen(["hg", "diff", "-U0", "-r", ".^",
>                                    "--include", "glob:**.c", "--include", "glob:**.cpp",
>                                    "--include", "glob:**.h",
>                                    "--exclude", "listfile:.clang-format-ignore"], stdout=PIPE)
>          else:
> -            git_process = Popen(["git", "diff", "--no-color", "-U0", "HEAD^"], stdout=PIPE)
> +            git_process = Popen(["git", "diff", "--no-color", "-U0", "HEAD^","--","*.cpp","*.c","*.h","':!.clang-format-ignore'"], stdout=PIPE)

Why do you have to explicitly list .clang-format-ignore?
Because it is formatted otherwise?

Comment 6

[Prithviraj Pawar]

Comment on attachment 8894768 [details]
Bug 1387036 - Improved the ./mach clang-format git filter.

https://reviewboard.mozilla.org/r/165936/#review171052

previously filterdiff had an --exclude flag for it. i thought it needs to be excluded here as well

Comment 7

[Prithviraj Pawar]

Comment on attachment 8894768 [details]
Bug 1387036 - Improved the ./mach clang-format git filter.

https://reviewboard.mozilla.org/r/165936/#review171052

> Why do you have to explicitly list .clang-format-ignore?
> Because it is formatted otherwise?

previously filterdiff had an --exclude flag for it. i thought it needs to be excluded here as well

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8894768 [details]
Bug 1387036 - Improved the ./mach clang-format git filter.

https://reviewboard.mozilla.org/r/165936/#review171058

::: tools/mach_commands.py:270
(Diff revision 1)
>              diff_process = Popen(["hg", "diff", "-U0", "-r", ".^",
>                                    "--include", "glob:**.c", "--include", "glob:**.cpp",
>                                    "--include", "glob:**.h",
>                                    "--exclude", "listfile:.clang-format-ignore"], stdout=PIPE)
>          else:
> -            git_process = Popen(["git", "diff", "--no-color", "-U0", "HEAD^"], stdout=PIPE)
> +            git_process = Popen(["git", "diff", "--no-color", "-U0", "HEAD^","--","*.cpp","*.c","*.h","':!.clang-format-ignore'"], stdout=PIPE)

I don't think it is necessary with this change!
Thanks

Comment 9

[Prithviraj Pawar]

Attachment: Bug 1387036 ./mach clang-format - improve the git filter

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

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8894777 [details]
Bug 1387036 ./mach clang-format - improve the git filter

https://reviewboard.mozilla.org/r/165944/#review171062

::: tools/mach_commands.py:270
(Diff revision 1)
>              diff_process = Popen(["hg", "diff", "-U0", "-r", ".^",
>                                    "--include", "glob:**.c", "--include", "glob:**.cpp",
>                                    "--include", "glob:**.h",
>                                    "--exclude", "listfile:.clang-format-ignore"], stdout=PIPE)
>          else:
> -            git_process = Popen(["git", "diff", "--no-color", "-U0", "HEAD^","--","*.cpp","*.c","*.h","':!.clang-format-ignore'"], stdout=PIPE)
> +            git_process = Popen(["git", "diff", "--no-color", "-U0", "HEAD^","--","*.cpp","*.c","*.h"], stdout=PIPE)

You create a new commit, you should amend the other (sorry)

Comment 11

[Prithviraj Pawar]

Attachment: Bug 1387036 : ./mach clang-format , removed filterdiff and added "git diff -- *" for restricting diff to a given set of file extensions.

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

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8894891 [details]
Bug 1387036 : ./mach clang-format , removed filterdiff and added "git diff -- *" for restricting diff to a given set of file extensions.

https://reviewboard.mozilla.org/r/166044/#review171200

::: commit-message-a921b:1
(Diff revision 1)
> +Bug 1387036 : Improved git filter ./mach clang-format

Please explain a bit more what you did
(improved is too generic)

Comment 13

[Sylvestre Ledru [:Sylvestre]]

I transferred the review to gps because I am not owner of this code.

Comment 14

[Prithviraj Pawar]

Comment on attachment 8894891 [details]
Bug 1387036 : ./mach clang-format , removed filterdiff and added "git diff -- *" for restricting diff to a given set of file extensions.

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

Comment 15

[Prithviraj Pawar]

Comment on attachment 8894891 [details]
Bug 1387036 : ./mach clang-format , removed filterdiff and added "git diff -- *" for restricting diff to a given set of file extensions.

https://reviewboard.mozilla.org/r/166044/#review171322

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Several things: 
* please test your change. Seems that you didn't as it is failing with:
UnboundLocalError: local variable 'diff_process' referenced before assignment

* git diff --no-color -U0 HEAD^ -- *.cpp 
shows the list of every diff in *.cpp from the previous commit, not the local changes.

For example, with hg, we are doing
$ hg diff -U0 -r .^ dom/
which will show only the local changes in dom/

* you can probably simplify the code by having one Popen() declaration and using the
if
else:
just to declare the command

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8894891 [details]
Bug 1387036 : ./mach clang-format , removed filterdiff and added "git diff -- *" for restricting diff to a given set of file extensions.

https://reviewboard.mozilla.org/r/166044/#review171354

Comment 18

[Prithviraj Pawar]

(In reply to Sylvestre Ledru [:sylvestre] from comment #16)
> Several things: 
> * please test your change. Seems that you didn't as it is failing with:
> UnboundLocalError: local variable 'diff_process' referenced before assignment
> 
> * git diff --no-color -U0 HEAD^ -- *.cpp 
> shows the list of every diff in *.cpp from the previous commit, not the
> local changes.
> 
> For example, with hg, we are doing
> $ hg diff -U0 -r .^ dom/
> which will show only the local changes in dom/
> 
> * you can probably simplify the code by having one Popen() declaration and
> using the
> if
> else:
> just to declare the command

HEAD^ shows all the changes before and after commit . If we use HEAD then it shows only the changes after the latest commit. Please verfiy?

Comment 19

[Sylvestre Ledru [:Sylvestre]]

"hg clang-format" should be applied only on the local changes (not committed)

For example:
$ git diff dom/presentation/AvailabilityCollection.cpp
diff --git a/dom/presentation/AvailabilityCollection.cpp b/dom/presentation/AvailabilityCollection.cpp
index 73752c750524..3282efbd6baf 100644
--- a/dom/presentation/AvailabilityCollection.cpp
+++ b/dom/presentation/AvailabilityCollection.cpp
@@ -10,7 +10,7 @@
 #include "PresentationAvailability.h"
 
 namespace mozilla {
-namespace dom {
+                       namespace dom {
 
 /* static */
 StaticAutoPtr<AvailabilityCollection>

$ git diff --no-color -U0 HEAD^ -- *.cpp 
diff --git a/dom/presentation/AvailabilityCollection.cpp b/dom/presentation/AvailabilityCollection.cpp
index 73752c750524..3282efbd6baf 100644
--- a/dom/presentation/AvailabilityCollection.cpp
+++ b/dom/presentation/AvailabilityCollection.cpp
@@ -13 +13 @@ namespace mozilla {
-namespace dom {
+                       namespace dom {
diff --git a/image/ImageFactory.cpp b/image/ImageFactory.cpp
index 11e2703a4a24..8c62a23ac8f9 100644
--- a/image/ImageFactory.cpp
+++ b/image/ImageFactory.cpp
@@ -99 +99,2 @@ ImageFactory::CreateImage(nsIRequest* aRequest,
-    if (NS_WARN_IF(obs)) {
+    NS_WARNING_ASSERTION(obs, "Can't get an observer service handle");
+    if (obs) {
diff --git a/image/RasterImage.cpp b/image/RasterImage.cpp
index 3586f322639f..a74204bb30e8 100644
--- a/image/RasterImage.cpp
[...]

I should get only the first file, the other are from another previous commit (4346bdf652efd32d614191d50215d3b553589c51 here)

Comment 20

[Prithviraj Pawar]

Yeah I got it. You can use $ git diff --no-color -U0 HEAD -- *.cpp      
It shows only the local changes

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Good, please update your patch then :)

Comment 22

[Prithviraj Pawar]

yeah !!

Comment 23

[Prithviraj Pawar]

Attachment: Bug 1387036 : ./mach clang-format , removed filterdiff and added "git diff -- *" for restricting diff to a given set of file extensions.

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

Comment 24

[Sylvestre Ledru [:Sylvestre]]

If I understood correctly, the git part never worked as planned, right?

Comment 25

[Prithviraj Pawar]

$git diff HEAD ....will show all the changes since the previous commit in local repo...also $filterdiff is avoided...

Comment 26

[Prithviraj Pawar]

Comment on attachment 8896626 [details]
Bug 1387036 : ./mach clang-format , removed filterdiff and added "git diff -- *" for restricting diff to a given set of file extensions.

https://reviewboard.mozilla.org/r/167912/#review173120

Comment 27

[Prithviraj Pawar]

Comment on attachment 8896626 [details]
Bug 1387036 : ./mach clang-format , removed filterdiff and added "git diff -- *" for restricting diff to a given set of file extensions.

https://reviewboard.mozilla.org/r/167912/#review173132

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8896626 [details]
Bug 1387036 : ./mach clang-format , removed filterdiff and added "git diff -- *" for restricting diff to a given set of file extensions.

https://reviewboard.mozilla.org/r/167912/#review173140

::: tools/mach_commands.py:270
(Diff revision 1)
>              diff_process = Popen(["hg", "diff", "-U0", "-r", ".^",
>                                    "--include", "glob:**.c", "--include", "glob:**.cpp",
>                                    "--include", "glob:**.h",
>                                    "--exclude", "listfile:.clang-format-ignore"], stdout=PIPE)
>          else:
> -            git_process = Popen(["git", "diff", "--no-color", "-U0", "HEAD^"], stdout=PIPE)
> +	    diff_process = Popen(["git", "diff", "--no-color", "-U0", "HEAD","--","*.c","*.cpp","*.h"], stdout=PIPE) 

please remove the trailing whitespace

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8896626 [details]
Bug 1387036 : ./mach clang-format , removed filterdiff and added "git diff -- *" for restricting diff to a given set of file extensions.

Besides that, works great!

Comment 30

[Prithviraj Pawar]

Comment on attachment 8896626 [details]
Bug 1387036 : ./mach clang-format , removed filterdiff and added "git diff -- *" for restricting diff to a given set of file extensions.

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

Comment 31

[Prithviraj Pawar]

Comment on attachment 8896626 [details]
Bug 1387036 : ./mach clang-format , removed filterdiff and added "git diff -- *" for restricting diff to a given set of file extensions.

https://reviewboard.mozilla.org/r/167912/#review173140

> please remove the trailing whitespace

sure.

Comment 32

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8896626 [details]
Bug 1387036 : ./mach clang-format , removed filterdiff and added "git diff -- *" for restricting diff to a given set of file extensions.

https://reviewboard.mozilla.org/r/167912/#review173146

Bravo. There are some room for improvements, are you interested?

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b941b5a3550d
/mach clang-format , removed filterdiff and added "git diff -- *" for restricting diff to a given set of file extensions. r=sylvestre

Comment 34

[Prithviraj Pawar]

Comment on attachment 8896626 [details]
Bug 1387036 : ./mach clang-format , removed filterdiff and added "git diff -- *" for restricting diff to a given set of file extensions.

https://reviewboard.mozilla.org/r/167912/#review173150

Comment 35

[Prithviraj Pawar]

Comment on attachment 8896626 [details]
Bug 1387036 : ./mach clang-format , removed filterdiff and added "git diff -- *" for restricting diff to a given set of file extensions.

https://reviewboard.mozilla.org/r/167912/#review173146

Thanks!!
Yeah. I am interested.

Comment 36

[Sylvestre Ledru [:Sylvestre]]

This is bug 1389956

Comment 37

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

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

Comment 38

[Jeff Muizelaar [:jrmuizel]]

This change seems to created a behaviour difference between the 'hg' and 'git' versions.

hg diff .^ includes the changes from the current commit
git diff HEAD does not include those changes.

Is this behaviour difference intentional?

Comment 39

[Sylvestre Ledru [:Sylvestre]]

nope, we want the same experience with git & hg.
Could you please open a new bug about that?
thanks

Comment 40

[Jeff Muizelaar [:jrmuizel]]

I filed bug 1674281