Changeset 57361 in webkit


Ignore:
Timestamp:
Apr 9, 2010 2:01:32 PM (14 years ago)
Author:
eric@webkit.org
Message:

2010-04-09 Eric Seidel <eric@webkit.org>

Reviewed by Adam Barth.

webkit-patch attached my patch to the wrong bug
https://bugs.webkit.org/show_bug.cgi?id=37015

The problem here is that SVN was violating SCM's implicit
contract of always returning paths relative to the repository root.
That can easily be fixed by telling SVN that the CWD is the repository root.

When fixing this I realized there are a large number of places in SCM.py where
we want to consider explicitly passing self.checkout_root as the CWD.
That would allow scm methods to be executed even when the CWD is not inside
the scm tree at all, and would also make sure (in the case of SVN) that paths
returned are relative to the root. Git (almost always) returns paths relative
to the repository root.

  • Scripts/webkitpy/common/checkout/scm.py:
    • Explicitly pass self.checkout_root as cwd in run_status_and_extract_filenames
    • Add a ton of FIXMEs about the need to go back and decide which methods require cwd=self.checkout_root and which do not. We'll probably add a helper function to scm (likely SCM._run) which always passes cwd=self.checkout_root to Executive.run_command
  • Scripts/webkitpy/common/checkout/scm_unittest.py:
    • Add a test for this change.
  • Scripts/webkitpy/tool/commands/upload.py:
    • Removed the explicit os.chdir to the repository root, since scm.py methods should be robust against the cwd not being equal to the root.
Location:
trunk/WebKitTools
Files:
4 edited

Legend:

