Fix reitveld base URL for googlesource.com repos.

Fix reitveld base URL for googlesource.com repos.

The base url was previously being generated as
URL@BRANCH.  I'm not sure if this works anywhere
but it certainly doesn't on googlesource.com.
Here we want URL/+/BRANCH.

R=iannucci@chromium.org, sergeyberezin@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=292493

[Sam Clegg, 2014-10-14 22:44:12]

sbc@chromium.org changed reviewers:
+ iannucci@chromium.org

[Sam Clegg, 2014-10-14 22:45:49]

https://codereview.chromium.org/652193004/diff/20001/git_cl.py
File git_cl.py (left):

https://codereview.chromium.org/652193004/diff/20001/git_cl.py#oldcode1701
git_cl.py:1701: if cl.GetRemoteUrl() and '/' in cl.GetUpstreamBranch():
'/' seems to always be in the branch name even for local branches where it is
(refs/branches/foo).

https://codereview.chromium.org/652193004/diff/20001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/652193004/diff/20001/git_cl.py#newcode1707
git_cl.py:1707: remote_url = remote + '@' + branch.split('/')[-1]
Should we leave '@' as the default?  I guess it makes some sense even though it
doesn't work in the URL bar.  Is there some other meaning of '@' that I don't
know know about?

[Sam Clegg, 2014-10-14 22:46:36]

Would be nice if the url was clickable in reitveld too, but at least this fixes
the url in the emails that reitveld sends.

[iannucci, 2014-10-15 00:09:02]

iannucci@chromium.org changed reviewers:
+ sergeyberezin@chromium.org

[iannucci, 2014-10-15 00:09:02]

Sergey, do you know if the base url is used for anything any more (e.g. CQ?)

[Sergey Berezin, 2014-10-15 00:11:57]

On 2014/10/15 00:09:02, iannucci wrote:
> Sergey, do you know if the base url is used for anything any more (e.g. CQ?)

It's used for some SVN repos (like tools) to derive relpath. It's not used for
git repos.

[iannucci, 2014-10-15 00:13:44]

On 2014/10/15 00:11:57, Sergey Berezin wrote:
> On 2014/10/15 00:09:02, iannucci wrote:
> > Sergey, do you know if the base url is used for anything any more (e.g. CQ?)
> 
> It's used for some SVN repos (like tools) to derive relpath. It's not used for
> git repos.

My worry/wonder is that I think that many people use git-cl with SVN
source-of-truth repos... I'm wondering if this change would goof up the CQ in
those cases.

[Sam Clegg, 2014-10-15 00:20:44]

On 2014/10/15 00:13:44, iannucci wrote:
> On 2014/10/15 00:11:57, Sergey Berezin wrote:
> > On 2014/10/15 00:09:02, iannucci wrote:
> > > Sergey, do you know if the base url is used for anything any more (e.g.
CQ?)
> > 
> > It's used for some SVN repos (like tools) to derive relpath. It's not used
for
> > git repos.
> 
> My worry/wonder is that I think that many people use git-cl with SVN
> source-of-truth repos... I'm wondering if this change would goof up the CQ in
> those cases.

It would only effect people with svn as the source of truth but who have not
configured git-svn, right?
Does the '@' sign mean something in then svn world?  I can't see how @master at
the end of a git url could ever be useful but no nothing of the CQ.

