Bug 87528 - webkitpy: change scm.add(), scm.delete() to accept multiple paths
Summary: webkitpy: change scm.add(), scm.delete() to accept multiple paths
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks: 87451
  Show dependency treegraph
 
Reported: 2012-05-25 12:34 PDT by Dirk Pranke
Modified: 2012-05-29 14:47 PDT (History)
5 users (show)

See Also:


Attachments
Patch (6.83 KB, patch)
2012-05-25 12:37 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
add tests (9.21 KB, patch)
2012-05-25 12:59 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
ignore - wrong bug (11.23 KB, patch)
2012-05-25 14:48 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2012-05-25 12:34:50 PDT
webkitpy: change scm.add(), scm.delete() to accept multiple paths
Comment 1 Dirk Pranke 2012-05-25 12:37:09 PDT
Created attachment 144124 [details]
Patch
Comment 2 Dirk Pranke 2012-05-25 12:59:16 PDT
Created attachment 144128 [details]
add tests
Comment 3 Dirk Pranke 2012-05-25 13:04:08 PDT
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
Comment 4 Ojan Vafai 2012-05-25 13:06:54 PDT
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. :)
Comment 5 Dirk Pranke 2012-05-25 13:48:11 PDT
Committed r118557: <http://trac.webkit.org/changeset/118557>
Comment 6 Dirk Pranke 2012-05-25 14:47:58 PDT
Reopening to attach new patch.
Comment 7 Dirk Pranke 2012-05-25 14:48:01 PDT
Created attachment 144150 [details]
ignore - wrong bug
Comment 8 Eric Seidel (no email) 2012-05-25 15:01:03 PDT
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.
Comment 9 Eric Seidel (no email) 2012-05-25 15:01:54 PDT
Ok, so where is the right bug so I can add my review comemnts there? :)
Comment 10 Eric Seidel (no email) 2012-05-25 15:20:16 PDT
(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?
Comment 11 Eric Seidel (no email) 2012-05-25 15:20:59 PDT
Sorry, I keep getting confused as to which bug when clicking on links in my bugmail.