[Impala-ASF-CR] IMPALA-6410: Tool to cherrypick changes across branches.

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9045 )

Change subject: IMPALA-6410: Tool to cherrypick changes across branches.
..

IMPALA-6410: Tool to cherrypick changes across branches.

This script compares two branches and optionally cherry-picks changes
across. It uses the Gerrit Change-Id as the key, and it supports a
configuration file and a string to ignore commits.

Change-Id: I6120ec2d6e914a1e5fda568178b32aafda8722a9
Reviewed-on: http://gerrit.cloudera.org:8080/9045
Reviewed-by: Jim Apple 
Tested-by: Impala Public Jenkins
---
A bin/compare_branches.py
A bin/ignored_commits.json
2 files changed, 274 insertions(+), 0 deletions(-)

Approvals:
  Jim Apple: Looks good to me, approved
  Impala Public Jenkins: Verified

--
To view, visit http://gerrit.cloudera.org:8080/9045
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I6120ec2d6e914a1e5fda568178b32aafda8722a9
Gerrit-Change-Number: 9045
Gerrit-PatchSet: 6
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-6410: Tool to cherrypick changes across branches.

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9045 )

Change subject: IMPALA-6410: Tool to cherrypick changes across branches.
..


Patch Set 5: Verified+1


--
To view, visit http://gerrit.cloudera.org:8080/9045
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6120ec2d6e914a1e5fda568178b32aafda8722a9
Gerrit-Change-Number: 9045
Gerrit-PatchSet: 5
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Wed, 24 Jan 2018 03:34:51 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6410: Tool to cherrypick changes across branches.

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9045 )

Change subject: IMPALA-6410: Tool to cherrypick changes across branches.
..


Patch Set 5:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1791/


--
To view, visit http://gerrit.cloudera.org:8080/9045
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6120ec2d6e914a1e5fda568178b32aafda8722a9
Gerrit-Change-Number: 9045
Gerrit-PatchSet: 5
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Wed, 24 Jan 2018 00:03:36 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6410: Tool to cherrypick changes across branches.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9045 )

Change subject: IMPALA-6410: Tool to cherrypick changes across branches.
..


Patch Set 5: Code-Review+2

Nice work!


--
To view, visit http://gerrit.cloudera.org:8080/9045
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6120ec2d6e914a1e5fda568178b32aafda8722a9
Gerrit-Change-Number: 9045
Gerrit-PatchSet: 5
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Tue, 23 Jan 2018 04:34:53 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6410: Tool to cherrypick changes across branches.

[Philip Zeyliger (Code Review)]

Philip Zeyliger has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9045 )

Change subject: IMPALA-6410: Tool to cherrypick changes across branches.
..


Patch Set 2:

(25 comments)

Thanks for the detailed review. Here's another iteration!

http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py
File bin/compare_branches.py:

