Add git cherry picking extension

Add git cherry picking extension

This extension uploads a fake cherry pick-style diff to rietveld with a modified project parameter. The modified project is intended to be used by the commit queue to attempt to land the change on a branch.

This works by grabbing the parent of the targeted revision and generating the diff. It is intended to be used to CQ trivial cherry picks which apply cleanly on top of other branches without conflicts.

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

[smut, 2014-07-15 21:57:44]

A cherry picking prototype. Sample cherry picked CL:
https://chromereviews.googleplex.com/63797016.

The goal here is to upload a CL to rietveld based on git hash, which can then be
sent to the CQ. The "based on git hash" part is a design requirement.

Opening for discussion.

[ghost stip (do not use), 2014-07-15 22:21:51]

seems like this should just be part of git cl upload, right? I guess that tool
explicitly depends on the notion of a 'branch' though

[smut, 2014-07-15 22:29:29]

On 2014/07/15 22:21:51, stip wrote:
> seems like this should just be part of git cl upload, right? I guess that tool
> explicitly depends on the notion of a 'branch' though

I thought so originally, but the "git cl" family of commands makes associations
with a specific issue number on rietveld. The idea behind this tool is that you
don't need a separate local branch and a CL associated with it. At any time, in
the middle of any of your work, you can run this tool to upload a cherry pick
CL. So, for example, it's intentional that this tool never records the issue
number, because it's not intended that the branch you run this from is
associated with the CL (this also implies you cannot upload another patchset on
top of a cherry pick CL generated by this tool-- which is again, intentional).

[agable, 2014-07-15 22:47:20]

https://chromiumcodereview.appspot.com/397593004/diff/30001/git_asdf.py
File git_asdf.py (right):

https://chromiumcodereview.appspot.com/397593004/diff/30001/git_asdf.py#newco...
git_asdf.py:24: author = run('config', 'user.email')
use config() from git_common, rather than run('config', ...).

https://chromiumcodereview.appspot.com/397593004/diff/30001/git_asdf.py#newco...
git_asdf.py:26: description = '%s\n\n(cherry picked from commit %s)\n' % (
I'd lose the parentheses -- match the format created by a normal git cherry-pick
as best you can.

https://chromiumcodereview.appspot.com/397593004/diff/30001/git_asdf.py#newco...
git_asdf.py:27: run('show', '--pretty=%B', '--quiet', commit), commit)
Indent at +4.

https://chromiumcodereview.appspot.com/397593004/diff/30001/git_asdf.py#newco...
git_asdf.py:37: ('base', '%s@%s' % (Changelist().GetRemoteUrl(),
target_branch)),
Indent +4.

https://chromiumcodereview.appspot.com/397593004/diff/30001/git_asdf.py#newco...
git_asdf.py:37: ('base', '%s@%s' % (Changelist().GetRemoteUrl(),
target_branch)),
Unfortunately, uploading url@branch for the issue base doesn't have any effect
on how the CQ handles the change. This is because Rietveld and the CQ still have
a lot of SVN-influenced assumptions built in, and that means they disregard
branches since SVN branches suck.

https://chromiumcodereview.appspot.com/397593004/diff/30001/git_asdf.py#newco...
git_asdf.py:48: '/upload', payload=payload, content_type=content_type)
Indent +4.

https://chromiumcodereview.appspot.com/397593004/diff/30001/git_asdf.py#newco...
git_asdf.py:54: '--branch',
Indent +4.

[smut, 2014-07-16 17:34:35]

https://chromiumcodereview.appspot.com/397593004/diff/30001/git_asdf.py
File git_asdf.py (right):

https://chromiumcodereview.appspot.com/397593004/diff/30001/git_asdf.py#newco...
git_asdf.py:24: author = run('config', 'user.email')
On 2014/07/15 22:47:20, agable wrote:
> use config() from git_common, rather than run('config', ...).

Done.

https://chromiumcodereview.appspot.com/397593004/diff/30001/git_asdf.py#newco...
git_asdf.py:26: description = '%s\n\n(cherry picked from commit %s)\n' % (
On 2014/07/15 22:47:20, agable wrote:
> I'd lose the parentheses -- match the format created by a normal git
cherry-pick
> as best you can.

This is the format I get when doing a git cherry-pick -x.

https://chromiumcodereview.appspot.com/397593004/diff/30001/git_asdf.py#newco...
git_asdf.py:27: run('show', '--pretty=%B', '--quiet', commit), commit)
On 2014/07/15 22:47:20, agable wrote:
> Indent at +4.

Done.

https://chromiumcodereview.appspot.com/397593004/diff/30001/git_asdf.py#newco...
git_asdf.py:37: ('base', '%s@%s' % (Changelist().GetRemoteUrl(),
target_branch)),
On 2014/07/15 22:47:20, agable wrote:
> Indent +4.

