Use current issue number for git cl patch

Use current issue number for git cl patch

This change adds the option to use the current issue number, if any,
when doing a git cl patch. Instead of doing git cl issue (copy the
number) git cl patch <number>, one can simply do git cl patch -i

BUG=

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

[Mircea Trofin, 2016-01-27 22:27:26]

Description was changed from

==========
Use current issue number for git cl patch

This change adds the option to use the current issue number, if any,
when doing a git cl patch. Instead of doing git cl issue (copy the
number) git cl patch <number>, one can simply do git cl patch -i

BUG=
==========

to

==========
Use current issue number for git cl patch

This change adds the option to use the current issue number, if any,
when doing a git cl patch. Instead of doing git cl issue (copy the
number) git cl patch <number>, one can simply do git cl patch -i

BUG=
==========

[Mircea Trofin, 2016-01-27 22:27:26]

mtrofin@chromium.org changed reviewers:
+ bradnelson@chromium.org

[Dirk Pranke, 2016-01-28 01:24:38]

dpranke@chromium.org changed reviewers:
+ dpranke@chromium.org

[Dirk Pranke, 2016-01-28 01:24:38]

I'm not sure I understand the purpose (intent) of this change.

If I understand it correctly, if you say 'git cl patch -i' then it will look for
the current issue number set in the git config (i.e.,
branch.$BRANCHNAME.rietveldissue) and download that patch, right?

If so, isn't it usually the case that the only way rietveldissue gets set to a
number is because either it was set by a prior `git cl patch` invocation or a
`git cl upload` invocation? And, if that's the case, won't that mean that you'd
usually be re-applying the patch to the branch (which should normally fail)?

The only time I can see wanting this is if you actually reset the contents of
the branch (i.e., `git reset --hard @{u}`) and then wanted to re-apply from
Rietveld. Is that the goal?

If so, I wonder if that's actually better captured via a different name, like
--reapply or something?

Or are there other uses I'm not seeing?

[Mircea Trofin, 2016-01-28 02:09:43]

On 2016/01/28 01:24:38, Dirk Pranke wrote:
> I'm not sure I understand the purpose (intent) of this change.
> 
> If I understand it correctly, if you say 'git cl patch -i' then it will look
for
> the current issue number set in the git config (i.e.,
> branch.$BRANCHNAME.rietveldissue) and download that patch, right?
> 
> If so, isn't it usually the case that the only way rietveldissue gets set to a
> number is because either it was set by a prior `git cl patch` invocation or a
> `git cl upload` invocation? And, if that's the case, won't that mean that
you'd
> usually be re-applying the patch to the branch (which should normally fail)?
> 
> The only time I can see wanting this is if you actually reset the contents of
> the branch (i.e., `git reset --hard @{u}`) and then wanted to re-apply from
> Rietveld. Is that the goal?
> 
> If so, I wonder if that's actually better captured via a different name, like
> --reapply or something?
> 
> Or are there other uses I'm not seeing?

Thanks for the feedback - the goal is, indeed, to help with the workflow
of resetting the branch and re-applying. This can easily be scripted or aliased,
but what was awkward was getting the current issue number fed back to 
git cl patch.

I think --reapply makes for a better name. 

Should we also include the git reset --hard part? My preference would be "not"
because 1) I may or may not want to also git pull, 2) the point above - it's
easily
scriptable, once we have this --reapply flag.

Thoughts?

[Dirk Pranke, 2016-01-28 02:32:35]

dpranke@chromium.org changed reviewers:
+ iannucci@chromium.org
- bradnelson@chromium.org

[Dirk Pranke, 2016-01-28 02:35:11]

I'd probably leave out the reset --hard part, or at least make it a separate
switch, for the reasons you give.

Copying agable@ and iannucci@ to solicit other opinions.

[iannucci, 2016-01-28 03:12:17]

On 2016/01/28 at 02:35:11, dpranke wrote:
> I'd probably leave out the reset --hard part, or at least make it a separate
switch, for the reasons you give.
> 
> Copying agable@ and iannucci@ to solicit other opinions.

That's a really cute gerrit you're trying to reinvent here :)

So IIUC, the workflow in question is something like:
  * have a computer A and B
  * work on issue X from computer A, upload to reitveld
  * on computer B apply issue X, work some more, upload to rietveld
  * on computer A, reset the branch for X to the content that's now on rietveld

