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 accidentally included by mak
Done


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@109
PS3, Line 109:
> This is a list (not a set or dict-with-empty-values), and it has to be in a
Done


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@114
PS3, Line 114: ranch_
> nit: "function"
Done


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@119
PS3, Line 119:       sh.git.fetch(options.target_remote_name)
> nit: move above line 116 for easier tracing by users?
Done


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@126
PS3, Line 126:   target_commits = build_commit_map(full_target_branch_name, 
merge_base)
> Maybe throw, for easier recovery by the caller?
I didn't do this. I don't like printing exceptions to the end user for 
something that is a managed error, and doing try-catch on the single caller 
seems over-wrought.


http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@140
PS3, Line 140:   skip_commits_matching = options.skip_commits_matching.replace(
> nit: long line
Done


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



--
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 <[email protected]>
Gerrit-Reviewer: Jim Apple <[email protected]>
Gerrit-Reviewer: Philip Zeyliger <[email protected]>
Gerrit-Reviewer: Taras Bobrovytsky <[email protected]>
Gerrit-Comment-Date: Thu, 18 Jan 2018 19:20:52 +0000
Gerrit-HasComments: Yes

Reply via email to