git-cl: Save remote name in "Base URL" field.

git-cl: Save remote name in "Base URL" field.

Ted suggested this to me as something that would be pretty
useful for Chrome OS.  We have a bunch of repos, and it can
be tough to tell which one a particular code review is
modifying.

Bumps upload.py to r515.

[evan, 2010-02-23 23:04:20]

This doesn't really make sense for Chrome (why put "chrome" in every commit?). 
Why not use the "SVN base" field on the review instead?  (See the left column;
it's some param you can set when you upload I think.)

[evan, 2010-02-23 23:21:16]

If you ask nicely I'll write the Rietveld patches necessary to remove the "SVN"
from the "SVN Base:" part of the page.

[derat, 2010-02-24 00:40:11]

Thanks for the offer.  I'm having trouble finding a way to use the SVN
base field for this, though.  upload.py uses this code to determine
what it should pass as the 'base' parameter when submitting the form:

  if isinstance(vcs, SubversionVCS):
    # base field is only allowed for Subversion.
    # Note: Fetching base files may become deprecated in future releases.
    base = vcs.GuessBase(options.download_base)
  else:
    base = None

GuessBase() uses "svn info" to try to get a URL.  The form at
http://codereview.appspot.com/new rejects anything that's non-HTTP in
its 'base' parameter; I'm doubtful that I can store arbitrary text
there.

On Tue, Feb 23, 2010 at 3:21 PM,  <evan@chromium.org> wrote:
> If you ask nicely I'll write the Rietveld patches necessary to remove
> the "SVN" from the "SVN Base:" part of the page.
>
> http://codereview.appspot.com/218073/show
>

[msb_google.com, 2010-03-01 17:50:19]

On Tue, Feb 23, 2010 at 4:40 PM, Daniel Erat <derat@chromium.org> wrote:

> Thanks for the offer.  I'm having trouble finding a way to use the SVN
> base field for this, though.  upload.py uses this code to determine
> what it should pass as the 'base' parameter when submitting the form:
>
>  if isinstance(vcs, SubversionVCS):
>    # base field is only allowed for Subversion.
>    # Note: Fetching base files may become deprecated in future releases.
>    base = vcs.GuessBase(options.download_base)
>  else:
>    base = None
>
> GuessBase() uses "svn info" to try to get a URL.  The form at
> http://codereview.appspot.com/new rejects anything that's non-HTTP in
> its 'base' parameter; I'm doubtful that I can store arbitrary text
> there.
>
> On Tue, Feb 23, 2010 at 3:21 PM,  <evan@chromium.org> wrote:
> > If you ask nicely I'll write the Rietveld patches necessary to remove
> > the "SVN" from the "SVN Base:" part of the page.
> >
> > http://codereview.appspot.com/218073/show
> >
>

If its any help, all of our git repos are also available via http. We could
store the http url in codereview.settings if it helps.

[derat, 2010-03-23 17:57:49]

Another look, please.  I bumped upload.py to r515.  Tested by uploading changes
to the Chromium repo and Chrome OS's chromiumos-overlay.git repo.

http://codereview.appspot.com/218073/diff/2001/3001
File git-cl (right):

http://codereview.appspot.com/218073/diff/2001/3001#newcode644
git-cl:644: if remote_parts[0] in ('git', 'ssh'):
let me know if you think that:

  if remote_parts[0] not in ('http', 'https'):

would be better.  seems a bit safer

[derat, 2010-03-23 17:58:27]

Oh, and note that we can leak jail hostnames with this, but that doesn't seem
like a huge deal to me.

On 2010/03/23 17:57:49, derat wrote:
> Another look, please.  I bumped upload.py to r515.  Tested by uploading
changes
> to the Chromium repo and Chrome OS's chromiumos-overlay.git repo.
> 
> http://codereview.appspot.com/218073/diff/2001/3001
> File git-cl (right):
> 
> http://codereview.appspot.com/218073/diff/2001/3001#newcode644
> git-cl:644: if remote_parts[0] in ('git', 'ssh'):
> let me know if you think that:
> 
>   if remote_parts[0] not in ('http', 'https'):
> 
> would be better.  seems a bit safer

[msb, 2010-03-23 18:54:16]

On 2010/03/23 17:58:27, derat wrote:
> Oh, and note that we can leak jail hostnames with this, but that doesn't seem
> like a huge deal to me.
> 

That's. chromiumos-git is already known via the wiki.

> On 2010/03/23 17:57:49, derat wrote:
> > Another look, please.  I bumped upload.py to r515.  Tested by uploading
> changes
> > to the Chromium repo and Chrome OS's chromiumos-overlay.git repo.
> > 
> > http://codereview.appspot.com/218073/diff/2001/3001
> > File git-cl (right):
> > 
> > http://codereview.appspot.com/218073/diff/2001/3001#newcode644
> > git-cl:644: if remote_parts[0] in ('git', 'ssh'):
> > let me know if you think that:
> > 
> >   if remote_parts[0] not in ('http', 'https'):
> > 
> > would be better.  seems a bit safer

LGTM on the git-cl change. Hate to nitpick but could you send the bumping of
upload.py as a separate CL.

[evan, 2010-03-23 19:01:58]

http://codereview.appspot.com/218073/diff/2001/3001
File git-cl (right):

http://codereview.appspot.com/218073/diff/2001/3001#newcode640
git-cl:640: # at least help the reviewer see which repo is being changed.
I investigated this, BTW.  It is because it's plumbed through to a Python
urlparse object, which for reasons opaque to me has a hardcoded list of allowed
schemes (which even change between versions!)

http://docs.python.org/library/urlparse.html

The fix is to switch out the Rietveld backing object, which I can do.

[M-A, 2010-03-23 19:03:05]

On 2010/03/23 19:01:58, evan wrote:
> I investigated this, BTW.  It is because it's plumbed through to a Python
> urlparse object, which for reasons opaque to me has a hardcoded list of
allowed
> schemes (which even change between versions!)
> 
> http://docs.python.org/library/urlparse.html
> 
> The fix is to switch out the Rietveld backing object, which I can do.

I'd really prefer that.

[msb, 2010-03-23 19:13:31]

On Tue, Mar 23, 2010 at 12:03 PM, <maruel@chromium.org> wrote:

> On 2010/03/23 19:01:58, evan wrote:
>
>> I investigated this, BTW.  It is because it's plumbed through to a
>>
> Python
>
>> urlparse object, which for reasons opaque to me has a hardcoded list
>>
> of allowed
>
>> schemes (which even change between versions!)
>>
>
>  http://docs.python.org/library/urlparse.html
>>
>
>  The fix is to switch out the Rietveld backing object, which I can do.
>>
>
> I'd really prefer that.
>
>
So would I but wouldn't that be a server side change. How long would it be
before the server picks up such a change. In the meantime, we're really
hurting with so many chromiumos repos and not being able to tell which repo
a given review is for. Could we apply derat's change until the server is
upgraded and then revert it?


>
> http://codereview.appspot.com/218073/show
>

[derat, 2010-03-23 19:22:02]

On Tue, Mar 23, 2010 at 12:13 PM, Mandeep Singh Baines <msb@chromium.org> wrote:
>
>
> On Tue, Mar 23, 2010 at 12:03 PM, <maruel@chromium.org> wrote:
>>
>> On 2010/03/23 19:01:58, evan wrote:
>>>
>>> I investigated this, BTW.  It is because it's plumbed through to a
>>
>> Python
>>>
>>> urlparse object, which for reasons opaque to me has a hardcoded list
>>
>> of allowed
>>>
>>> schemes (which even change between versions!)
>>
>>> http://docs.python.org/library/urlparse.html
>>
>>> The fix is to switch out the Rietveld backing object, which I can do.
>>
>> I'd really prefer that.
>
> So would I but wouldn't that be a server side change. How long would it be
> before the server picks up such a change. In the meantime, we're really
> hurting with so many chromiumos repos and not being able to tell which repo
> a given review is for. Could we apply derat's change until the server is
> upgraded and then revert it?

I added a todo to remove the mangling after the server has been updated.

>> http://codereview.appspot.com/218073/show
>
>

[evan, 2010-03-23 19:31:49]

http://codereview.appspot.com/696041

[msb, 2010-03-25 00:34:18]

LGTM. Maybe add link to evan's CL in the TODO.

Rietveld is nice enough to print the current version in the bottom right corner:

This is Rietveld r516.

On 2010/03/23 19:22:02, derat wrote:
> On Tue, Mar 23, 2010 at 12:13 PM, Mandeep Singh Baines
<mailto:msb@chromium.org> wrote:
> >
> >
> > On Tue, Mar 23, 2010 at 12:03 PM, <mailto:maruel@chromium.org> wrote:
> >>
> >> On 2010/03/23 19:01:58, evan wrote:
> >>>
> >>> I investigated this, BTW. &nbsp;It is because it's plumbed through to a
> >>
> >> Python
> >>>
> >>> urlparse object, which for reasons opaque to me has a hardcoded list
> >>
> >> of allowed
> >>>
> >>> schemes (which even change between versions!)
> >>
> >>> http://docs.python.org/library/urlparse.html
> >>
> >>> The fix is to switch out the Rietveld backing object, which I can do.
> >>
> >> I'd really prefer that.
> >
> > So would I but wouldn't that be a server side change. How long would it be
> > before the server picks up such a change. In the meantime, we're really
> > hurting with so many chromiumos repos and not being able to tell which repo
> > a given review is for. Could we apply derat's change until the server is
> > upgraded and then revert it?
> 
> I added a todo to remove the mangling after the server has been updated.
> 
> >> http://codereview.appspot.com/218073/show
> >
> >
>

[evan, 2010-03-25 00:38:30]

FYI, I sent John the merge changelist to integrate my upstream Rietveld change
into the chromium branch.  So in theory we could have the new, baseurl-accepting
server live within a day.

[msb, 2010-03-25 03:25:40]

evan@chromium.org (evan@chromium.org) wrote:
> FYI, I sent John the merge changelist to integrate my upstream Rietveld
> change into the chromium branch.  So in theory we could have the new,
> baseurl-accepting server live within a day.
>

Cool. If that's the case, maybe we should hold off on this change.

Didn't realize it could get pushed so fast:)

> http://codereview.appspot.com/218073/show

[derat, 2010-03-29 21:39:32]

On 2010/03/25 00:38:30, evan wrote:
> FYI, I sent John the merge changelist to integrate my upstream Rietveld change
> into the chromium branch.  So in theory we could have the new,
baseurl-accepting
> server live within a day.

I've updated my git-cl change to not mangle the base URL in anticipation of the
Rietveld change going live (it looks like it hasn't yet: "Issue creation errors:
{'base': [u'Invalid URL']}").