Is that correct?

[Mircea Trofin, 2016-01-28 03:23:12]

On 2016/01/28 03:12:17, iannucci wrote:
> On 2016/01/28 at 02:35:11, dpranke wrote:
> > I'd probably leave out the reset --hard part, or at least make it a separate
> switch, for the reasons you give.
> > 
> > Copying agable@ and iannucci@ to solicit other opinions.
> 
> That's a really cute gerrit you're trying to reinvent here :)
> 
> So IIUC, the workflow in question is something like:
>   * have a computer A and B
>   * work on issue X from computer A, upload to reitveld
>   * on computer B apply issue X, work some more, upload to rietveld
>   * on computer A, reset the branch for X to the content that's now on
rietveld
> 
> Is that correct?

That's correct.

[Mircea Trofin, 2016-01-28 05:54:45]

Patchset #2 (id:20001) has been deleted

[Mircea Trofin, 2016-01-28 05:57:58]

Updated with the current feedback ("reapply" for the flag name)

[iannucci, 2016-01-29 00:54:48]

Hm. I'm not sure how this /can/ work without the reset, unless you always
reapply from the exact same machine (which defeats the purpose of it).

https://chromiumcodereview.appspot.com/1641903002/diff/40001/git_cl.py
File git_cl.py (right):

https://chromiumcodereview.appspot.com/1641903002/diff/40001/git_cl.py#newcod...
git_cl.py:2958: if len(args) == 1:
if len(args) < 0

I think is what you want. --reapply implies 'no extra arguments' right?

[Mircea Trofin, 2016-01-29 05:01:42]

On 2016/01/29 00:54:48, iannucci wrote:
> Hm. I'm not sure how this /can/ work without the reset, unless you always
> reapply from the exact same machine (which defeats the purpose of it).
> 
> https://chromiumcodereview.appspot.com/1641903002/diff/40001/git_cl.py
> File git_cl.py (right):
> 
>
https://chromiumcodereview.appspot.com/1641903002/diff/40001/git_cl.py#newcod...
> git_cl.py:2958: if len(args) == 1:
> if len(args) < 0
> 
> I think is what you want. --reapply implies 'no extra arguments' right?

This won't work without a reset, but see earlier discussion (w. Dirk Pranke).

[Mircea Trofin, 2016-01-29 05:01:49]

https://chromiumcodereview.appspot.com/1641903002/diff/40001/git_cl.py
File git_cl.py (right):

https://chromiumcodereview.appspot.com/1641903002/diff/40001/git_cl.py#newcod...
git_cl.py:2958: if len(args) == 1:
On 2016/01/29 00:54:48, iannucci wrote:
> if len(args) < 0
> 
> I think is what you want. --reapply implies 'no extra arguments' right?

I'm confused, can len(args) be negative?

[iannucci, 2016-01-29 18:59:52]

On 2016/01/29 at 05:01:49, mtrofin wrote:
> https://chromiumcodereview.appspot.com/1641903002/diff/40001/git_cl.py
> File git_cl.py (right):
> 
>
https://chromiumcodereview.appspot.com/1641903002/diff/40001/git_cl.py#newcod...
> git_cl.py:2958: if len(args) == 1:
> On 2016/01/29 00:54:48, iannucci wrote:
> > if len(args) < 0
> > 
> > I think is what you want. --reapply implies 'no extra arguments' right?
> 
> I'm confused, can len(args) be negative?

er... > 0

[iannucci, 2016-01-29 19:01:09]

On 2016/01/29 at 05:01:42, mtrofin wrote:
> On 2016/01/29 00:54:48, iannucci wrote:
> > Hm. I'm not sure how this /can/ work without the reset, unless you always
> > reapply from the exact same machine (which defeats the purpose of it).
> > 
> > https://chromiumcodereview.appspot.com/1641903002/diff/40001/git_cl.py
> > File git_cl.py (right):
> > 
> >
https://chromiumcodereview.appspot.com/1641903002/diff/40001/git_cl.py#newcod...
> > git_cl.py:2958: if len(args) == 1:
> > if len(args) < 0
> > 
> > I think is what you want. --reapply implies 'no extra arguments' right?
> 
> This won't work without a reset, but see earlier discussion (w. Dirk Pranke).