Done.

https://chromiumcodereview.appspot.com/397593004/diff/30001/git_asdf.py#newco...
git_asdf.py:37: ('base', '%s@%s' % (Changelist().GetRemoteUrl(),
target_branch)),
On 2014/07/15 22:47:20, agable wrote:
> Unfortunately, uploading url@branch for the issue base doesn't have any effect
> on how the CQ handles the change. This is because Rietveld and the CQ still
have
> a lot of SVN-influenced assumptions built in, and that means they disregard
> branches since SVN branches suck.

Ah, ok. Then I'll go with your project suggestion.

https://chromiumcodereview.appspot.com/397593004/diff/30001/git_asdf.py#newco...
git_asdf.py:48: '/upload', payload=payload, content_type=content_type)
On 2014/07/15 22:47:20, agable wrote:
> Indent +4.

Done.

https://chromiumcodereview.appspot.com/397593004/diff/30001/git_asdf.py#newco...
git_asdf.py:54: '--branch',
On 2014/07/15 22:47:20, agable wrote:
> Indent +4.

Done.

[iannucci, 2014-07-21 23:24:39]

To be clear, this is not really a cherry pick, but just uploading a diff of HEAD
v <commit> to Rietveld.

I'm skeptical that this will work with file renames. Additionally, I'm fairly
certain that upload.py will get the wrong base files.

[smut, 2014-07-22 17:52:46]

On 2014/07/21 23:24:39, iannucci wrote:
> To be clear, this is not really a cherry pick, but just uploading a diff of
HEAD
> v <commit> to Rietveld.
> 
> I'm skeptical that this will work with file renames. Additionally, I'm fairly
> certain that upload.py will get the wrong base files.

It's cherry pick in that it's done by SHA, which was a requirement for this
tool. Are you saying this will only get the wrong base files on a rename?
Because I have a sample CL that did not involve renames that worked:
https://chromereviews.googleplex.com/63797016/. That said, this does fail to
produce a side-by-side diff for some patchsets (but it produces the unified
diff).