http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@19
PS3, Line 19: #
> Can you add a motivation section on why this is automated? If it were singl
Done


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@25
PS3, Line 25: g
> nit: heterogeneous spacing.
Done


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@26
PS3, Line 26:
> nit: long line
Done


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@26
PS3, Line 26:
> Can you make the commits a struct with a field for the hash and an optional
Done.


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@32
PS3, Line 32: import sh
> Big-picture-wise, I think it would help if there were some prose explaining
I captured this at https://issues.apache.org/jira/browse/IMPALA-6410.


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@49
PS3, Line 49: for result_dict in json_data:
> nit: The param is a file name, yes? Can you make a note that it uses the js
Done


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@50
PS3, Line 50:   logging.debug("Parsing result_dict: 
{0}".format(result_dict))
> nit: we normally use https://www.python.org/dev/peps/pep-0257/: double quot
I think I followed this suggestion.


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@68
PS3, Line 68:
> git help log calls this the "subject", rather than the message.
Done


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@71
PS3, Line 71: d = ch
> Possibl digression: does sh.git have documentation other than the generic s
What do you mean by "tabbed values"?

I don't think "sh.git" is special. Here's an echo example:

>>> import sh
>>> sh.echo.hi("--foo")
hi --foo

We use "sh" all over the place, so this is consistent with that, but I 
generally find it more permissive than need be.


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@75
PS3, Line 75: ebug("Comm
> Why swallow this exception?
I removed it. This was poorly handling what happens when git log returns 
nothing and also the newline that git throws in at the end. I handled it more 
clearly.

I also, um, added a commit message with a UTF8 character and I seem to have 
survived that (with one fix).


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@79
PS3, Line 79:
> Log an error if there are two or more matches?
I changed this to use findall() and capture all of them. search was just 
finding the first one, but I think it's possible for someone to mention two 
jiras.

Here are some examples:

$git log --oneline | grep IMPALA.*IMPALA | head -n 3
ceeb130 IMPALA-2172, IMPALA-6391: [DOCS] Distinguish char_length() from length()
604e48d IMPALA-6330, IMPALA-5702: Avoid boost's trim() to workaround crash 
after dynamic linking.
b4a73a6 IMPALA-4978 / IMPALA-5631: [DOCS] Add FQDN known issue


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@82
PS3, Line 82:   help='Cherry-pick mismatched commits to current branch.')
> nit: long line
Done


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@86
PS3, Line 86:   help='Name of the source git remote. If set to empty 
string, ' +
> It would help readers of this file (I think) if this were right after the c
Done. And I made --help include the help text at the top.

I moved things to argparse.


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@90
PS3, Line 90:
> Also has to be the target branch, yes?
Updated the help text. You shouldn't really check out "remote branches" (it's 
not particularly meaningful), but you should check something out that matches.


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@95
PS3, Line 95: at causes the commit to be
> By that, do you mean that the default is to compare asf-gerrit/master and a
Tried to clarify this.


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@97
PS3, Line 97:   parser.add_option('--verbose', '-v', action='store_true', 
default=False,
> So the special empty-string behavior applies to this flag, too, yes?
Done


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@101
PS3, Line 101:
> Can you call out a place to learn the schema?
Done


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@102
PS3, Line 102:   log_level = logging.WARNING
> Can you add a note about case-sensitivity?
Done


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@102
PS3, Line 102:
> What about making this a bit less likely to be 

[Impala-ASF-CR] IMPALA-6410: Tool to cherrypick changes across branches.

[Philip Zeyliger (Code Review)]

Hello Taras Bobrovytsky, Jim Apple,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9045

to look at the new patch set (#4).

Change subject: IMPALA-6410: Tool to cherrypick changes across branches.
..

IMPALA-6410: Tool to cherrypick changes across branches.

This script compares two branches and optionally cherry-picks changes
across. It uses the Gerrit Change-Id as the key, and it supports a
configuration file and a string to ignore commits.

Change-Id: I6120ec2d6e914a1e5fda568178b32aafda8722a9
---
A bin/compare_branches.py
A bin/ignored_commits.json
2 files changed, 274 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/9045/4
--
To view, visit http://gerrit.cloudera.org:8080/9045
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6120ec2d6e914a1e5fda568178b32aafda8722a9
Gerrit-Change-Number: 9045
Gerrit-PatchSet: 4
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-6410: Tool to cherrypick changes across branches.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9045 )

Change subject: IMPALA-6410: Tool to cherrypick changes across branches.
..


Patch Set 3:

(29 comments)

Thanks for taking this on!

http://gerrit.cloudera.org:8080/#/c/9045/2/bin/compare_branches.py
File bin/compare_branches.py:

http://gerrit.cloudera.org:8080/#/c/9045/2/bin/compare_branches.py@15
PS2, Line 15: # Compares two specified branches, using the Gerrit Change-Id as 
the
Can you add an example invocation and the output of running it with --help?


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py
File bin/compare_branches.py:

http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@15
PS3, Line 15: # Compares two specified branches, using the Gerrit Change-Id as 
the
Could you add a couple of example invocations?


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@19
PS3, Line 19: #
Can you add a motivation section on why this is automated? If it were single 
cherry-picks, we would just do them manually, but this is special because it 
helps maintain two active development branches.


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@25
PS3, Line 25:
nit: heterogeneous spacing.


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@26
PS3, Line 26: # "commits": [ "", 
"" ]
nit: long line


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@26
PS3, Line 26: 
Can you make the commits a struct with a field for the hash and an optional 
field for comments on why a particular commit is ignored?