Is CQ used by many projects outside of nacl and chromium?  (I can verify that
this doesn't break nacl CQ, which is an svn source of truth).

[Sam Clegg, 2014-10-15 00:21:28]

On 2014/10/15 00:20:44, Sam Clegg wrote:
> On 2014/10/15 00:13:44, iannucci wrote:
> > On 2014/10/15 00:11:57, Sergey Berezin wrote:
> > > On 2014/10/15 00:09:02, iannucci wrote:
> > > > Sergey, do you know if the base url is used for anything any more (e.g.
> CQ?)
> > > 
> > > It's used for some SVN repos (like tools) to derive relpath. It's not used
> for
> > > git repos.
> > 
> > My worry/wonder is that I think that many people use git-cl with SVN
> > source-of-truth repos... I'm wondering if this change would goof up the CQ
in
> > those cases.
> 
> It would only effect people with svn as the source of truth but who have not
> configured git-svn, right?
> Does the '@' sign mean something in then svn world?  I can't see how @master
at
> the end of a git url could ever be useful but no nothing of the CQ.

/but no/but I know/

> 
> Is CQ used by many projects outside of nacl and chromium?  (I can verify that
> this doesn't break nacl CQ, which is an svn source of truth).

[iannucci, 2014-10-15 20:41:44]

per offline discussion, lgtm

[Sergey Berezin, 2014-10-15 20:45:42]

lgtm

[Sam Clegg, 2014-10-15 22:01:28]

The CQ bit was checked by sbc@chromium.org

[commit-bot: I haz the power, 2014-10-15 22:02:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/652193004/20001

[commit-bot: I haz the power, 2014-10-15 22:02:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/652193004/20001

[commit-bot: I haz the power, 2014-10-15 22:02:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-15 22:02:58]

Failed to apply patch for git_cl.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: git_cl.py
  |diff --git a/git_cl.py b/git_cl.py
  |index
4c5f9634c36a91d8c1690e00460286294aceec5d..d920bce889ba31c71fe426b9e639cc00ada42597
100755
  |--- a/git_cl.py
  |+++ b/git_cl.py
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       git_cl.py
Index: git_cl.py
diff --git a/git_cl.py b/git_cl.py
index
4c5f9634c36a91d8c1690e00460286294aceec5d..d920bce889ba31c71fe426b9e639cc00ada42597
100755
--- a/git_cl.py
+++ b/git_cl.py
@@ -1698,9 +1698,13 @@ def RietveldUpload(options, args, cl, change):
                     if ': ' in line)
         remote_url = keys.get('URL', None)
     else:
-      if cl.GetRemoteUrl() and '/' in cl.GetUpstreamBranch():
-        remote_url = (cl.GetRemoteUrl() + '@'
-                      + cl.GetUpstreamBranch().split('/')[-1])
+      remote = cl.GetRemoteUrl()
+      branch = cl.GetUpstreamBranch()
+      if remote and branch:
+        if 'googlesource' in remote:
+          remote_url = remote + '/+/' + branch.split('/')[-1]
+        else:
+          remote_url = remote + '@' + branch.split('/')[-1]
   if remote_url:
     upload_args.extend(['--base_url', remote_url])

[Sam Clegg, 2014-10-15 23:25:58]

On 2014/10/15 22:02:58, I haz the power (commit-bot) wrote:
> Failed to apply patch for git_cl.py:
> While running patch -p1 --forward --force --no-backup-if-mismatch;
>   can't find file to patch at input line 6
>   Perhaps you used the wrong -p or --strip option?
>   The text leading up to this was:
>   --------------------------
>   |Index: git_cl.py
>   |diff --git a/git_cl.py b/git_cl.py
>   |index
>
4c5f9634c36a91d8c1690e00460286294aceec5d..d920bce889ba31c71fe426b9e639cc00ada42597
> 100755
>   |--- a/git_cl.py
>   |+++ b/git_cl.py
>   --------------------------
>   No file to patch.  Skipping patch.
>   1 out of 1 hunk ignored
> 
> Patch:       git_cl.py
> Index: git_cl.py
> diff --git a/git_cl.py b/git_cl.py
> index
>
4c5f9634c36a91d8c1690e00460286294aceec5d..d920bce889ba31c71fe426b9e639cc00ada42597
> 100755
> --- a/git_cl.py
> +++ b/git_cl.py
> @@ -1698,9 +1698,13 @@ def RietveldUpload(options, args, cl, change):
>                      if ': ' in line)
>          remote_url = keys.get('URL', None)
>      else:
> -      if cl.GetRemoteUrl() and '/' in cl.GetUpstreamBranch():
> -        remote_url = (cl.GetRemoteUrl() + '@'
> -                      + cl.GetUpstreamBranch().split('/')[-1])
> +      remote = cl.GetRemoteUrl()
> +      branch = cl.GetUpstreamBranch()
> +      if remote and branch:
> +        if 'googlesource' in remote:
> +          remote_url = remote + '/+/' + branch.split('/')[-1]
> +        else:
> +          remote_url = remote + '@' + branch.split('/')[-1]
>    if remote_url:
>      upload_args.extend(['--base_url', remote_url])

I'm trying to land this with dcommit but I'm getting a failing presubmit:

./submodule-merge-test.sh 
Setting up test SVN repo...
Setting up test remote git-svn-submodule repo...
error: pathspec 'refs/remotes/trunk' did not match any file(s) known to git.

This happens with and without my change.  Is this a known issue?

[iannucci, 2014-10-16 20:49:48]

Sounds unrelated to this change, but that's the default git-svn remote
(e.g. if you don't use fetch or git auto-svn to set up git-svn).

On Wed, Oct 15, 2014 at 4:25 PM, <sbc@chromium.org> wrote:

> On 2014/10/15 22:02:58, I haz the power (commit-bot) wrote:
>
>> Failed to apply patch for git_cl.py:
>> While running patch -p1 --forward --force --no-backup-if-mismatch;
>>    can't find file to patch at input line 6
>>    Perhaps you used the wrong -p or --strip option?
>>    The text leading up to this was:
>>    --------------------------
>>    |Index: git_cl.py
>>    |diff --git a/git_cl.py b/git_cl.py
>>    |index
>>
>
> 4c5f9634c36a91d8c1690e00460286294aceec5d..d920bce889ba31c71fe426b9e639cc
> 00ada42597
>
>> 100755
>>    |--- a/git_cl.py
>>    |+++ b/git_cl.py
>>    --------------------------
>>    No file to patch.  Skipping patch.
>>    1 out of 1 hunk ignored
>>
>
>  Patch:       git_cl.py
>> Index: git_cl.py
>> diff --git a/git_cl.py b/git_cl.py
>> index
>>
>
> 4c5f9634c36a91d8c1690e00460286294aceec5d..d920bce889ba31c71fe426b9e639cc
> 00ada42597
>
>> 100755
>> --- a/git_cl.py
>> +++ b/git_cl.py
>> @@ -1698,9 +1698,13 @@ def RietveldUpload(options, args, cl, change):
>>                       if ': ' in line)
>>           remote_url = keys.get('URL', None)
>>       else:
>> -      if cl.GetRemoteUrl() and '/' in cl.GetUpstreamBranch():
>> -        remote_url = (cl.GetRemoteUrl() + '@'
>> -                      + cl.GetUpstreamBranch().split('/')[-1])
>> +      remote = cl.GetRemoteUrl()
>> +      branch = cl.GetUpstreamBranch()
>> +      if remote and branch:
>> +        if 'googlesource' in remote:
>> +          remote_url = remote + '/+/' + branch.split('/')[-1]
>> +        else:
>> +          remote_url = remote + '@' + branch.split('/')[-1]
>>     if remote_url:
>>       upload_args.extend(['--base_url', remote_url])
>>
>
> I'm trying to land this with dcommit but I'm getting a failing presubmit:
>
> ./submodule-merge-test.sh
> Setting up test SVN repo...
> Setting up test remote git-svn-submodule repo...
> error: pathspec 'refs/remotes/trunk' did not match any file(s) known to
> git.
>
> This happens with and without my change.  Is this a known issue?
>
> https://codereview.chromium.org/652193004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Sam Clegg, 2014-10-16 21:01:29]

Committed patchset #2 (id:20001) manually as 292493.

[jam, 2014-10-17 18:26:16]

On 2014/10/16 21:01:29, Sam Clegg wrote:
> Committed patchset #2 (id:20001) manually as 292493.

hi I think this may have broke commits to tools/build, see my email to troopers.

[jam, 2014-10-17 18:32:04]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/665653002/ by jam@chromium.org.

The reason for reverting is: I've verified that by updating depot_tools to be
before this cl, things work again.

See the base urls and CQ landing https://codereview.chromium.org/645033006
successfully..

[jam, 2014-10-17 18:43:59]

On 2014/10/17 18:32:04, jam wrote:
> A revert of this CL (patchset #2 id:20001) has been created in
> https://codereview.chromium.org/665653002/ by mailto:jam@chromium.org.
> 
> The reason for reverting is: I've verified that by updating depot_tools to be
> before this cl, things work again.
> 
> See the base urls and CQ landing https://codereview.chromium.org/645033006
> successfully..

even the revert failed through the CQ because of this, so I did it manually
using svn (committed in r292513)