Right, I'm saying that --reapply should issue a warning "this will reset your
branch to the state of the CL in rietveld. Un-uploaded changes will be deleted.
Continue (y/N)?:" and actually do the reset. Otherwise it's just weird.

[Dirk Pranke, 2016-01-29 19:05:24]

On 2016/01/29 19:01:09, iannucci wrote:
> Right, I'm saying that --reapply should issue a warning "this will reset your
> branch to the state of the CL in rietveld. Un-uploaded changes will be
deleted.
> Continue (y/N)?:" and actually do the reset. Otherwise it's just weird.

Having thought about this a bit more as well, I'm inclined to agree w/
iannucci@,
partially.

I would have it always do the reset. I suggest maybe also supporting a --pull
flag so that you can update at the same time.

I would not, generally, be a fan of adding the warning prompt, but I could be
open to doing so if we checked and say that the local repo contained 
un-uploaded changes (or the repo was otherwise dirty).

But both of those things do make life slightly more complex. It's kind of a
question as to whether you see this more as a user-level command or a porcelain
command. I'd see it more as the former, since obviously the existing git-cl
patch
provides the underlying (porcelainish) functionality as needed.

[Mircea Trofin, 2016-01-29 19:10:45]

On 2016/01/29 19:05:24, Dirk Pranke wrote:
> On 2016/01/29 19:01:09, iannucci wrote:
> > Right, I'm saying that --reapply should issue a warning "this will reset
your
> > branch to the state of the CL in rietveld. Un-uploaded changes will be
> deleted.
> > Continue (y/N)?:" and actually do the reset. Otherwise it's just weird.
> 
> Having thought about this a bit more as well, I'm inclined to agree w/
> iannucci@,
> partially.
> 
> I would have it always do the reset. I suggest maybe also supporting a --pull
> flag so that you can update at the same time.
> 
> I would not, generally, be a fan of adding the warning prompt, but I could be
> open to doing so if we checked and say that the local repo contained 
> un-uploaded changes (or the repo was otherwise dirty).
> 
> But both of those things do make life slightly more complex. It's kind of a
> question as to whether you see this more as a user-level command or a
porcelain
> command. I'd see it more as the former, since obviously the existing git-cl
> patch
> provides the underlying (porcelainish) functionality as needed.

What's a porcelain command?

I like the --pull suggestion, and no prompts. Updated patch coming
shortly.

Thanks!

[Dirk Pranke, 2016-01-29 19:13:37]

On 2016/01/29 19:10:45, Mircea Trofin wrote:
> What's a porcelain command?

"porcelain" is git terminology, though looking at it now, I actually had
it backwards, and wanted "plumbing" instead :).

See https://git-scm.com/book/en/v2/Git-Internals-Plumbing-and-Porcelain

[iannucci, 2016-01-29 19:21:30]

Just before we get too far sideways... have you considered using the sandbox
refs? You can push to refs/experimental/${username}/whatever in the chromium/src
repo (and we can enable it for any other repo as well). That's what many folks
do to move changes between machines.

[Dirk Pranke, 2016-01-29 19:31:14]

On 2016/01/29 19:21:30, iannucci wrote:
> Just before we get too far sideways... have you considered using the sandbox
> refs? You can push to refs/experimental/${username}/whatever in the
chromium/src
> repo (and we can enable it for any other repo as well). That's what many folks
> do to move changes between machines.

I didn't realize we had that enabled in the chromium repo, that's good to know.

That said, I still think this functionality is a good idea. I at least do
something
like this (using rietveld as the staging area) all the time.

[Mircea Trofin, 2016-01-30 01:12:44]

Patchset #4 (id:80001) has been deleted

[Mircea Trofin, 2016-01-30 01:12:51]

Patchset #3 (id:60001) has been deleted

[iannucci, 2016-01-30 02:54:21]

lgtm fwiw. dpranke did you have additional comments?

https://codereview.chromium.org/1641903002/diff/100001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/1641903002/diff/100001/git_cl.py#newcode2956
git_cl.py:2956: help="Reset the branch and reapply the issue.")
I would add some warning text here: "CAUTION: This will completely reset changes
on your branch to the content contained in the rietveld issue" or something like
that.

https://codereview.chromium.org/1641903002/diff/100001/git_cl.py#newcode2968
git_cl.py:2968: parser.print_help()
maybe want to try

parser.error("--reapply implies 0 additional arguments"), which I think should
also print help and exit.

