Bug 208729 – upload.py gets confused by git-svn checkouts, unable to upload test results from my desk build

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Blaze Burg]

.

[Blaze Burg]

<rdar://problem/60105447>

[Blaze Burg]

Created attachment 392756 [details]
Patch

[Matt Lewis]

Please add Unit tests for the changes. The changes to stub_repository and base.py look good to me, but I'll refer to Jonathan.

[Jonathan Bedard]

Comment on attachment 392756 [details]
Patch

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

We also need some unit tests for this...probably going to involve mocking the git-svn-id command.

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:286
> +    def git_svn_id_regexp():

Seems weird as a static method, should probably be a class variable. Usually we do something like:
    GIT_SVN_ID_REGEX = r'....'

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:301
> +    def svn_revision(self, path):

If I'm understanding this, we're adding svn_revision to every git SCM now, right?

What is the result if we have a pure git repo (ie, no SVN)

> Tools/Scripts/webkitpy/port/base.py:1611
> +            # At the time of writing, bots update WebKit via SVN and are not affected by this.

This comment probably isn't needed.

If we're going to keep it, we should leave out the bot vs desk difference and just say 'For git-svn, prefer the SVN branch and revision'

[Blaze Burg]

(In reply to Jonathan Bedard from comment #4)
> Comment on attachment 392756 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=392756&action=review
> 
> We also need some unit tests for this...probably going to involve mocking
> the git-svn-id command.

I'll give it a try.

> > Tools/Scripts/webkitpy/common/checkout/scm/git.py:286
> > +    def git_svn_id_regexp():
> 
> Seems weird as a static method, should probably be a class variable. Usually
> we do something like:
>     GIT_SVN_ID_REGEX = r'....'
> 
> > Tools/Scripts/webkitpy/common/checkout/scm/git.py:301
> > +    def svn_revision(self, path):
> 
> If I'm understanding this, we're adding svn_revision to every git SCM now,
> right?
> 
> What is the result if we have a pure git repo (ie, no SVN)

svn_revision is existing, the - part of the diff is above the + parts. It returns "" in case it's a pure git repo.

This patch adds svn_branch, which behaves similarly to svn_revision in the case of a pure git repo.

> 
> > Tools/Scripts/webkitpy/port/base.py:1611
> > +            # At the time of writing, bots update WebKit via SVN and are not affected by this.
> 
> This comment probably isn't needed.
> 
> If we're going to keep it, we should leave out the bot vs desk difference
> and just say 'For git-svn, prefer the SVN branch and revision'

OK

[Jonathan Bedard]

(In reply to Brian Burg from comment #5)
> ....
> > > Tools/Scripts/webkitpy/common/checkout/scm/git.py:301
> > > +    def svn_revision(self, path):
> > 
> > If I'm understanding this, we're adding svn_revision to every git SCM now,
> > right?
> > 
> > What is the result if we have a pure git repo (ie, no SVN)
> 
> svn_revision is existing, the - part of the diff is above the + parts. It
> returns "" in case it's a pure git repo.
> 
> This patch adds svn_branch, which behaves similarly to svn_revision in the
> case of a pure git repo.

Perfect! That sounds like the correct behavior.

[Blaze Burg]

Comment on attachment 392756 [details]
Patch

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

>> Tools/Scripts/webkitpy/common/checkout/scm/git.py:301
>> +    def svn_revision(self, path):
> 
> If I'm understanding this, we're adding svn_revision to every git SCM now, right?
> 
> What is the result if we have a pure git repo (ie, no SVN)

If no git-svn-id can be found, then return "" per Line 298.

[Blaze Burg]

Created attachment 393058 [details]
Patch

[Jonathan Bedard]

Comment on attachment 393058 [details]
Patch

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

> Tools/Scripts/webkitpy/port/base.py:1612
> +            native_revision = scm.native_revision(path)

I don't think we need this outside of the if statement, should probably move it to the used_revision assignment.

> Tools/Scripts/webkitpy/port/base.py:1619
> +            native_branch = scm.native_branch(path)

Ditto.

[Blaze Burg]

Comment on attachment 393058 [details]
Patch

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

>> Tools/Scripts/webkitpy/port/base.py:1612
>> +            native_revision = scm.native_revision(path)
> 
> I don't think we need this outside of the if statement, should probably move it to the used_revision assignment.

Sure thing. I write stuff out this way to begin, as it's easier to debug. But I guess it could be condensed now.

[Jonathan Bedard]

(In reply to Brian Burg from comment #10)
> Comment on attachment 393058 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393058&action=review
> 
> >> Tools/Scripts/webkitpy/port/base.py:1612
> >> +            native_revision = scm.native_revision(path)
> > 
> > I don't think we need this outside of the if statement, should probably move it to the used_revision assignment.
> 
> Sure thing. I write stuff out this way to begin, as it's easier to debug.
> But I guess it could be condensed now.

Normally not a big deal, but SCM calls frequently result in relatively expensive filesystem operations.

[Blaze Burg]

Committed r258164: <https://trac.webkit.org/changeset/258164>