Bug 201206 – Tooling sets up WebKit repository using HTTP rather than HTTPS

• Component: Tools / Tests
• Status: RESOLVED DUPLICATE

[Alan Coon]

As of the moment, our tooling is configured to set up new WebKit repositories using http:// and git:// rather than https://. We should transition over into using https:// in webkit-patch setup-git-clone, as well as offer to migrate your repository to https:// while using update-webkit.

[Alan Coon]

<rdar://problem/54412280>

[Alan Coon]

Created attachment 377408 [details]
Patch

[Jonathan Bedard]

Comment on attachment 377408 [details]
Patch

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

We're baking this into update-webkit...I don't know how often contributors use that script. I know that I (for example) never have.

We should also probably bake this warning into webkit-patch (the script used to upload patches to bugzilla), which I think would be easiest to do by baking the warning into the the SCM classes in webkitpy. There might be good reasons to not do this, but it's certainly worth an investigation.

> Tools/Scripts/migrate-webkit-git-to-https:109
> +        command = 'df -lk {}'.format(path)

I feel like this command won't work on Windows.

Usually in our tooling, we do this sort of thing in webkitpy/common/system/platforminfo.py, although I don't actually see this this particular abstraction, nor am I convinced that this script needs a webkitpy dependency.

> Tools/Scripts/migrate-webkit-git-to-https:122
> +    if not is_git_repo(from_repo_path) or not is_git_repo(to_repo_path):

There is already a pretty extensive SCM abstraction in webkitpy/common/checkout/scm, that may be of use.

> Tools/Scripts/migrate-webkit-git-to-https:141
> +        command = 'git -C {} remote -v'.format(repo_path)

We usually represent commands as arrays, why the deviation here?

[Alan Coon]

Created attachment 377663 [details]
Patch

[Alan Coon]

Jonathan, to address your concerns I did the following:

1. I added a warning to the SCM __init__() that warns the user if his/her repository is HTTP. 

This means that webkit-patch is called, the SCM child being instantiated will call its class specific override of uses_secure_protocol() and print a warning if the remote URL is found not to be HTTPS or SSH (in the case of Git).

2. I modified the `df -lk` logic, and now use built-in Python logic to determine the available disk space. This should be platform agnostic.

3. I changed the command you noted to be in an array to eliminate the deviation.

[Jonathan Bedard]

Comment on attachment 377663 [details]
Patch

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

> Tools/Scripts/migrate-webkit-git-to-https:1
> +#!/usr/bin/env python -u

Do we intend this to be a temporary script?

> Tools/Scripts/update-webkit:61
> +  -s|--skip-warning-https   do not ask whether user would like to migrate to HTTPS, if applicable

I'd change to 'silence HTTPS migration prompt'

> Tools/Scripts/webkitdirs.pm:3015
> +            my $response = <STDIN>;

