1387035 - ./mach clang-format - use python/mozversioncontrol for hg/git detection

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[Sylvestre Ledru [:Sylvestre]]

Comment from gps in bug 1376803:

"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."

Comment 1

[Xavier ALT [:dex]]

Hi Sylvestre, I would like to work on this.

Comment 2

[Sylvestre Ledru [:Sylvestre]]

sure, please give it a try!

Comment 3

[Xavier ALT [:dex]]

Attachment: Bug 1387035 - Update clang-format methods to use mozversioncontrol for hg/git detection

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

Comment 4

[Sylvestre Ledru [:Sylvestre]]

gps is the pro for this, redirecting to him the review.

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8897597 [details]
Bug 1387035 - Update clang-format methods to use mozversioncontrol for hg/git detection

https://reviewboard.mozilla.org/r/168872/#review174238

::: tools/mach_commands.py:265
(Diff revision 1)
>      def run_clang_format_diff(self, clang_format_diff, show):
>          # Run clang-format on the diff
>          # Note that this will potentially miss a lot things
>          from subprocess import Popen, PIPE
>  
> -        if os.path.exists(".hg"):
> +        repository = mozversioncontrol.get_repository_object(self.topsrcdir)

Use ``self.repository``.

::: tools/mach_commands.py:267
(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)
>              try:
>                  diff_process = Popen(["filterdiff", "--include=*.h",

Ideally these pieces would be abstracted and exposed as a method on the repository object. But that's a bit of scope bloat. The change as-is is better than it was before.

::: tools/mach_commands.py:340
(Diff revision 1)
>  
>          # Run clang-format
>          cf_process = Popen(args)
>          if show:
>              # show the diff
> -            if os.path.exists(".hg"):
> +            repository = mozversioncontrol.get_repository_object(self.topsrcdir)

Use ``self.repository``.

Comment 6

[Xavier ALT [:dex]]

(In reply to Gregory Szorc [:gps] from comment #5)

> ::: tools/mach_commands.py:267
> (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)
> >              try:
> >                  diff_process = Popen(["filterdiff", "--include=*.h",
> 
> Ideally these pieces would be abstracted and exposed as a method on the
> repository object. But that's a bit of scope bloat. The change as-is is
> better than it was before.
> 

I can also do that change, but I need some advise about the what the method signature should look like.

All current methods of mozversioncontrol Repository class are returning full output string, never a PIPE, so I was thinking to introduce:

def get_changes_to_format(self, pipe=False):
    """Return diffs of modified files that needs to be formatted.

    :param pipe: if True return a Popen pipe, else the output string.
    """

Are you ok with that ?

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8897597 [details]
Bug 1387035 - Update clang-format methods to use mozversioncontrol for hg/git detection

https://reviewboard.mozilla.org/r/168872/#review174498

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

This code has been removed in bug 1387036
please update your code!
thanks

Comment 8

[Xavier ALT [:dex]]

Comment on attachment 8897597 [details]
Bug 1387035 - Update clang-format methods to use mozversioncontrol for hg/git detection

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

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8897597 [details]
Bug 1387035 - Update clang-format methods to use mozversioncontrol for hg/git detection

https://reviewboard.mozilla.org/r/168872/#review174720

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0828f347805f
Update clang-format methods to use mozversioncontrol for hg/git detection r=gps

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0828f347805f

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Bravo for your patch!
thanks!