Upload project option

Upload project option in 'gcl' and 'git cl' uploading commands.



BUG=374398
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=276491

[chromium-reviews, 2014-05-23 16:12:34]

Ping...

Best regards,

Sheng


On Tue, May 20, 2014 at 4:14 PM, <sheyang@chromium.org> wrote:

> Reviewers: agable, iannucci,
>
> Description:
> Upload project option
>
>
>
> BUG=374398
>
> Please review this at https://codereview.chromium.org/298703003/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/tools/depot_tools
>
> Affected files (+22, -0 lines):
>   M gcl.py
>   M git_cl.py
>   M tests/gcl_unittest.py
>   M tests/git_cl_test.py
>   M third_party/upload.py
>
>
> Index: gcl.py
> diff --git a/gcl.py b/gcl.py
> index cf48e782ee616b8b47d04ceb84f3f26b1f8c1505..
> 775ab94dd6ce9d346749bfa987c3a00be6b99b83 100755
> --- a/gcl.py
> +++ b/gcl.py
> @@ -911,6 +911,10 @@ def CMDupload(change_info, args):
>        if GetCodeReviewSetting("PRIVATE") == "True":
>          upload_arg.append("--private")
>
> +      project = GetCodeReviewSetting("PROJECT")
> +      if project:
> +        upload_arg.append("--project=%s" % project)
> +
>      # If we have a lot of files with long paths, then we won't be able to
> fit
>      # the command to "svn diff".  Instead, we generate the diff manually
> for
>      # each file and concatenate them before passing it to upload.py.
> Index: git_cl.py
> diff --git a/git_cl.py b/git_cl.py
> index e8695e82eff5e402b5d7ee09ecaa23216ea75c6b..
> 8690dabf73b062510626c50a20aa10119482dba2 100755
> --- a/git_cl.py
> +++ b/git_cl.py
> @@ -1074,6 +1074,12 @@ def LoadCodereviewSettingsFromFile(fileobj):
>      RunGit(['config', keyvals['PUSH_URL_CONFIG'],
>              keyvals['ORIGIN_URL_CONFIG']])
>
> +  if 'PROJECT' in keyvals:
> +    branchref = RunGit(['symbolic-ref', 'HEAD']).strip()
> +    branch = ShortBranchName(branchref)
> +    RunGit(['config', 'branch.%s.project' % branch, keyvals['PROJECT']],
> +           error_ok=False)
> +
>
>  def urlretrieve(source, destination):
>    """urllib is broken for SSL connections via a proxy therefore we
> @@ -1612,6 +1618,11 @@ def RietveldUpload(options, args, cl):
>    if remote_url:
>      upload_args.extend(['--base_url', remote_url])
>
> +  project = RunGit(['config', 'branch.%s.project' % cl.GetBranch()],
> +                   error_ok=True).strip()
> +  if project:
> +    upload_args.extend(['--project', project])
> +
>    try:
>      upload_args = ['upload'] + upload_args + args
>      logging.info('upload.RealMain(%s)', upload_args)
> Index: tests/gcl_unittest.py
> diff --git a/tests/gcl_unittest.py b/tests/gcl_unittest.py
> index e96fed0f7790855ed244f17345daed7fb620cb69..
> b3dbf53f2516c53d095cd97543982206d519ab5a 100755
> --- a/tests/gcl_unittest.py
> +++ b/tests/gcl_unittest.py
> @@ -367,6 +367,7 @@ class CMDuploadUnittest(GclTestsBase):
>      gcl.os.close(42)
>      gcl.GetCodeReviewSetting('CC_LIST')
>      gcl.GetCodeReviewSetting('PRIVATE')
> +    gcl.GetCodeReviewSetting('PROJECT')
>      gcl.os.getcwd().AndReturn('somewhere')
>      gcl.os.chdir(change_info.GetLocalRoot())
>      gcl.GenerateDiff(change_info.GetFileNames())
> @@ -409,6 +410,7 @@ class CMDuploadUnittest(GclTestsBase):
>      gcl.os.close(42)
>      gcl.GetCodeReviewSetting('CC_LIST')
>      gcl.GetCodeReviewSetting('PRIVATE')
> +    gcl.GetCodeReviewSetting('PROJECT')
>      gcl.os.getcwd().AndReturn('somewhere')
>      gcl.os.chdir(change_info.GetLocalRoot())
>      gcl.GenerateDiff(change_info.GetFileNames())
> Index: tests/git_cl_test.py
> diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py
> index 694fd80278af8459465d35f007b3a1b0a5dbdfe1..
> 951dde59fbdca6404668fc04edc6717e45323024 100755
> --- a/tests/git_cl_test.py
> +++ b/tests/git_cl_test.py
> @@ -210,6 +210,7 @@ class TestGitCl(TestCase):
>           (('', None), 0)),
>          ((['git', 'rev-parse', '--show-cdup'],), ''),
>          ((['git', 'svn', 'info'],), ''),
> +        ((['git', 'config', 'branch.master.project'],), ''),
>          ((['git',
>             'config', 'branch.master.rietveldissue', '1'],), ''),
>          ((['git', 'config', 'branch.master.rietveldserver',
> Index: third_party/upload.py
> diff --git a/third_party/upload.py b/third_party/upload.py
> index 908edc476a4b9e70886f5c5b81547c3c62502f20..
> 74b97b6a0758f227f466b5e0080c44cef54a3988 100755
> --- a/third_party/upload.py
> +++ b/third_party/upload.py
> @@ -629,6 +629,8 @@ group.add_option("-j", "--number-parallel-uploads",
>  group = parser.add_option_group("Issue options")
>  group.add_option("-t", "--title", action="store", dest="title",
>                   help="New issue subject or new patch set title")
> +group.add_option("--project", action="store", dest="project",
> +                 help="The project the issue belongs to")
>  group.add_option("-m", "--message", action="store", dest="message",
>                   default=None,
>                   help="New issue description or new patch set message")
> @@ -2596,6 +2598,8 @@ def RealMain(argv, data=None):
>      for cc in options.cc.split(','):
>        CheckReviewer(cc)
>      form_fields.append(("cc", options.cc))
> +  if options.project:
> +    form_fields.append(("project", options.project))
>
>    # Process --message, --title and --file.
>    message = options.message or ""
>
>
>

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

[iannucci, 2014-05-27 18:00:33]

You should make the CL description more descriptive, too :)

https://codereview.chromium.org/298703003/diff/1/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/298703003/diff/1/git_cl.py#newcode1078
git_cl.py:1078: branchref = RunGit(['symbolic-ref', 'HEAD']).strip()
What happens when the user has a detached HEAD?

https://codereview.chromium.org/298703003/diff/1/git_cl.py#newcode1079
git_cl.py:1079: branch = ShortBranchName(branchref)
what does shortbranchname do? Is this the same as `git symbolic-ref --short
HEAD`?

https://codereview.chromium.org/298703003/diff/1/third_party/upload.py
File third_party/upload.py (right):

https://codereview.chromium.org/298703003/diff/1/third_party/upload.py#newcod...
third_party/upload.py:633: help="The project the issue belongs to")
This file is a copy of
https://code.google.com/p/rietveld/source/browse/upload.py?name=chromium , which
is merged from https://code.google.com/p/rietveld/source/browse/upload.py .
You'll need to land these changes there first.

Get the Rietveld source code with mercurial, create the patch against the
default branch, then upload it by running ./upload.py (from the Rietveld
checkout). It will upload a review to codereview.appspot.com.

[agable, 2014-05-30 14:24:06]

https://codereview.chromium.org/298703003/diff/1/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/298703003/diff/1/git_cl.py#newcode1080
git_cl.py:1080: RunGit(['config', 'branch.%s.project' % branch,
keyvals['PROJECT']],
a) Why are you saving the PROJECT value in .git/config? (I see the other values
get stored there, but I don't understand why, since the codereview.settings file
is going to get read every time no matter what anyway).
b) Why are you saving PROJECT on a per-branch basis? The codereview.settings
file should change on a per-branch basis in general, given chromium's branching
model.

[sheyang, 2014-06-02 22:56:47]

https://codereview.chromium.org/298703003/diff/1/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/298703003/diff/1/git_cl.py#newcode1078
git_cl.py:1078: branchref = RunGit(['symbolic-ref', 'HEAD']).strip()
On 2014/05/27 18:00:33, iannucci wrote:
> What happens when the user has a detached HEAD?

Discussed offline. This will be saved into .git/config as other settings.

https://codereview.chromium.org/298703003/diff/1/git_cl.py#newcode1079
git_cl.py:1079: branch = ShortBranchName(branchref)
On 2014/05/27 18:00:33, iannucci wrote:
> what does shortbranchname do? Is this the same as `git symbolic-ref --short
> HEAD`?

It converts the name like 'refs/heads/foo' to 'foo'. Not sure what it's for, and
probably will change the behavior to show the full name, but for now just use it
as everywhere else.

https://codereview.chromium.org/298703003/diff/1/git_cl.py#newcode1080
git_cl.py:1080: RunGit(['config', 'branch.%s.project' % branch,
keyvals['PROJECT']],
On 2014/05/30 14:24:07, agable wrote:
> a) Why are you saving the PROJECT value in .git/config? (I see the other
values
> get stored there, but I don't understand why, since the codereview.settings
file
> is going to get read every time no matter what anyway).
> b) Why are you saving PROJECT on a per-branch basis? The codereview.settings
> file should change on a per-branch basis in general, given chromium's
branching
> model.
1. Yep it's not ideal... Just to make it consistent with any other settings for
now
2. Fixed

https://codereview.chromium.org/298703003/diff/1/third_party/upload.py
File third_party/upload.py (right):

https://codereview.chromium.org/298703003/diff/1/third_party/upload.py#newcod...
third_party/upload.py:633: help="The project the issue belongs to")
On 2014/05/27 18:00:33, iannucci wrote:
> This file is a copy of
> https://code.google.com/p/rietveld/source/browse/upload.py?name=chromium ,
which
> is merged from https://code.google.com/p/rietveld/source/browse/upload.py .
> You'll need to land these changes there first.
> 
> Get the Rietveld source code with mercurial, create the patch against the
> default branch, then upload it by running ./upload.py (from the Rietveld
> checkout). It will upload a review to http://codereview.appspot.com.

Upload a separate CL https://codereview.appspot.com/102740044/

[agable, 2014-06-03 17:41:12]

https://codereview.chromium.org/298703003/diff/40001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/298703003/diff/40001/git_cl.py#newcode448
git_cl.py:448: self.project = self._GetRietveldConfig('project', error_ok=True)
There is now nothing that actually sets this gitconfig property. After the
Rietveld-side upload.py CL lands, you're going to have to pull that change back
into this CL before landing.

https://codereview.chromium.org/298703003/diff/40001/git_cl.py#newcode1087
git_cl.py:1087: 
Leave two newlines here.

[sheyang, 2014-06-04 01:52:52]

https://codereview.chromium.org/298703003/diff/40001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/298703003/diff/40001/git_cl.py#newcode448
git_cl.py:448: self.project = self._GetRietveldConfig('project', error_ok=True)
On 2014/06/03 17:41:12, agable wrote:
> There is now nothing that actually sets this gitconfig property. After the
> Rietveld-side upload.py CL lands, you're going to have to pull that change
back
> into this CL before landing.

Done.

https://codereview.chromium.org/298703003/diff/40001/git_cl.py#newcode1087
git_cl.py:1087: 
On 2014/06/03 17:41:12, agable wrote:
> Leave two newlines here.

Done.

[chromium-reviews, 2014-06-09 22:26:23]

Ping...

Best regards,

Sheng


On Tue, Jun 3, 2014 at 6:52 PM, <sheyang@chromium.org> wrote:

>
> https://codereview.chromium.org/298703003/diff/40001/git_cl.py
> File git_cl.py (right):
>
> https://codereview.chromium.org/298703003/diff/40001/git_cl.py#newcode448
> git_cl.py:448: self.project = self._GetRietveldConfig('project',
> error_ok=True)
> On 2014/06/03 17:41:12, agable wrote:
>
>> There is now nothing that actually sets this gitconfig property. After
>>
> the
>
>> Rietveld-side upload.py CL lands, you're going to have to pull that
>>
> change back
>
>> into this CL before landing.
>>
>
> Done.
>
> https://codereview.chromium.org/298703003/diff/40001/git_cl.py#newcode1087
> git_cl.py:1087:
> On 2014/06/03 17:41:12, agable wrote:
>
>> Leave two newlines here.
>>
>
> Done.
>
> https://codereview.chromium.org/298703003/
>

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

[agable, 2014-06-11 21:07:57]

On 2014/06/09 22:26:23, chromium-reviews_chromium.org wrote:
> Ping...
> 
> Best regards,
> 
> Sheng
> 
> 
> On Tue, Jun 3, 2014 at 6:52 PM, <mailto:sheyang@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/298703003/diff/40001/git_cl.py
> > File git_cl.py (right):
> >
> > https://codereview.chromium.org/298703003/diff/40001/git_cl.py#newcode448
> > git_cl.py:448: self.project = self._GetRietveldConfig('project',
> > error_ok=True)
> > On 2014/06/03 17:41:12, agable wrote:
> >
> >> There is now nothing that actually sets this gitconfig property. After
> >>
> > the
> >
> >> Rietveld-side upload.py CL lands, you're going to have to pull that
> >>
> > change back
> >
> >> into this CL before landing.
> >>
> >
> > Done.
> >
> > https://codereview.chromium.org/298703003/diff/40001/git_cl.py#newcode1087
> > git_cl.py:1087:
> > On 2014/06/03 17:41:12, agable wrote:
> >
> >> Leave two newlines here.
> >>
> >
> > Done.
> >
> > https://codereview.chromium.org/298703003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

This is much cleaner, thanks. LGTM

[sheyang, 2014-06-11 21:33:11]

The CQ bit was checked by sheyang@chromium.org

[commit-bot: I haz the power, 2014-06-11 21:34:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sheyang@chromium.org/298703003/60001

[commit-bot: I haz the power, 2014-06-11 21:38:00]

Change committed as 276491