Also, can you call out the requirements for the hash? It has to be the full 
hash, yes?


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@32
PS3, Line 32: # Debug logging to stderr can be enabled with the --verbose flag.
Big-picture-wise, I think it would help if there were some prose explaining the 
process of maintaining two active branches, including things like how on 
denotes that a patch should not be cherry picked, when review happens on 
cherry-picks, and so on. Either Wiki or README would work, I think. What do you 
think?


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@49
PS3, Line 49: def read_ignored_commits(ignored_commits_file):
nit: The param is a file name, yes? Can you make a note that it uses the json 
format above?


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@50
PS3, Line 50:   '''Returns a dictionary containing commits that should be 
ignored.
nit: we normally use https://www.python.org/dev/peps/pep-0257/: double quotes, 
single subject line, then an empty line, then indentation of the following line 
that lines up with the quotation mark (not the first line's prose).


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@64
PS3, Line 64:   '''Creates a map from change id to (commit_msg, hash, author, 
date, body).'''
nit: can this tuple have the same order as the destructuring on line 74?


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@68
PS3, Line 68: %s
git help log calls this the "subject", rather than the message.


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@71
PS3, Line 71: sh.git
Possibl digression: does sh.git have documentation other than the generic sh 
documentation and https://amoffat.github.io/sh/sections/contrib.html#git?

I was hoping to understand if there was a way to separate tabbed values that 
was the common idiom.


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@75
PS3, Line 75: ValueError
Why swallow this exception?


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@79
PS3, Line 79: 0
Log an error if there are two or more matches?


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@82
PS3, Line 82:   logging.debug('Warning: commit {0} ({1}...) has no 
Change-Id.'.format(commit_hash, msg[:40]))
nit: long line


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@86
PS3, Line 86: def create_parser():
It would help readers of this file (I think) if this were right after the 
comment that opens the file. I think a few of our other scripts even paste the 
--help into the comment at the top.


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@90
PS3, Line 90: current
Also has to be the target branch, yes?


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@95
PS3, Line 95: branch names are used as is
By that, do you mean that the default is to compare asf-gerrit/master and 
asf-gerrit/2.x?


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@97
PS3, Line 97:   help='Name of the target git remote; defaults to source 
remote')
So the special empty-string 

[Impala-ASF-CR] IMPALA-6410: Tool to cherrypick changes across branches.

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9045 )

Change subject: IMPALA-6410: Tool to cherrypick changes across branches.
..


Patch Set 3: Code-Review+1


--
To view, visit http://gerrit.cloudera.org:8080/9045
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6120ec2d6e914a1e5fda568178b32aafda8722a9
Gerrit-Change-Number: 9045
Gerrit-PatchSet: 3
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Wed, 17 Jan 2018 23:37:28 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6410: Tool to cherrypick changes across branches.

[Philip Zeyliger (Code Review)]

Hello Taras Bobrovytsky,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9045

to look at the new patch set (#3).

Change subject: IMPALA-6410: Tool to cherrypick changes across branches.
..

IMPALA-6410: Tool to cherrypick changes across branches.

This script compares two branches and optionally cherry-picks changes
across. It uses the Gerrit Change-Id as the key, and it supports a
configuration file and a string to ignore commits.

Change-Id: I6120ec2d6e914a1e5fda568178b32aafda8722a9
---
A bin/compare_branches.py
A bin/ignored_commits.json
2 files changed, 218 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/9045/3
--
To view, visit http://gerrit.cloudera.org:8080/9045
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6120ec2d6e914a1e5fda568178b32aafda8722a9
Gerrit-Change-Number: 9045
Gerrit-PatchSet: 3
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-6410: Tool to cherrypick changes across branches.

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9045 )

Change subject: IMPALA-6410: Tool to cherrypick changes across branches.
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/9045/2/bin/compare_branches.py
File bin/compare_branches.py:

http://gerrit.cloudera.org:8080/#/c/9045/2/bin/compare_branches.py@21
PS2, Line 21: # [ { "source": "master", "target": "2.x", "commits": [ 
"", 
"" ] } ]
long line


http://gerrit.cloudera.org:8080/#/c/9045/2/bin/compare_branches.py@30
PS2, Line 30: from pprint import pformat
We should have "import..." grouped together and sorted, followed by "from ... 
import ..." statements also sorted


http://gerrit.cloudera.org:8080/#/c/9045/2/bin/compare_branches.py@179
PS2, Line 179: print "Cherrypicking %d changes." % 
(len(cherry_pick_hashes),)
The main() function seems long. Do you think it would make sense to separate 
the cherry picking into its own function?



--
To view, visit http://gerrit.cloudera.org:8080/9045
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6120ec2d6e914a1e5fda568178b32aafda8722a9
Gerrit-Change-Number: 9045
Gerrit-PatchSet: 2
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Wed, 17 Jan 2018 20:35:50 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6410: Tool to cherrypick changes across branches.

[Philip Zeyliger (Code Review)]

Philip Zeyliger has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9045


Change subject: IMPALA-6410: Tool to cherrypick changes across branches.
..

IMPALA-6410: Tool to cherrypick changes across branches.

This script compares two branches and optionally cherry-picks changes
across. It uses the Gerrit Change-Id as the key, and it supports a
configuration file and a string to ignore commits.

Change-Id: I6120ec2d6e914a1e5fda568178b32aafda8722a9
---
A bin/compare_branches.py
A bin/ignored_commits.json
2 files changed, 194 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/9045/1
--
To view, visit http://gerrit.cloudera.org:8080/9045
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6120ec2d6e914a1e5fda568178b32aafda8722a9
Gerrit-Change-Number: 9045
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger