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": [ "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" ] nit: long line http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@26 PS3, Line 26: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa 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 behavior applies to this flag, too, yes? http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@101 PS3, Line 101: help='JSON File that contains ignored commits.') Can you call out a place to learn the schema? http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@102 PS3, Line 102: not for {branch} What about making this a bit less likely to be accidentally included by making it less like common prose. Perhaps pre-pending "cherry-pickers: "? http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@102 PS3, Line 102: parser.add_option('--skip_commits_matching', default="not for {branch}", Can you add a note about case-sensitivity? http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@109 PS3, Line 109: cherry_pick_hashes This is a list (not a set or dict-with-empty-values), and it has to be in a certain order, yes? http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@114 PS3, Line 114: script nit: "function" http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@119 PS3, Line 119: print "Cherrypicking %d changes." % (len(cherry_pick_hashes),) nit: move above line 116 for easier tracing by users? http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@126 PS3, Line 126: sys.exit(1) Maybe throw, for easier recovery by the caller? http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@140 PS3, Line 140: logging.basicConfig(level=log_level, format='%(asctime)s %(threadName)s %(levelname)s: %(message)s') nit: long line http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@159 PS3, Line 159: merge_base = sh.git("merge-base", full_source_branch_name, full_target_branch_name).strip() nit: long line http://gerrit.cloudera.org:8080/#/c/9045/3/bin/compare_branches.py@183 PS3, Line 183: # This conditional block just for debug logging of ignored commits Thanks - this makes it easier on me as a reader -- 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 <phi...@cloudera.com> Gerrit-Reviewer: Jim Apple <jbapple-imp...@apache.org> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-Comment-Date: Thu, 18 Jan 2018 06:04:26 +0000 Gerrit-HasComments: Yes