Have we audited automation to make sure nothing is using this script? I seem to recall that at some point, one of our buildbots used it...

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:513
> +        """

We usually do # comments...also, I don't know if we need anything other than 'Strategy copied from isGitZVNDirectory() in VCSutils.pm'

[Jonathan Bedard]

Comment on attachment 377663 [details]
Patch

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

> Tools/Scripts/migrate-webkit-git-to-https:109
> +        if sys.version_info >= (3, 3):

Why the version difference? Any reason to prefer shutil over os. In this case?

[Daniel Bates]

Comment on attachment 377663 [details]
Patch

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

Thank you for taking this on! This patch is a good first stab in my opinion. I expected the solution to entail touching update-webkit and I see that you did that, which is great. I  expected this because update-webkit is the officially documented way to update your checkout [1]. The migration process part of this patch is not the solution I expected because:

1. To update the **SVN** repo URL it does a full clone instead of just updating Git’s state. Quick search found <https://stackoverflow.com/questions/5975667/how-to-switch-svn-repositories-using-git-svn> and <https://git.wiki.kernel.org/index.php/GitSvnSwitch> (the former actually references the latter). Quick glance at those pages suggests the “rewriteRoot” method may be promising.  If neither of these pages provides a reliable solution that we can use then I expected the ChangeLog to explain why we cannot use these methods and must do a full clone.

2. I did not expect to see full cloning of the repo to update the **Git** remote. I expected to see use of “git remote” to do this or an explanation in the ChangeLog why we cannot use “git remote” or why it does not make sense to use it if a full clone is needed for (1).

3. It does not copy over Git config settings, local branches, etc etc.

4. It does not tell the user that it did not copy over Git config settings.

5. Because of (1) and (2) it requires enough free space to do a full clone and this is not a quick operation.

[1] <https://webkit.org/getting-the-code/>

> Tools/Scripts/webkitdirs.pm:3019
> +                    "https://svn.webkit.org/repository/webkit/trunk") == 0)

I did not expect to see “== 0”. I expected to see one of the following:

1. !system(...)

2. !exitStatus(system(...)) <— this is the more technically correct solution as system() returns a exit status (read: not an exit code). There is a difference between exit status and exit code and it comes down to the former encodes that latter plus whether the command exited due to a signal (i.e. SIGINT). Read the Perl documentation for <https://perldoc.perl.org/functions/system.html> and <https://perldoc.perl.org/perlvar.html#%24%3f> for more details. Anyway, exitStatus() knows how to decode the return value of system() and get you the exit code.

> Tools/Scripts/webkitdirs.pm:3027
> +            my $svnRemoteURL = `git config svn-remote.svn.url`;

I did not expect to see this. I expected to see a call to function in VCSUtils.pm that does this. If there is not such function then I expected that the solution would include adding such a function.

> Tools/Scripts/webkitdirs.pm:3032
> +            my $remoteOriginURL = `git config remote.origin.url`;

Ditto.

> Tools/Scripts/webkitdirs.pm:3039
> +            print("WARNING: your Git-based WebKit checkout appears to be using an insecure protocol, git:// or http://.\n");

I did not expect a warning message with the word “appears” in it because:

1. A warning message is scary to me.
2. Because of (1) I expect that the program is 100% certain that something is wrong and thus it is scaring me to do something.
3. The word “appears” makes it sounds like the program itself it unsure, which goes against (2).

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:507
> +    def is_git_svn_repository(self):

I did not expect to this see function in the solution. I expected that we already have a function like this. Did you look around?

[Jonathan Bedard]

(In reply to Daniel Bates from comment #8)
> Comment on attachment 377663 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377663&action=review
> 
> ...
>
> 1. To update the **SVN** repo URL it does a full clone instead of just
> updating Git’s state. Quick search found
> <https://stackoverflow.com/questions/5975667/how-to-switch-svn-repositories-
> using-git-svn> and <https://git.wiki.kernel.org/index.php/GitSvnSwitch> (the
> former actually references the latter). Quick glance at those pages suggests
> the “rewriteRoot” method may be promising.  If neither of these pages
> provides a reliable solution that we can use then I expected the ChangeLog
> to explain why we cannot use these methods and must do a full clone.

If there is a reasonable way to do this without a full-clone, that would be preferable. It's worth noting that when we needed to do something similar with the Safari repository, we thought a full clone was acceptable. In that case though, if I recall, we were moving from ssh to https, which seems like a bigger change than moving from http to https.

[Alan Coon]

(In reply to Jonathan Bedard from comment #6)
> Comment on attachment 377663 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377663&action=review
> 
> > Tools/Scripts/migrate-webkit-git-to-https:1
> > +#!/usr/bin/env python -u
> 
> Do we intend this to be a temporary script?

I think we do intend for this to be somewhat temporary, seeing as we hopefully may move away from GitSVN entirely at some point.

> > Tools/Scripts/update-webkit:61
> > +  -s|--skip-warning-https   do not ask whether user would like to migrate to HTTPS, if applicable
> 
> I'd change to 'silence HTTPS migration prompt'

Sounds good to me.

> > Tools/Scripts/webkitdirs.pm:3015
> > +            my $response = <STDIN>;
> 
> Have we audited automation to make sure nothing is using this script? I seem
> to recall that at some point, one of our buildbots used it...

Yes this is pretty crucial. I'll come talk to you about this in person

> > Tools/Scripts/webkitpy/common/checkout/scm/git.py:513
> > +        """
> 
> We usually do # comments...also, I don't know if we need anything other than
> 'Strategy copied from isGitZVNDirectory() in VCSutils.pm'

Sounds good.

[Alan Coon]

