Bug 87528 – webkitpy: change scm.add(), scm.delete() to accept multiple paths

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

webkitpy: change scm.add(), scm.delete() to accept multiple paths

[Dirk Pranke]

Created attachment 144124 [details]
Patch

[Dirk Pranke]

Created attachment 144128 [details]
add tests

[Dirk Pranke]

I'm not actually wild about adding add_list() and delete_list() ... it's kind of crufty. The alternatives would be to change add() and delete() to require lists and update all the calling code, or to change add() and delete() to detect whether the parameter is a single string or a list of strings and do the right thing. I haven't convinced myself that either of those solutions is better, either

[Ojan Vafai]

Comment on attachment 144128 [details]
add tests

View in context: https://bugs.webkit.org/attachment.cgi?id=144128&action=review

> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1187
> -        self._second_local_commt()
> +        self._second_local_commit()

lol whoops. :)

[Dirk Pranke]

Committed r118557: <http://trac.webkit.org/changeset/118557>

[Dirk Pranke]

Reopening to attach new patch.

[Dirk Pranke]

Created attachment 144150 [details]
ignore - wrong bug

[Eric Seidel (no email)]

Comment on attachment 144150 [details]
ignore - wrong bug

View in context: https://bugs.webkit.org/attachment.cgi?id=144150&action=review

> Tools/Scripts/webkitpy/common/system/executive.py:464
> +def _run_command_thunk(cmd_line_and_cwd):
> +    (cmd_line, cwd) = cmd_line_and_cwd
> +    proc = subprocess.Popen(cmd_line, cwd=cwd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
> +    stdout, stderr = proc.communicate()
> +    return (proc.returncode, stdout, stderr)

This could easily have been a class method instead of a module method.

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:67
> +        self._defer_scm_adds = False

It doesn't feel like it's really "defer"ing anything.  It's just printing instead of doing.

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:126
> +            print "scm-add:%s" % path

Do you have any testing of this printing and parsing of the resulting values?  Spaces in file names?  unicode?

> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:194
> +        files_to_add = set()
> +        for output in [result[1] for result in command_results]:
> +            files_to_add.update(line.replace('scm-add:', '') for line in output.splitlines() if line.startswith('scm-add:'))
> +        scm.add_list(list(files_to_add))

Yeah, this should be tested.  You'll likely have to make it a separate function.  The whole idea of serliazing/deserializing the add list is split across two classes currently.

> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:209
> +def run_command(cmd_line_and_cwd):
> +    (cmd_line, cwd) = cmd_line_and_cwd
> +    host = Host()
> +    output = host.executive.run_command(cmd_line, cwd=cwd)

:(  You really don't want to instantiate a new host per command.

[Eric Seidel (no email)]

Ok, so where is the right bug so I can add my review comemnts there? :)

[Eric Seidel (no email)]

(In reply to comment #8)
> (From update of attachment 144150 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144150&action=review
> 
> > Tools/Scripts/webkitpy/common/system/executive.py:464
> > +def _run_command_thunk(cmd_line_and_cwd):
> > +    (cmd_line, cwd) = cmd_line_and_cwd
> > +    proc = subprocess.Popen(cmd_line, cwd=cwd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
> > +    stdout, stderr = proc.communicate()
> > +    return (proc.returncode, stdout, stderr)
> 
> This could easily have been a class method instead of a module method.

Sorry, I should have said "static".

@staticmethod
def _run_command_thunk(command_line_and_cwd):
    pass

MyClass._run_command_thunk
should then do exactly what you want, no?

[Eric Seidel (no email)]

Sorry, I keep getting confused as to which bug when clicking on links in my bugmail.