Unmodified
Added
Removed
  • trunk/WebKitTools/ChangeLog

    r57339 r57361  
     12010-04-09  Eric Seidel  <eric@webkit.org>
     2
     3        Reviewed by Adam Barth.
     4
     5        webkit-patch attached my patch to the wrong bug
     6        https://bugs.webkit.org/show_bug.cgi?id=37015
     7
     8        The problem here is that SVN was violating SCM's implicit
     9        contract of always returning paths relative to the repository root.
     10        That can easily be fixed by telling SVN that the CWD is the repository root.
     11
     12        When fixing this I realized there are a large number of places in SCM.py where
     13        we want to consider explicitly passing self.checkout_root as the CWD.
     14        That would allow scm methods to be executed even when the CWD is not inside
     15        the scm tree at all, and would also make sure (in the case of SVN) that paths
     16        returned are relative to the root.  Git (almost always) returns paths relative
     17        to the repository root.
     18
     19        * Scripts/webkitpy/common/checkout/scm.py:
     20         - Explicitly pass self.checkout_root as cwd in run_status_and_extract_filenames
     21         - Add a ton of FIXMEs about the need to go back and decide which methods require cwd=self.checkout_root
     22           and which do not.  We'll probably add a helper function to scm (likely SCM._run) which
     23           always passes cwd=self.checkout_root to Executive.run_command
     24        * Scripts/webkitpy/common/checkout/scm_unittest.py:
     25         - Add a test for this change.
     26        * Scripts/webkitpy/tool/commands/upload.py:
     27         - Removed the explicit os.chdir to the repository root, since scm.py methods
     28           should be robust against the cwd not being equal to the root.
     29
    1302010-04-09  Adam Roben  <aroben@apple.com>
    231
  • trunk/WebKitTools/Scripts/webkitpy/common/checkout/scm.py

    r57091 r57361  
    3333import re
    3434
     35# FIXME: Instead of using run_command directly, most places in this
     36# class would rather use an SCM.run method which automatically set
     37# cwd=self.checkout_root.
    3538from webkitpy.common.system.executive import Executive, run_command, ScriptError
    3639from webkitpy.common.system.user import User
     
    9194
    9295
     96# SCM methods are expected to return paths relative to self.checkout_root.
    9397class SCM:
    9498    def __init__(self, cwd):
     
    105109    def ensure_clean_working_directory(self, force_clean):
    106110        if not force_clean and not self.working_directory_is_clean():
     111            # FIXME: Shouldn't this use cwd=self.checkout_root?
    107112            print run_command(self.status_command(), error_handler=Executive.ignore_error)
    108113            raise ScriptError(message="Working directory has modifications, pass --force-clean or --no-clean to continue.")
     
    123128    def run_status_and_extract_filenames(self, status_command, status_regexp):
    124129        filenames = []
    125         for line in run_command(status_command).splitlines():
     130        # We run with cwd=self.checkout_root so that returned-paths are root-relative.
     131        for line in run_command(status_command, cwd=self.checkout_root).splitlines():
    126132            match = re.search(status_regexp, line)
    127133            if not match:
     
    290296
    291297    def working_directory_is_clean(self):
     298        # FIXME: Should this use cwd=self.checkout_root?
    292299        return run_command(['svn', 'diff']) == ""
    293300
    294301    def clean_working_directory(self):
     302        # FIXME: Shouldn't this use cwd=self.checkout_root?
    295303        run_command(['svn', 'revert', '-R', '.'])
    296304
     
    331339
    332340    def diff_for_revision(self, revision):
     341        # FIXME: This should probably use cwd=self.checkout_root
    333342        return run_command(['svn', 'diff', '-c', str(revision)])
    334343
     
    341350        log("WARNING: svn merge has been known to take more than 10 minutes to complete.  It is recommended you use git for rollouts.")
    342351        log("Running '%s'" % " ".join(svn_merge_args))
     352        # FIXME: Should this use cwd=self.checkout_root?
    343353        run_command(svn_merge_args)
    344354
    345355    def revert_files(self, file_paths):
     356        # FIXME: This should probably use cwd=self.checkout_root.
    346357        run_command(['svn', 'revert'] + file_paths)
    347358
     
    358369            svn_commit_args.extend(["--username", username])
    359370        svn_commit_args.extend(["-m", message])
     371        # FIXME: Should this use cwd=self.checkout_root?
    360372        return run_command(svn_commit_args, error_handler=commit_error_handler)
    361373
     
    389401    @classmethod
    390402    def read_git_config(cls, key):
     403        # FIXME: This should probably use cwd=self.checkout_root.
    391404        return run_command(["git", "config", key],
    392405            error_handler=Executive.ignore_error).rstrip('\n')
     
    397410
    398411    def discard_local_commits(self):
     412        # FIXME: This should probably use cwd=self.checkout_root
    399413        run_command(['git', 'reset', '--hard', self.svn_branch_name()])
    400414   
    401415    def local_commits(self):
     416        # FIXME: This should probably use cwd=self.checkout_root
    402417        return run_command(['git', 'log', '--pretty=oneline', 'HEAD...' + self.svn_branch_name()]).splitlines()
    403418
     
    406421
    407422    def working_directory_is_clean(self):
     423        # FIXME: This should probably use cwd=self.checkout_root
    408424        return run_command(['git', 'diff', 'HEAD', '--name-only']) == ""
    409425
    410426    def clean_working_directory(self):
     427        # FIXME: These should probably use cwd=self.checkout_root.
    411428        # Could run git clean here too, but that wouldn't match working_directory_is_clean
    412429        run_command(['git', 'reset', '--hard', 'HEAD'])
     
    447464
    448465    def create_patch(self):
     466        # FIXME: This should probably use cwd=self.checkout_root
    449467        return run_command(['git', 'diff', '--binary', 'HEAD'])
    450468
    451469    @classmethod
    452470    def git_commit_from_svn_revision(cls, revision):
     471        # FIXME: This should probably use cwd=self.checkout_root
    453472        git_commit = run_command(['git', 'svn', 'find-rev', 'r%s' % revision]).rstrip()
    454473        # git svn find-rev always exits 0, even when the revision is not found.
  • trunk/WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py

    r57091 r57361  
    7070# Exists to share svn repository creation code between the git and svn tests
    7171class SVNTestRepository:
    72     @staticmethod
    73     def _setup_test_commits(test_object):
     72    @classmethod
     73    def _svn_add(cls, path):
     74        run_command(["svn", "add", path])
     75
     76    @classmethod
     77    def _svn_commit(cls, message):
     78        run_command(["svn", "commit", "--quiet", "--message", message])
     79
     80    @classmethod
     81    def _setup_test_commits(cls, test_object):
    7482        # Add some test commits
    7583        os.chdir(test_object.svn_checkout_path)
    76         test_file = open('test_file', 'w')
    77         test_file.write("test1")
    78         test_file.flush()
    79 
    80         run_command(['svn', 'add', 'test_file'])
    81         run_command(['svn', 'commit', '--quiet', '--message', 'initial commit'])
    82        
    83         test_file.write("test2")
    84         test_file.flush()
    85        
    86         run_command(['svn', 'commit', '--quiet', '--message', 'second commit'])
    87        
    88         test_file.write("test3\n")
    89         test_file.flush()
     84
     85        write_into_file_at_path("test_file", "test1")
     86        cls._svn_add("test_file")
     87        cls._svn_commit("initial commit")
     88
     89        write_into_file_at_path("test_file", "test1test2")
     90        # This used to be the last commit, but doing so broke
     91        # GitTest.test_apply_git_patch which use the inverse diff of the last commit.
     92        # svn-apply fails to remove directories in Git, see:
     93        # https://bugs.webkit.org/show_bug.cgi?id=34871
     94        os.mkdir("test_dir")
     95        # Slash should always be the right path separator since we use cygwin on Windows.
     96        test_file3_path = "test_dir/test_file3"
     97        write_into_file_at_path(test_file3_path, "third file")
     98        cls._svn_add("test_dir")
     99        cls._svn_commit("second commit")
     100
     101        write_into_file_at_path("test_file", "test1test2test3\n")
    90102        write_into_file_at_path("test_file2", "second file")
    91         run_command(['svn', 'add', 'test_file2'])
    92 
    93         run_command(['svn', 'commit', '--quiet', '--message', 'third commit'])
    94 
    95         test_file.write("test4\n")
    96         test_file.close()
    97 
    98         run_command(['svn', 'commit', '--quiet', '--message', 'fourth commit'])
     103        cls._svn_add("test_file2")
     104        cls._svn_commit("third commit")
     105
     106        write_into_file_at_path("test_file", "test1test2test3\ntest4\n")
     107        cls._svn_commit("fourth commit")
    99108
    100109        # svn does not seem to update after commit as I would expect.
     
    204213        self.assertEqual(self.scm.svn_revision_from_commit_text(commit_text), '0')
    205214
     215    def _shared_test_changed_files(self):
     216        write_into_file_at_path("test_file", "changed content")
     217        self.assertEqual(self.scm.changed_files(), ["test_file"])
     218        write_into_file_at_path("test_dir/test_file3", "new stuff")
     219        self.assertEqual(self.scm.changed_files(), ["test_dir/test_file3", "test_file"])
     220        old_cwd = os.getcwd()
     221        os.chdir("test_dir")
     222        # Validate that changed_files does not change with our cwd, see bug 37015.
     223        self.assertEqual(self.scm.changed_files(), ["test_dir/test_file3", "test_file"])
     224        os.chdir(old_cwd)
     225
    206226    def _shared_test_changed_files_for_revision(self):
    207         self.assertEqual(self.scm.changed_files_for_revision(2), ["test_file"])
     227        # SVN reports directory changes, Git does not.
     228        changed_files = self.scm.changed_files_for_revision(2)
     229        if "test_dir" in changed_files:
     230            changed_files.remove("test_dir")
     231        self.assertEqual(changed_files, ["test_dir/test_file3", "test_file"])
    208232        self.assertEqual(sorted(self.scm.changed_files_for_revision(3)), sorted(["test_file", "test_file2"])) # Git and SVN return different orders.
    209233        self.assertEqual(self.scm.changed_files_for_revision(4), ["test_file"])
     
    428452
    429453    def test_create_patch_is_full_patch(self):
    430         test_dir_path = os.path.join(self.svn_checkout_path, 'test_dir')
     454        test_dir_path = os.path.join(self.svn_checkout_path, "test_dir2")
    431455        os.mkdir(test_dir_path)
    432456        test_file_path = os.path.join(test_dir_path, 'test_file2')
    433457        write_into_file_at_path(test_file_path, 'test content')
    434         run_command(['svn', 'add', 'test_dir'])
     458        run_command(['svn', 'add', 'test_dir2'])
    435459
    436460        # create_patch depends on 'svn-create-patch', so make a dummy version.
     
    528552        self._shared_test_svn_apply_git_patch()
    529553
     554    def test_changed_files(self):
     555        self._shared_test_changed_files()
     556
    530557    def test_changed_files_for_revision(self):
    531558        self._shared_test_changed_files_for_revision()
     
    656683    def test_apply_git_patch(self):
    657684        scm = detect_scm_system(self.git_checkout_path)
     685        # We carefullly pick a diff which does not have a directory addition
     686        # as currently svn-apply will error out when trying to remove directories
     687        # in Git: https://bugs.webkit.org/show_bug.cgi?id=34871
    658688        patch = self._create_patch(run_command(['git', 'diff', 'HEAD..HEAD^']))
    659689        self._setup_webkittools_scripts_symlink(scm)
     
    708738        self.assertEqual(patch_from_local_commit, patch_since_local_commit)
    709739
     740    def test_changed_files(self):
     741        self._shared_test_changed_files()
     742
    710743    def test_changed_files_for_revision(self):
    711744        self._shared_test_changed_files_for_revision()
  • trunk/WebKitTools/Scripts/webkitpy/tool/commands/upload.py

    r56863 r57361  
    5353
    5454    def execute(self, options, args, tool):
    55         os.chdir(tool.scm().checkout_root)
     55        # This command is a useful test to make sure commit_message_for_this_commit
     56        # always returns the right value regardless of the current working directory.
    5657        print "%s" % tool.checkout().commit_message_for_this_commit().message()
    5758
Note: See TracChangeset for help on using the changeset viewer.