https://codereview.chromium.org/1641903002/diff/100001/git_cl.py#newcode2974
git_cl.py:2974: RunGit(['reset', '--hard', upstream])
you may need to check for None here. Some folks (still) don't track upstream
branches and just have them implicitly follow e.g. origin/master.

You can try this out locally with `git checkout -b some_branch` instead of using
`git new-branch`. I suspect this function will return None in such a case and
the RunGit line will pop an exception.

https://codereview.chromium.org/1641903002/diff/100001/git_cl.py#newcode2980
git_cl.py:2980: return 1
same here and below re: parser.error

[Dirk Pranke, 2016-02-01 23:19:19]

lgtm w/ iannucci's suggestions and an updated CL subject and description.

https://codereview.chromium.org/1641903002/diff/100001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/1641903002/diff/100001/git_cl.py#newcode2968
git_cl.py:2968: parser.print_help()
nit: s/if len(args) > 0:/if args:/

[Mircea Trofin, 2016-02-02 00:35:40]

The CQ bit was checked by mtrofin@chromium.org

[Mircea Trofin, 2016-02-02 00:35:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/1641903002/#ps120001
(title: " ")

[Mircea Trofin, 2016-02-02 00:35:49]

https://codereview.chromium.org/1641903002/diff/100001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/1641903002/diff/100001/git_cl.py#newcode2956
git_cl.py:2956: help="Reset the branch and reapply the issue.")
On 2016/01/30 02:54:20, iannucci wrote:
> I would add some warning text here: "CAUTION: This will completely reset
changes
> on your branch to the content contained in the rietveld issue" or something
like
> that.

Done.

https://codereview.chromium.org/1641903002/diff/100001/git_cl.py#newcode2968
git_cl.py:2968: parser.print_help()
On 2016/01/30 02:54:20, iannucci wrote:
> maybe want to try
> 
> parser.error("--reapply implies 0 additional arguments"), which I think should
> also print help and exit.

Done.

https://codereview.chromium.org/1641903002/diff/100001/git_cl.py#newcode2968
git_cl.py:2968: parser.print_help()
On 2016/02/01 23:19:19, Dirk Pranke wrote:
> nit: s/if len(args) > 0:/if args:/
> 

I saw len(args) elsewhere. For consistency, I'd prefer keeping it as such.

https://codereview.chromium.org/1641903002/diff/100001/git_cl.py#newcode2974
git_cl.py:2974: RunGit(['reset', '--hard', upstream])
On 2016/01/30 02:54:21, iannucci wrote:
> you may need to check for None here. Some folks (still) don't track upstream
> branches and just have them implicitly follow e.g. origin/master.
> 
> You can try this out locally with `git checkout -b some_branch` instead of
using
> `git new-branch`. I suspect this function will return None in such a case and
> the RunGit line will pop an exception.

Actually, that'll set the branch to master, but I added a test to be sure.

https://codereview.chromium.org/1641903002/diff/100001/git_cl.py#newcode2980
git_cl.py:2980: return 1
On 2016/01/30 02:54:21, iannucci wrote:
> same here and below re: parser.error

Done.

[commit-bot: I haz the power, 2016-02-02 00:36:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1641903002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1641903002/120001

[commit-bot: I haz the power, 2016-02-02 00:44:10]

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

[commit-bot: I haz the power, 2016-02-02 00:44:11]

Try jobs failed on following builders:
  depot_tools_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/depot_tools_pre...)

[Mircea Trofin, 2016-02-04 16:23:01]

The CQ bit was checked by mtrofin@chromium.org

[commit-bot: I haz the power, 2016-02-04 16:23:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1641903002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1641903002/120001

[commit-bot: I haz the power, 2016-02-04 16:25:13]

Description was changed from

==========
Use current issue number for git cl patch

This change adds the option to use the current issue number, if any,
when doing a git cl patch. Instead of doing git cl issue (copy the
number) git cl patch <number>, one can simply do git cl patch -i

BUG=
==========

to

==========
Use current issue number for git cl patch

This change adds the option to use the current issue number, if any,
when doing a git cl patch. Instead of doing git cl issue (copy the
number) git cl patch <number>, one can simply do git cl patch -i

BUG=

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

[commit-bot: I haz the power, 2016-02-04 16:25:14]

Committed patchset #4 (id:120001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=298582