To say that it won't work with renames is an overstatement. It works
(https://chromereviews.googleplex.com/59527013), it just considers a rename to
be a deletion + addition of an identical file. I think the review links
generated by this tool are basically just for CQing. The original commit that
was "cherry picked" from will have the nicer diff.

I can dig deeper into improving the diffs, but it'd be helpful to be pointed in
the right direction. I also don't think that these points are disputing the
design implemented in this CL, just implementation details. Correct me if I'm
wrong about that.

[iannucci, 2014-07-22 18:44:34]

On 2014/07/22 17:52:46, smut wrote:
> On 2014/07/21 23:24:39, iannucci wrote:
> > To be clear, this is not really a cherry pick, but just uploading a diff of
> HEAD
> > v <commit> to Rietveld.
> > 
> > I'm skeptical that this will work with file renames. Additionally, I'm
fairly
> > certain that upload.py will get the wrong base files.
> 
> It's cherry pick in that it's done by SHA, which was a requirement for this
> tool. Are you saying this will only get the wrong base files on a rename?
> Because I have a sample CL that did not involve renames that worked:
> https://chromereviews.googleplex.com/63797016/. That said, this does fail to
> produce a side-by-side diff for some patchsets (but it produces the unified
> diff).
> 
> To say that it won't work with renames is an overstatement. It works
> (https://chromereviews.googleplex.com/59527013), it just considers a rename to
> be a deletion + addition of an identical file. I think the review links
> generated by this tool are basically just for CQing. The original commit that
> was "cherry picked" from will have the nicer diff.
> 
> I can dig deeper into improving the diffs, but it'd be helpful to be pointed
in
> the right direction. I also don't think that these points are disputing the
> design implemented in this CL, just implementation details. Correct me if I'm
> wrong about that.

Right, just the implementation, the idea is good to me.

On second thought, I think you're right that renames and copies will work just
fine (at least in git). They won't work well for SVN-backed repos though, but
that's a vanishingly small concern.

Take a look at the way that upload.py finds base files. It'll be consulting the
working copy for the paths, I think. It could be made smarter to actually be
given a base git revision, and then use `git cat-file` to dig the base files out
directly from git. This would avoid the need for a checkout. It might also be
possible to prepare a mock checkout of just the files affected by the diff, and
then run upload.py in that.

[iannucci, 2014-07-22 18:46:13]

On 2014/07/22 18:44:34, iannucci wrote:
> On 2014/07/22 17:52:46, smut wrote:
> > On 2014/07/21 23:24:39, iannucci wrote:
> > > To be clear, this is not really a cherry pick, but just uploading a diff
of
> > HEAD
> > > v <commit> to Rietveld.
> > > 
> > > I'm skeptical that this will work with file renames. Additionally, I'm
> fairly
> > > certain that upload.py will get the wrong base files.
> > 
> > It's cherry pick in that it's done by SHA, which was a requirement for this
> > tool. Are you saying this will only get the wrong base files on a rename?
> > Because I have a sample CL that did not involve renames that worked:
> > https://chromereviews.googleplex.com/63797016/. That said, this does fail to
> > produce a side-by-side diff for some patchsets (but it produces the unified
> > diff).
> > 
> > To say that it won't work with renames is an overstatement. It works
> > (https://chromereviews.googleplex.com/59527013), it just considers a rename
to
> > be a deletion + addition of an identical file. I think the review links
> > generated by this tool are basically just for CQing. The original commit
that
> > was "cherry picked" from will have the nicer diff.
> > 
> > I can dig deeper into improving the diffs, but it'd be helpful to be pointed
> in
> > the right direction. I also don't think that these points are disputing the
> > design implemented in this CL, just implementation details. Correct me if
I'm
> > wrong about that.
> 
> Right, just the implementation, the idea is good to me.
> 
> On second thought, I think you're right that renames and copies will work just
> fine (at least in git). They won't work well for SVN-backed repos though, but
> that's a vanishingly small concern.
> 
> Take a look at the way that upload.py finds base files. It'll be consulting
the
> working copy for the paths, I think. It could be made smarter to actually be
> given a base git revision, and then use `git cat-file` to dig the base files
out
> directly from git. This would avoid the need for a checkout. It might also be
> possible to prepare a mock checkout of just the files affected by the diff,
and
> then run upload.py in that.

(upload.py is a Rietveld file whose source is here:
https://code.google.com/p/rietveld/source/browse/upload.py, but there's a copy
in depot_tools:
https://chromium.googlesource.com/chromium/tools/depot_tools/+/master/third_p...)

[smut, 2014-07-22 22:09:13]

On 2014/07/22 18:46:13, iannucci wrote:
> On 2014/07/22 18:44:34, iannucci wrote:
> > On 2014/07/22 17:52:46, smut wrote:
> > > On 2014/07/21 23:24:39, iannucci wrote:
> > > > To be clear, this is not really a cherry pick, but just uploading a diff
> of
> > > HEAD
> > > > v <commit> to Rietveld.
> > > > 
> > > > I'm skeptical that this will work with file renames. Additionally, I'm
> > fairly
> > > > certain that upload.py will get the wrong base files.
> > > 
> > > It's cherry pick in that it's done by SHA, which was a requirement for
this
> > > tool. Are you saying this will only get the wrong base files on a rename?
> > > Because I have a sample CL that did not involve renames that worked:
> > > https://chromereviews.googleplex.com/63797016/. That said, this does fail
to
> > > produce a side-by-side diff for some patchsets (but it produces the
unified
> > > diff).
> > > 
> > > To say that it won't work with renames is an overstatement. It works
> > > (https://chromereviews.googleplex.com/59527013), it just considers a
rename
> to
> > > be a deletion + addition of an identical file. I think the review links
> > > generated by this tool are basically just for CQing. The original commit
> that
> > > was "cherry picked" from will have the nicer diff.
> > > 
> > > I can dig deeper into improving the diffs, but it'd be helpful to be
pointed
> > in
> > > the right direction. I also don't think that these points are disputing
the
> > > design implemented in this CL, just implementation details. Correct me if
> I'm
> > > wrong about that.
> > 
> > Right, just the implementation, the idea is good to me.
> > 
> > On second thought, I think you're right that renames and copies will work
just
> > fine (at least in git). They won't work well for SVN-backed repos though,
but
> > that's a vanishingly small concern.
> > 
> > Take a look at the way that upload.py finds base files. It'll be consulting
> the
> > working copy for the paths, I think. It could be made smarter to actually be
> > given a base git revision, and then use `git cat-file` to dig the base files
> out
> > directly from git. This would avoid the need for a checkout. It might also
be
> > possible to prepare a mock checkout of just the files affected by the diff,
> and
> > then run upload.py in that.
> 
> (upload.py is a Rietveld file whose source is here:
> https://code.google.com/p/rietveld/source/browse/upload.py, but there's a copy
> in depot_tools:
>
https://chromium.googlesource.com/chromium/tools/depot_tools/+/master/third_p...)

I'm trying to limit use of upload.py, because it's very complex and handles a
lot of cases. Plus it has built-in assumptions that you're uploading your
current diff, whereas this tool has nothing to do with the state of your current
local branch.

I've added base file uploading. Here are some up-to-date sample CLs created by
the latest patch set: https://chromereviews.googleplex.com/60557014/,
https://chromereviews.googleplex.com/63367014/.

[iannucci, 2014-07-22 22:16:03]

On 2014/07/22 22:09:13, smut wrote:
> On 2014/07/22 18:46:13, iannucci wrote:
> > On 2014/07/22 18:44:34, iannucci wrote:
> > > On 2014/07/22 17:52:46, smut wrote:
> > > > On 2014/07/21 23:24:39, iannucci wrote:
> > > > > To be clear, this is not really a cherry pick, but just uploading a
diff
> > of
> > > > HEAD
> > > > > v <commit> to Rietveld.
> > > > > 
> > > > > I'm skeptical that this will work with file renames. Additionally, I'm
> > > fairly
> > > > > certain that upload.py will get the wrong base files.
> > > > 
> > > > It's cherry pick in that it's done by SHA, which was a requirement for
> this
> > > > tool. Are you saying this will only get the wrong base files on a
rename?
> > > > Because I have a sample CL that did not involve renames that worked:
> > > > https://chromereviews.googleplex.com/63797016/. That said, this does
fail
> to
> > > > produce a side-by-side diff for some patchsets (but it produces the
> unified
> > > > diff).
> > > > 
> > > > To say that it won't work with renames is an overstatement. It works
> > > > (https://chromereviews.googleplex.com/59527013), it just considers a
> rename
> > to
> > > > be a deletion + addition of an identical file. I think the review links
> > > > generated by this tool are basically just for CQing. The original commit
> > that
> > > > was "cherry picked" from will have the nicer diff.
> > > > 
> > > > I can dig deeper into improving the diffs, but it'd be helpful to be
> pointed
> > > in
> > > > the right direction. I also don't think that these points are disputing
> the
> > > > design implemented in this CL, just implementation details. Correct me
if
> > I'm
> > > > wrong about that.
> > > 
> > > Right, just the implementation, the idea is good to me.
> > > 
> > > On second thought, I think you're right that renames and copies will work
> just
> > > fine (at least in git). They won't work well for SVN-backed repos though,
> but
> > > that's a vanishingly small concern.
> > > 
> > > Take a look at the way that upload.py finds base files. It'll be
consulting
> > the
> > > working copy for the paths, I think. It could be made smarter to actually
be
> > > given a base git revision, and then use `git cat-file` to dig the base
files
> > out
> > > directly from git. This would avoid the need for a checkout. It might also
> be
> > > possible to prepare a mock checkout of just the files affected by the
diff,
> > and
> > > then run upload.py in that.
> > 
> > (upload.py is a Rietveld file whose source is here:
> > https://code.google.com/p/rietveld/source/browse/upload.py, but there's a
copy
> > in depot_tools:
> >
>
https://chromium.googlesource.com/chromium/tools/depot_tools/+/master/third_p...)
> 
> I'm trying to limit use of upload.py, because it's very complex and handles a
> lot of cases. Plus it has built-in assumptions that you're uploading your
> current diff, whereas this tool has nothing to do with the state of your
current
> local branch.
> 
> I've added base file uploading. Here are some up-to-date sample CLs created by
> the latest patch set: https://chromereviews.googleplex.com/60557014/,
> https://chromereviews.googleplex.com/63367014/.

Ah, I see... Be aware that this /will/ break in the near future, as jrobbins is
rewriting the upload protocol, and it's widely assumed that upload.py is the
only implementor of that protocol. If you cleaned up upload.py, then that
cleaned up interface would be ported over, and should continue to work after the
new protocol is in place.

[smut, 2014-07-22 22:21:23]

On 2014/07/22 22:16:03, iannucci wrote:
> On 2014/07/22 22:09:13, smut wrote:
> > On 2014/07/22 18:46:13, iannucci wrote:
> > > On 2014/07/22 18:44:34, iannucci wrote:
> > > > On 2014/07/22 17:52:46, smut wrote:
> > > > > On 2014/07/21 23:24:39, iannucci wrote:
> > > > > > To be clear, this is not really a cherry pick, but just uploading a
> diff
> > > of
> > > > > HEAD
> > > > > > v <commit> to Rietveld.
> > > > > > 
> > > > > > I'm skeptical that this will work with file renames. Additionally,
I'm
> > > > fairly
> > > > > > certain that upload.py will get the wrong base files.
> > > > > 
> > > > > It's cherry pick in that it's done by SHA, which was a requirement for
> > this
> > > > > tool. Are you saying this will only get the wrong base files on a
> rename?
> > > > > Because I have a sample CL that did not involve renames that worked:
> > > > > https://chromereviews.googleplex.com/63797016/. That said, this does
> fail
> > to
> > > > > produce a side-by-side diff for some patchsets (but it produces the
> > unified
> > > > > diff).
> > > > > 
> > > > > To say that it won't work with renames is an overstatement. It works
> > > > > (https://chromereviews.googleplex.com/59527013), it just considers a
> > rename
> > > to
> > > > > be a deletion + addition of an identical file. I think the review
links
> > > > > generated by this tool are basically just for CQing. The original
commit
> > > that
> > > > > was "cherry picked" from will have the nicer diff.
> > > > > 
> > > > > I can dig deeper into improving the diffs, but it'd be helpful to be
> > pointed
> > > > in
> > > > > the right direction. I also don't think that these points are
disputing
> > the
> > > > > design implemented in this CL, just implementation details. Correct me
> if
> > > I'm
> > > > > wrong about that.
> > > > 
> > > > Right, just the implementation, the idea is good to me.
> > > > 
> > > > On second thought, I think you're right that renames and copies will
work
> > just
> > > > fine (at least in git). They won't work well for SVN-backed repos
though,
> > but
> > > > that's a vanishingly small concern.
> > > > 
> > > > Take a look at the way that upload.py finds base files. It'll be
> consulting
> > > the
> > > > working copy for the paths, I think. It could be made smarter to
actually
> be
> > > > given a base git revision, and then use `git cat-file` to dig the base
> files
> > > out
> > > > directly from git. This would avoid the need for a checkout. It might
also
> > be
> > > > possible to prepare a mock checkout of just the files affected by the
> diff,
> > > and
> > > > then run upload.py in that.
> > > 
> > > (upload.py is a Rietveld file whose source is here:
> > > https://code.google.com/p/rietveld/source/browse/upload.py, but there's a
> copy
> > > in depot_tools:
> > >
> >
>
https://chromium.googlesource.com/chromium/tools/depot_tools/+/master/third_p...)
> > 
> > I'm trying to limit use of upload.py, because it's very complex and handles
a
> > lot of cases. Plus it has built-in assumptions that you're uploading your
> > current diff, whereas this tool has nothing to do with the state of your
> current
> > local branch.
> > 
> > I've added base file uploading. Here are some up-to-date sample CLs created
by
> > the latest patch set: https://chromereviews.googleplex.com/60557014/,
> > https://chromereviews.googleplex.com/63367014/.
> 
> Ah, I see... Be aware that this /will/ break in the near future, as jrobbins
is
> rewriting the upload protocol, and it's widely assumed that upload.py is the
> only implementor of that protocol. If you cleaned up upload.py, then that
> cleaned up interface would be ported over, and should continue to work after
the
> new protocol is in place.

I'm not sure how to go about genericizing upload.py so that it can upload
arbitrary patchsets and not just the diff against HEAD. I'm tempted to say that
I'll just fix the breakage in my script when the uploading API changes. I assume
that will be simpler.

[iannucci, 2014-07-22 22:34:08]

On 2014/07/22 22:21:23, smut wrote:
> On 2014/07/22 22:16:03, iannucci wrote:
> > On 2014/07/22 22:09:13, smut wrote:
> > > On 2014/07/22 18:46:13, iannucci wrote:
> > > > On 2014/07/22 18:44:34, iannucci wrote:
> > > > > On 2014/07/22 17:52:46, smut wrote:
> > > > > > On 2014/07/21 23:24:39, iannucci wrote:
> > > > > > > To be clear, this is not really a cherry pick, but just uploading
a
> > diff
> > > > of
> > > > > > HEAD
> > > > > > > v <commit> to Rietveld.
> > > > > > > 
> > > > > > > I'm skeptical that this will work with file renames. Additionally,
> I'm
> > > > > fairly
> > > > > > > certain that upload.py will get the wrong base files.
> > > > > > 
> > > > > > It's cherry pick in that it's done by SHA, which was a requirement
for
> > > this
> > > > > > tool. Are you saying this will only get the wrong base files on a
> > rename?
> > > > > > Because I have a sample CL that did not involve renames that worked:
> > > > > > https://chromereviews.googleplex.com/63797016/. That said, this does
> > fail
> > > to
> > > > > > produce a side-by-side diff for some patchsets (but it produces the
> > > unified
> > > > > > diff).
> > > > > > 
> > > > > > To say that it won't work with renames is an overstatement. It works
> > > > > > (https://chromereviews.googleplex.com/59527013), it just considers a
> > > rename
> > > > to
> > > > > > be a deletion + addition of an identical file. I think the review
> links
> > > > > > generated by this tool are basically just for CQing. The original
> commit
> > > > that
> > > > > > was "cherry picked" from will have the nicer diff.
> > > > > > 
> > > > > > I can dig deeper into improving the diffs, but it'd be helpful to be
> > > pointed
> > > > > in
> > > > > > the right direction. I also don't think that these points are
> disputing
> > > the
> > > > > > design implemented in this CL, just implementation details. Correct
me
> > if
> > > > I'm
> > > > > > wrong about that.
> > > > > 
> > > > > Right, just the implementation, the idea is good to me.
> > > > > 
> > > > > On second thought, I think you're right that renames and copies will
> work
> > > just
> > > > > fine (at least in git). They won't work well for SVN-backed repos
> though,
> > > but
> > > > > that's a vanishingly small concern.
> > > > > 
> > > > > Take a look at the way that upload.py finds base files. It'll be
> > consulting
> > > > the
> > > > > working copy for the paths, I think. It could be made smarter to
> actually
> > be
> > > > > given a base git revision, and then use `git cat-file` to dig the base
> > files
> > > > out
> > > > > directly from git. This would avoid the need for a checkout. It might
> also
> > > be
> > > > > possible to prepare a mock checkout of just the files affected by the
> > diff,
> > > > and
> > > > > then run upload.py in that.
> > > > 
> > > > (upload.py is a Rietveld file whose source is here:
> > > > https://code.google.com/p/rietveld/source/browse/upload.py, but there's
a
> > copy
> > > > in depot_tools:
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/tools/depot_tools/+/master/third_p...)
> > > 
> > > I'm trying to limit use of upload.py, because it's very complex and
handles
> a
> > > lot of cases. Plus it has built-in assumptions that you're uploading your
> > > current diff, whereas this tool has nothing to do with the state of your
> > current
> > > local branch.
> > > 
> > > I've added base file uploading. Here are some up-to-date sample CLs
created
> by
> > > the latest patch set: https://chromereviews.googleplex.com/60557014/,
> > > https://chromereviews.googleplex.com/63367014/.
> > 
> > Ah, I see... Be aware that this /will/ break in the near future, as jrobbins
> is
> > rewriting the upload protocol, and it's widely assumed that upload.py is the
> > only implementor of that protocol. If you cleaned up upload.py, then that
> > cleaned up interface would be ported over, and should continue to work after
> the
> > new protocol is in place.
> 
> I'm not sure how to go about genericizing upload.py so that it can upload
> arbitrary patchsets and not just the diff against HEAD. I'm tempted to say
that
> I'll just fix the breakage in my script when the uploading API changes. I
assume
> that will be simpler.

That may indeed be simpler. upload.py is pretty ugly. I was just wondering if it
was possible to refactor it into more of a library so that you could use it (and
that it's main() method would also use the library portion).

[smut, 2014-07-22 22:36:14]

On 2014/07/22 22:34:08, iannucci wrote:
> On 2014/07/22 22:21:23, smut wrote:
> > On 2014/07/22 22:16:03, iannucci wrote:
> > > On 2014/07/22 22:09:13, smut wrote:
> > > > On 2014/07/22 18:46:13, iannucci wrote:
> > > > > On 2014/07/22 18:44:34, iannucci wrote:
> > > > > > On 2014/07/22 17:52:46, smut wrote:
> > > > > > > On 2014/07/21 23:24:39, iannucci wrote:
> > > > > > > > To be clear, this is not really a cherry pick, but just
uploading
> a
> > > diff
> > > > > of
> > > > > > > HEAD
> > > > > > > > v <commit> to Rietveld.
> > > > > > > > 
> > > > > > > > I'm skeptical that this will work with file renames.
Additionally,
> > I'm
> > > > > > fairly
> > > > > > > > certain that upload.py will get the wrong base files.
> > > > > > > 
> > > > > > > It's cherry pick in that it's done by SHA, which was a requirement
> for
> > > > this
> > > > > > > tool. Are you saying this will only get the wrong base files on a
> > > rename?
> > > > > > > Because I have a sample CL that did not involve renames that
worked:
> > > > > > > https://chromereviews.googleplex.com/63797016/. That said, this
does
> > > fail
> > > > to
> > > > > > > produce a side-by-side diff for some patchsets (but it produces
the
> > > > unified
> > > > > > > diff).
> > > > > > > 
> > > > > > > To say that it won't work with renames is an overstatement. It
works
> > > > > > > (https://chromereviews.googleplex.com/59527013), it just considers
a
> > > > rename
> > > > > to
> > > > > > > be a deletion + addition of an identical file. I think the review
> > links
> > > > > > > generated by this tool are basically just for CQing. The original
> > commit
> > > > > that
> > > > > > > was "cherry picked" from will have the nicer diff.
> > > > > > > 
> > > > > > > I can dig deeper into improving the diffs, but it'd be helpful to
be
> > > > pointed
> > > > > > in
> > > > > > > the right direction. I also don't think that these points are
> > disputing
> > > > the
> > > > > > > design implemented in this CL, just implementation details.
Correct
> me
> > > if
> > > > > I'm
> > > > > > > wrong about that.
> > > > > > 
> > > > > > Right, just the implementation, the idea is good to me.
> > > > > > 
> > > > > > On second thought, I think you're right that renames and copies will
> > work
> > > > just
> > > > > > fine (at least in git). They won't work well for SVN-backed repos
> > though,
> > > > but
> > > > > > that's a vanishingly small concern.
> > > > > > 
> > > > > > Take a look at the way that upload.py finds base files. It'll be
> > > consulting
> > > > > the
> > > > > > working copy for the paths, I think. It could be made smarter to
> > actually
> > > be
> > > > > > given a base git revision, and then use `git cat-file` to dig the
base
> > > files
> > > > > out
> > > > > > directly from git. This would avoid the need for a checkout. It
might
> > also
> > > > be
> > > > > > possible to prepare a mock checkout of just the files affected by
the
> > > diff,
> > > > > and
> > > > > > then run upload.py in that.
> > > > > 
> > > > > (upload.py is a Rietveld file whose source is here:
> > > > > https://code.google.com/p/rietveld/source/browse/upload.py, but
there's
> a
> > > copy
> > > > > in depot_tools:
> > > > >
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/tools/depot_tools/+/master/third_p...)
> > > > 
> > > > I'm trying to limit use of upload.py, because it's very complex and
> handles
> > a
> > > > lot of cases. Plus it has built-in assumptions that you're uploading
your
> > > > current diff, whereas this tool has nothing to do with the state of your
> > > current
> > > > local branch.
> > > > 
> > > > I've added base file uploading. Here are some up-to-date sample CLs
> created
> > by
> > > > the latest patch set: https://chromereviews.googleplex.com/60557014/,
> > > > https://chromereviews.googleplex.com/63367014/.
> > > 
> > > Ah, I see... Be aware that this /will/ break in the near future, as
jrobbins
> > is
> > > rewriting the upload protocol, and it's widely assumed that upload.py is
the
> > > only implementor of that protocol. If you cleaned up upload.py, then that
> > > cleaned up interface would be ported over, and should continue to work
after
> > the
> > > new protocol is in place.
> > 
> > I'm not sure how to go about genericizing upload.py so that it can upload
> > arbitrary patchsets and not just the diff against HEAD. I'm tempted to say
> that
> > I'll just fix the breakage in my script when the uploading API changes. I
> assume
> > that will be simpler.
> 
> That may indeed be simpler. upload.py is pretty ugly. I was just wondering if
it
> was possible to refactor it into more of a library so that you could use it
(and
> that it's main() method would also use the library portion).

Maybe at some point. How's the current state of this tool?

[smut, 2014-07-23 21:42:44]

I've added a manpage entry for git-asdf. Does anyone want to propose a useful
name? How about git-upload-cherry-pick? Too long? Inaccurate?

[agable, 2014-07-28 21:31:55]

On 2014/07/23 21:42:44, smut wrote:
> I've added a manpage entry for git-asdf. Does anyone want to propose a useful
> name? How about git-upload-cherry-pick? Too long? Inaccurate?

I kinda like git boom-lift. Or git bucket-truck. Heh. Bucket truck.

[smut, 2014-07-29 18:38:53]

git-cherry-pick-upload

[iannucci, 2014-07-29 18:41:35]

lgtm, Can probably beef up the CL description a smidge.

https://codereview.chromium.org/397593004/diff/150001/man/src/git-cherry-pick...
File man/src/git-cherry-pick-upload.demo.1.sh (right):

https://codereview.chromium.org/397593004/diff/150001/man/src/git-cherry-pick...
man/src/git-cherry-pick-upload.demo.1.sh:4: run git cherry-pick-upload -h
fancy... actually that's a damn good idea for generating the SYNOPSIS section...
/me files this into brain for later implementation.

https://codereview.chromium.org/397593004/diff/150001/man/src/git-cherry-pick...
File man/src/git-cherry-pick-upload.txt (right):

https://codereview.chromium.org/397593004/diff/150001/man/src/git-cherry-pick...
man/src/git-cherry-pick-upload.txt:12: 'git cherry-pick-upload'
should have example options here, probably -b <local_branch_ref>
<hash_to_cherry_pick>

https://codereview.chromium.org/397593004/diff/150001/man/src/git-cherry-pick...
man/src/git-cherry-pick-upload.txt:19: and then uploads that diff to rietveld.
This paragraph is confusing. I think you mean 

It cherry-picks '<hash>' onto the specified branch, using your local clone's
state of the branch. Because of this, make your local branch ref is up-to-date
(using git-fetch) before using this command. Otherwise the CL that this uploads
may not apply cleanly to the branch when you try to land it.

mebbeh?

[smut, 2014-07-29 18:54:35]

The CQ bit was checked by smut@google.com

[commit-bot: I haz the power, 2014-07-29 18:55:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/smut@google.com/397593004/170001

[commit-bot: I haz the power, 2014-07-29 18:57:09]

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

[commit-bot: I haz the power, 2014-07-29 18:57:10]

Presubmit check for 397593004-170001 failed and returned exit status 1.

Running presubmit commit checks ...
Checking out rietveld...
Running save-description-on-failure.sh
Running push-basic.sh
Running upstream.sh
Running submit-from-new-dir.sh
Running abandon.sh
Running submodule-merge-test.sh
Running upload-local-tracking-branch.sh
Running hooks.sh
Running post-dcommit-hook-test.sh
Running upload-stale.sh
Running patch.sh
Running basic.sh

** Presubmit ERRORS **
Pylint (100 files) (33.16s) failed
************* Module
/b/infra_internal/commit_queue/workdir/tools/depot_tools/git_cherry_pick_upload.py
W0212: 52,11:cherry_pick: Access to a protected member _send of a client class
W0212: 91,56:cherry_pick: Access to a protected member _send of a client class
W0212:113,42:cherry_pick: Access to a protected member _send of a client class
W0212:119,32:cherry_pick: Access to a protected member _send of a client class


Presubmit checks took 107.3s to calculate.

Was the presubmit check useful? If not, run "git cl presubmit -v"
to figure out which PRESUBMIT.py was run, then run git blame
on the file to figure out who to ask for help.

[smut, 2014-07-29 18:59:04]

The CQ bit was checked by smut@google.com

[commit-bot: I haz the power, 2014-07-29 18:59:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/smut@google.com/397593004/190001

[commit-bot: I haz the power, 2014-07-29 19:01:15]

Change committed as 286273

[smut, 2014-07-29 22:15:44]

https://codereview.chromium.org/397593004/diff/150001/man/src/git-cherry-pick...
File man/src/git-cherry-pick-upload.txt (right):

https://codereview.chromium.org/397593004/diff/150001/man/src/git-cherry-pick...
man/src/git-cherry-pick-upload.txt:12: 'git cherry-pick-upload'
On 2014/07/29 18:41:35, iannucci wrote:
> should have example options here, probably -b <local_branch_ref>
> <hash_to_cherry_pick>

Done.

https://codereview.chromium.org/397593004/diff/150001/man/src/git-cherry-pick...
man/src/git-cherry-pick-upload.txt:19: and then uploads that diff to rietveld.
On 2014/07/29 18:41:35, iannucci wrote:
> This paragraph is confusing. I think you mean 
> 
> It cherry-picks '<hash>' onto the specified branch, using your local clone's
> state of the branch. Because of this, make your local branch ref is up-to-date
> (using git-fetch) before using this command. Otherwise the CL that this
uploads
> may not apply cleanly to the branch when you try to land it.
> 
> mebbeh?

It doesn't cherry pick the hash onto anything. It finds the parent of the hash,
generates the diff, and uploads that diff to rietveld with a modified 'project'
parameter.

[iannucci, 2014-07-29 22:53:01]

On 2014/07/29 22:15:44, smut wrote:
>
https://codereview.chromium.org/397593004/diff/150001/man/src/git-cherry-pick...
> File man/src/git-cherry-pick-upload.txt (right):
> 
>
https://codereview.chromium.org/397593004/diff/150001/man/src/git-cherry-pick...
> man/src/git-cherry-pick-upload.txt:12: 'git cherry-pick-upload'
> On 2014/07/29 18:41:35, iannucci wrote:
> > should have example options here, probably -b <local_branch_ref>
> > <hash_to_cherry_pick>
> 
> Done.
> 
>
https://codereview.chromium.org/397593004/diff/150001/man/src/git-cherry-pick...
> man/src/git-cherry-pick-upload.txt:19: and then uploads that diff to rietveld.
> On 2014/07/29 18:41:35, iannucci wrote:
> > This paragraph is confusing. I think you mean 
> > 
> > It cherry-picks '<hash>' onto the specified branch, using your local clone's
> > state of the branch. Because of this, make your local branch ref is
up-to-date
> > (using git-fetch) before using this command. Otherwise the CL that this
> uploads
> > may not apply cleanly to the branch when you try to land it.
> > 
> > mebbeh?
> 
> It doesn't cherry pick the hash onto anything. It finds the parent of the
hash,
> generates the diff, and uploads that diff to rietveld with a modified
'project'
> parameter.

oh, right. okie dokie.