[release-tools] Tool to find related commits

tools/release/search_related_commits.py

+#!/usr/bin/env python
+# Copyright 2015 the V8 project authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import argparse
+import os
+import sys
+import re
+import operator
+from subprocess import Popen, PIPE
+
+def search_all_related_commits(
+      git_working_dir, of, until, deadline, verbose=False):

[Michael Achenbach, 2015/04/24 12:45:12]

Suggestion: Better name for "of"? It know it sounds fluent as
search_all_related_commits "of", but the single variable "of" in the code flow
is confusing. I'd like something like "revision" better.

[Michael Achenbach, 2015/04/24 12:45:12]

nit: 4 space indentation for params.

[Michael Hablich, 2015/04/27 11:06:07]

Done.

[Michael Hablich, 2015/04/27 11:06:08]

Renamed it inside the function.

+
+  all_commits_raw = (_git_execute(
+      git_working_dir, ["rev-list", "--reverse", of + ".." + until], verbose))
+  if verbose:
+    print "All commits between <of> and <until>: " + all_commits_raw
+
+  all_commits = all_commits_raw.splitlines()
+  all_related_commits = {}
+  already_treated_commits = []
+  for commit in all_commits:
+    if commit in already_treated_commits:
+      continue
+
+    related_commits = (search_related_commits(
+        git_working_dir, commit, until, deadline, verbose))
+    if len(related_commits) > 0:

[Michael Achenbach, 2015/04/24 12:45:12]

nit:
if related_commits:

[Michael Hablich, 2015/04/27 11:06:08]

I like it that way as it is more explicit and better readable.

[Michael Achenbach, 2015/04/27 13:09:43]

Hmm, but not pythonish. Most places in the infrastructure code, we use the
shorter version as
bool(related_commits) implies len(related_commits) > 0

+      all_related_commits[commit] = related_commits
+      already_treated_commits = already_treated_commits + related_commits

[Michael Achenbach, 2015/04/24 12:45:12]

already_treated_commits.extend(related_commits)

[Michael Hablich, 2015/04/27 11:06:07]

Done.

+
+    already_treated_commits.append(commit)
+
+  return all_related_commits
+
+def search_related_commits(git_working_dir, of, until, deadline, verbose=False):
+
+  if deadline != "":

[Michael Achenbach, 2015/04/24 12:45:12]

Better no default for deadline (i.e. default is None) and then:
if deadline:

[Michael Hablich, 2015/04/27 11:06:08]

Done.

+    afterDeadline = False

[Michael Achenbach, 2015/04/24 12:45:12]

unused variable?

[Michael Hablich, 2015/04/27 11:06:08]

Done.

