[Webkit-unassigned] [Bug 38156] many webkit-patch commands fail in a non-svn tracking git checkout

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=38156





--- Comment #7 from Ojan Vafai <ojan at chromium.org>  2010-05-25 11:10:40 PST ---
(In reply to comment #5)
> (From update of attachment 56907 [details])
> > +++ b/WebKitTools/Scripts/webkitpy/common/checkout/scm.py
> 
> > +    def _branch_exists(self, branch):
> > +        return self.run(['git', 'show-ref', '--quiet', '--verify', branch], return_exit_code=True) == 0
> 
> Would it be better to call the parameter something other than "branch" (e.g.
> "branch_path") so the caller knows to pass branch names in the form: refs/heads/<branch_name>.

Renamed to branch_ref.

> >      def delete_branch(self, branch):
> > -        if self.run(['git', 'show-ref', '--quiet', '--verify', 'refs/heads/' + branch], return_exit_code=True) == 0:
> > +        if self._branch_exists('refs/heads/' + branch):
> >              self.run(['git', 'branch', '-D', branch])
> 
> Similarly, would it be better to be more specific with the parameter name
> and call it something like branch_name (to distinguish from passing in a path)?

Done.

> >      def svn_branch_name(self):
> 
> Since with this patch this function is no longer being used exclusively for
> Git repos that track SVN, is there a more accurate name that can be used?

s/svn/remote here and in a couple other functions.

> > +        # A git clone of git://git.webkit.org/WebKit.git that is not tracking svn.
> > +        if self._branch_exists('refs/remotes/origin/master'):
> > +            return 'refs/remotes/origin/master'
> > +
> > +        raise ScriptError(message="\"trunk\" and \"origin/master\" branches do not exist. Can't find a branch to diff against.")
> 
> I recommend using single quotes -- especially for strings containing double quotes.
> That way you don't need to escape the double quotes.

Yeah. There is a single quote in there as well, but whatever. :)

> > +    def test_svn_branch_name_git_only(self):
> > +        git_checkout2_path = tempfile.mkdtemp(suffix="git_test_checkout2")
> > +        run_command(['git', 'clone', '--quiet', self.git_checkout_path,  git_checkout2_path])
> > +        os.chdir(git_checkout2_path)
> > +        scm = detect_scm_system(git_checkout2_path)
> > +        self.assertEqual(scm.svn_branch_name(), 'refs/remotes/origin/master')
> 
> Should there be some sort of setUp/tearDown specific to these methods or
> equivalent logic to set the working directory back to what is was before
> the test ran?  Also, for the tests that don't need to track SVN, can we get
> away without setting up the SVN repo in the setUp/tearDown?

That's probably better. They should run a lot faster that way.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.