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

RESOLVED FIXED in Firefox 57

Status

enhancement
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: sylvestre, Assigned: dex, Mentored)

Tracking

(Blocks 1 bug, {good-first-bug})

Trunk
mozilla57
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [good first bug][lang=python])

Attachments

(1 attachment)

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."
Mentor: sledru
Keywords: good-first-bug
Whiteboard: [good first bug][lang=python]
Hi Sylvestre, I would like to work on this.
sure, please give it a try!
Attachment #8897597 - Flags: review?(sledru) → review?(gps)
gps is the pro for this, redirecting to him the review.
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``.
Attachment #8897597 - Flags: review?(gps) → review-
(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 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 on attachment 8897597 [details]
Bug 1387035 - Update clang-format methods to use mozversioncontrol for hg/git detection

https://reviewboard.mozilla.org/r/168872/#review174720
Attachment #8897597 - Flags: review?(gps) → review+
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0828f347805f
Update clang-format methods to use mozversioncontrol for hg/git detection r=gps
https://hg.mozilla.org/mozilla-central/rev/0828f347805f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Bravo for your patch!
thanks!
Assignee: nobody → dex
Attachment #8897597 - Flags: review?(sledru)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.