+
+    commits_between = (_git_execute(
+        git_working_dir,
+        ["rev-list", "--reverse", of + ".." + deadline],
+        verbose))
+    if commits_between.strip() == "":
+      return []
+
+  #Extract commit position
+  original_message = _git_execute(git_working_dir,
+      ["show", "-s", "--format=%B", of], verbose)
+  title = original_message.splitlines()[0]
+
+  matches = re.search("(\{#)([0-9]*)(\})", original_message)
+  commit_position = matches.group(2)
+  if verbose:
+    print "1.) Commit position to look for: " + commit_position
+
+  found_by_hash = (_git_execute(

[Michael Achenbach, 2015/04/27 13:09:43]

I don't know for what the outer parentheses are needed? Also in the remainder of
this file...

+      git_working_dir,
+      ["log", "--reverse", of + ".." + until, "--grep=" + of, "--format=%H"],
+      verbose))
+  found_by_hash = found_by_hash.strip()
+
+  if verbose:
+    print "2.) Found by hash: " + found_by_hash
+
+  found_by_commit_pos = (_git_execute(
+      git_working_dir,(
+          ["log", "--reverse",
+          of + ".." + until,
+          "--grep=" + commit_position,
+          "--format=%H"]),
+      verbose))
+
+  found_by_commit_pos = found_by_commit_pos.strip()
+
+  if verbose:
+    print "3.) Found by commit position: " + found_by_commit_pos
+
+  #Replace brackets or else they are wrongly interpreted by --grep
+  title = title.replace("[", "\\[")
+  title = title.replace("]", "\\]")
+
+  found_by_title = (_git_execute(
+      git_working_dir,
+      ["log", "--reverse", of + ".." + until, '--grep=' + title, "--format=%H"],
+      verbose))
+
+  found_by_title = found_by_title.strip()
+
+  if verbose:
+    print "4.) Found by title: " + found_by_title
+
+  hits = (_convert_to_array(found_by_hash)

[Michael Achenbach, 2015/04/24 12:45:12]

nit: Indentation: Either use 4 space indentation or same level, operators on
last line ending:
hits = (
    _convert_to_array(found_by_hash) +
    _convert_to_array(found_by_commit_pos) +
    _convert_to_array(found_by_title)
)
or
hits = (_convert_to_array(found_by_hash) +
        _convert_to_array(found_by_commit_pos) +
        _convert_to_array(found_by_title))

[Michael Hablich, 2015/04/27 11:06:07]

Done.

+      + _convert_to_array(found_by_commit_pos)
+      + _convert_to_array(found_by_title))
+  hits = _remove_duplicates(hits)
+
+  return hits
+
+def _convert_to_array(string_of_hashes):
+  if len(string_of_hashes) == 0:
+    return []
+  return string_of_hashes.splitlines()
+
+def _remove_duplicates(array):
+   no_duplicates = []
+   for current in array:
+    if not current in no_duplicates:
+      no_duplicates.append(current)
+   return no_duplicates
+
+def _git_execute(working_dir, commands, verbose=False):
+
+  fullCommand = ["git", "-C", working_dir] + commands
+  if verbose:
+    print "Git working dir: " + working_dir
+    print "Executing git command:" + str(fullCommand)
+  p = Popen(args=fullCommand, stdin=PIPE,
+      stdout=PIPE, stderr=PIPE)
+  output, err = p.communicate()
+  rc = p.returncode
+  if rc != 0:
+    raise Exception(err)
+  if verbose:
+    print "Git return value: " + output
+  return output
+
+if __name__ == "__main__":  # pragma: no cover
+  parser = argparse.ArgumentParser(
+      ("This tool searches the git repository for "
+       "commits which are related to the commit <of>."))
+  parser.add_argument("-g", "--git-dir", required=False, default=".",
+                        help="The path to your git working directory.")
+  parser.add_argument("--verbose", action="store_true",
+      help="Enables verbose output")
+  parser.add_argument("of", nargs=1,
+      help="Hash of the commit to be searched.")
+  parser.add_argument("until", nargs=1,
+      help="Commit when searching should stop")
+  parser.add_argument("--all", action="store_true",
+      help=("Searches for related commits in all "
+            "commits between <of> and <until>"))
+  parser.add_argument("--deadline", required=False, default="",
+      help=("The script will only list related commits "
+            "which are separated by hash <--deadline>."))
+  parser.add_argument("--prettyprint", action="store_true",
+      help=("Pretty prints the output"))
+
+  args = sys.argv[1:]
+  options = parser.parse_args(args)
+  if options.all:
+    all_related_commits = (search_all_related_commits(options.git_dir,

[Michael Achenbach, 2015/04/24 12:45:12]

nit: Doesn't need the outer parenthesis + params all on one level:
all_related_commits = search_all_related_commits(
    options.git_dir,
    ...

Or indent like this:
all_related_commits = (
    search_all_related_commits(
        options.git_dir,
        ...

[Michael Hablich, 2015/04/27 11:06:07]

Done.

+        options.of[0],
+        options.until[0],
+        options.deadline,
+        options.verbose))
+
+    high_level_commits = sorted(all_related_commits.keys(),key = lambda x:(

[Michael Achenbach, 2015/04/24 12:45:12]

nit:
,space
:space

+        (_git_execute(options.git_dir,

[Michael Achenbach, 2015/04/24 12:45:12]

nit: too many parenthesis

[Michael Achenbach, 2015/04/27 13:09:43]

What about this? Maybe also move the lambda into a local variable for better
readability.

[Michael Achenbach, 2015/04/28 21:23:51]

I still have the feeling this comment was overlooked.

+                      ["show", "--quiet", "--date=iso", x, "--format=%ad"],
+                      options.verbose)).strip()))
+
+    for current_key in high_level_commits:
+      if options.prettyprint:
+        print "+" + (_git_execute(

[Michael Achenbach, 2015/04/24 12:45:12]

Difficult to read the way the parenthesis are set and the way it is indented.
Maybe extract method?

[Michael Hablich, 2015/04/27 11:06:07]

Done.

[Michael Achenbach, 2015/04/27 13:09:43]

Much more readable :)

+            options.git_dir,
+            (
+                ["show",
+                "--quiet",
+                "--date=iso",
+                current_key,
+                "--format=%ad # %H # %s"]),
+            options.verbose)).strip()
+      else:
+        print "+" + current_key
+
+      found_commits = all_related_commits[current_key]
+      for current_commit in found_commits:
+        if options.prettyprint:
+           print "| " + (_git_execute(

[Michael Achenbach, 2015/04/24 12:45:12]

Same here.

[Michael Hablich, 2015/04/27 11:06:08]

Done.

+            options.git_dir,
+            (
+                ["show",
+                "--quiet",
+                "--date=iso",
+                current_commit,
+                "--format=%ad # %H # %s"]),
+            options.verbose)).strip()
+        else:
+          print "| " + current_commit
+  else:
+    hits = search_related_commits(options.git_dir, options.of[0],
+        options.until[0], options.deadline, options.verbose)
+    if len(hits) > 0:
+      print "\n".join(hits)