Bug 208729 - upload.py gets confused by git-svn checkouts, unable to upload test results from my desk build
Summary: upload.py gets confused by git-svn checkouts, unable to upload test results f...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-06 12:24 PST by BJ Burg
Modified: 2020-03-09 14:50 PDT (History)
7 users (show)

See Also:


Attachments
Patch (8.06 KB, patch)
2020-03-06 12:38 PST, BJ Burg
no flags Details | Formatted Diff | Diff
Patch (10.61 KB, patch)
2020-03-09 11:33 PDT, BJ Burg
jbedard: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2020-03-06 12:24:05 PST
.
Comment 1 BJ Burg 2020-03-06 12:24:23 PST
<rdar://problem/60105447>
Comment 2 BJ Burg 2020-03-06 12:38:17 PST
Created attachment 392756 [details]
Patch
Comment 3 Matt Lewis 2020-03-06 13:24:11 PST
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.
Comment 4 Jonathan Bedard 2020-03-06 13:32:56 PST
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'
Comment 5 BJ Burg 2020-03-06 17:00:52 PST
(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
Comment 6 Jonathan Bedard 2020-03-06 17:04:47 PST
(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.
Comment 7 BJ Burg 2020-03-09 09:26:07 PDT
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.
Comment 8 BJ Burg 2020-03-09 11:33:21 PDT
Created attachment 393058 [details]
Patch
Comment 9 Jonathan Bedard 2020-03-09 13:07:16 PDT
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.
Comment 10 BJ Burg 2020-03-09 14:36:46 PDT
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.
Comment 11 Jonathan Bedard 2020-03-09 14:49:01 PDT
(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.
Comment 12 BJ Burg 2020-03-09 14:50:29 PDT
Committed r258164: <https://trac.webkit.org/changeset/258164>