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