(In reply to Jonathan Bedard from comment #7)
> Comment on attachment 377663 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377663&action=review
> 
> > Tools/Scripts/migrate-webkit-git-to-https:109
> > +        if sys.version_info >= (3, 3):
> 
> Why the version difference? Any reason to prefer shutil over os. In this
> case?

Ah yes, I just left a comment there to describe what I'm about to say here, but shutil.disk_usage is only on Python 3.3 and is more accurate than the os.statvfs strategy, which I use for backwards compatibility for < 3.3.

[Alan Coon]

(In reply to Daniel Bates from comment #8)
> Comment on attachment 377663 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377663&action=review
> 
> Thank you for taking this on! This patch is a good first stab in my opinion.

Thanks for the review Dan

> I expected the solution to entail touching update-webkit and I see that you
> did that, which is great. I  expected this because update-webkit is the
> officially documented way to update your checkout [1]. The migration process
> part of this patch is not the solution I expected because:
> 
> 1. To update the **SVN** repo URL it does a full clone instead of just
> updating Git’s state. Quick search found
> <https://stackoverflow.com/questions/5975667/how-to-switch-svn-repositories-
> using-git-svn> and <https://git.wiki.kernel.org/index.php/GitSvnSwitch> (the
> former actually references the latter). Quick glance at those pages suggests
> the “rewriteRoot” method may be promising.  If neither of these pages
> provides a reliable solution that we can use then I expected the ChangeLog
> to explain why we cannot use these methods and must do a full clone.
> 
> 2. I did not expect to see full cloning of the repo to update the **Git**
> remote. I expected to see use of “git remote” to do this or an explanation
> in the ChangeLog why we cannot use “git remote” or why it does not make
> sense to use it if a full clone is needed for (1).
> 
> 3. It does not copy over Git config settings, local branches, etc etc.
> 
> 4. It does not tell the user that it did not copy over Git config settings.
> 
> 5. Because of (1) and (2) it requires enough free space to do a full clone
> and this is not a quick operation.
> 
> [1] <https://webkit.org/getting-the-code/>

The original idea was to preserve the original directory incase something goes wrong during the process. However I tried out one of the methods outlined in the GitSVN kernel.org page and it seems to be working well so I'll update with a patch using that strategy.

I believe this will also address concerns 2, 3, 4, 5, as we are no longer doing a new clone.


> > Tools/Scripts/webkitdirs.pm:3019
> > +                    "https://svn.webkit.org/repository/webkit/trunk") == 0)
> 
> I did not expect to see “== 0”. I expected to see one of the following:
> 
> 1. !system(...)
> 
> 2. !exitStatus(system(...)) <— this is the more technically correct solution
> as system() returns a exit status (read: not an exit code). There is a
> difference between exit status and exit code and it comes down to the former
> encodes that latter plus whether the command exited due to a signal (i.e.
> SIGINT). Read the Perl documentation for
> <https://perldoc.perl.org/functions/system.html> and
> <https://perldoc.perl.org/perlvar.html#%24%3f> for more details. Anyway,
> exitStatus() knows how to decode the return value of system() and get you
> the exit code.

exitStatus() is now used to detect failed system calls

> > Tools/Scripts/webkitdirs.pm:3027
> > +            my $svnRemoteURL = `git config svn-remote.svn.url`;
> 
> I did not expect to see this. I expected to see a call to function in
> VCSUtils.pm that does this. If there is not such function then I expected
> that the solution would include adding such a function.

Now calls VCSUtils::gitConfig() rather than `git config...`

> > Tools/Scripts/webkitdirs.pm:3032
> > +            my $remoteOriginURL = `git config remote.origin.url`;
> 
> Ditto.
> 
> > Tools/Scripts/webkitdirs.pm:3039
> > +            print("WARNING: your Git-based WebKit checkout appears to be using an insecure protocol, git:// or http://.\n");
> 
> I did not expect a warning message with the word “appears” in it because:
> 
> 1. A warning message is scary to me.
> 2. Because of (1) I expect that the program is 100% certain that something
> is wrong and thus it is scaring me to do something.
> 3. The word “appears” makes it sounds like the program itself it unsure,
> which goes against (2).

This will be changed in my new patch, I agree better language is necessary here.
 
> > Tools/Scripts/webkitpy/common/checkout/scm/git.py:507
> > +    def is_git_svn_repository(self):
> 
> I did not expect to this see function in the solution. I expected that we
> already have a function like this. Did you look around?

I found a is_git_svn_directory method in a Sublime Text plugin class that's located at Tools/CopyPermalink/Submlime\ Text/CopyWebKitPermalink/CopyWebKitPermalink.py. My course of thought was that since the directory does not have an __init__.py file, and it's buried deep in an unrelated part of the repository it may not make much sense to use it.

[Alexey Proskuryakov]

Is this still an issue?

[Alan Coon]

(In reply to Alexey Proskuryakov from comment #13)
> Is this still an issue?

No we can close this

[Alan Coon]

*** This bug has been marked as a duplicate of bug 201658 ***