add_codereview_message script to append messages to a CL

add_codereview_message script to append messages to a CL

Add add_codereview_message.py script.  This script takes a message and
a codereview URL as arguments calls `git cl upload` to append the
message to the given codereview issue.

Motivation: We are automating a process of testing Chromium/Skia DEPS
rolls where roll_deps.py is automatically run every night, and then at
a later time, compare_codereview.py is run to generate a report of the
trybot results.  This script can be used to append that report to the
DEPS roll Cl.

This CL also refactors functioanlity common to roll_deps and
add_codereview_message:
*   Add tools/git_utils.py module.
    -   ChangeGitBranch class was factored out of
        roll_deps.GitBranchCLUpload.  The other half of that class
        became roll_deps.git_cl_uploader function.  I make use of
        this new class in both roll_deps and upload_deps_roll.
    -   test_git_executable moved to this new module.
*   Add tools/misc_utils.py - move VerboseSubprocess, ChangeDir,
    and ReSearch classes from roll_deps module.
*   Remove generic functions from roll_deps.
*   Add git_cl_uploader function to roll_deps.  Refactor
    roll_deps.roll_deps function to make use of it

BUG=skia:

Committed: http://code.google.com/p/skia/source/detail?r=13251

[hal.canary, 2014-01-29 17:04:30]

Rob,  here's the script you asked for last week.  Please make sure it works for
you and that I didn't break anything in my refactoring.

Eric, ptal.  Do we want to combine this with compare_codereview.py?

[robertphillips, 2014-01-30 14:04:40]

Sorry - I forgot to use this last night and just used the auto-roll-bot's
results. I will exercise this tonight.

[borenet, 2014-01-30 14:36:35]

https://codereview.chromium.org/141483011/diff/20001/tools/add_codereview_mes...
File tools/add_codereview_message.py (right):

https://codereview.chromium.org/141483011/diff/20001/tools/add_codereview_mes...
tools/add_codereview_message.py:43: """Function that does the meat of the work.
This docstring is not informative...

https://codereview.chromium.org/141483011/diff/20001/tools/add_codereview_mes...
tools/add_codereview_message.py:47: this url.
Why not just take an issue number?  Asking for the full URL requires the caller
to determine which codereview server is in use, build the URL, etc, and we
aren't even using it here.

https://codereview.chromium.org/141483011/diff/20001/tools/add_codereview_mes...
tools/add_codereview_message.py:56: carbon-copied
I'd prefer having these as separate arguments. I think it's much clearer that
way (eg. you use options.chromium_path below, but it's not listed here), and it
doesn't force the caller to create an options object.

https://codereview.chromium.org/141483011/diff/20001/tools/add_codereview_mes...
tools/add_codereview_message.py:104: ])
I'd prefer these global variables declared before any functions.

https://codereview.chromium.org/141483011/diff/20001/tools/add_codereview_mes...
tools/add_codereview_message.py:119: ' variable is set.')
Personally, I think that this script could be useful outside of the DEPS roll
process and therefore would like to see this option changed to "checkout_path"
or something, and maybe default to CWD.

https://codereview.chromium.org/141483011/diff/20001/tools/add_codereview_mes...
tools/add_codereview_message.py:128: help='Git executable, defaults to "git".',)
Now that we have a git_utils module, we could add a "find_git" function which
does something like:

for git in ('git', 'git.exe', 'git.bat'):
  if test_git_executable(git):
    return git
raise Exception(...)

It could even take a "hint" parameter for those who have git in some weird place
not in PATH, although I think it makes sense not to care about that case.

https://codereview.chromium.org/141483011/diff/20001/tools/git_utils.py
File tools/git_utils.py (right):

https://codereview.chromium.org/141483011/diff/20001/tools/git_utils.py#newco...
tools/git_utils.py:33: brach_name: (string) if not None, the name of the branch
to
"branch_name".

Also, please document that this exact name might not be used if the branch
already exists.

https://codereview.chromium.org/141483011/diff/20001/tools/git_utils.py#newco...
tools/git_utils.py:37: commit to branch from.  If None, then use origin/master
I think upstream_branch or tracking_branch are better names for this.

[hal.canary, 2014-01-30 20:11:02]

https://codereview.chromium.org/141483011/diff/20001/tools/add_codereview_mes...
File tools/add_codereview_message.py (right):

https://codereview.chromium.org/141483011/diff/20001/tools/add_codereview_mes...
tools/add_codereview_message.py:43: """Function that does the meat of the work.
On 2014/01/30 14:36:35, borenet wrote:
> This docstring is not informative...

Done.

https://codereview.chromium.org/141483011/diff/20001/tools/add_codereview_mes...
tools/add_codereview_message.py:47: this url.
On 2014/01/30 14:36:35, borenet wrote:
> Why not just take an issue number?  Asking for the full URL requires the
caller
> to determine which codereview server is in use, build the URL, etc, and we
> aren't even using it here.

As the code is written, it will take either.  I'll document that.

https://codereview.chromium.org/141483011/diff/20001/tools/add_codereview_mes...
tools/add_codereview_message.py:56: carbon-copied
On 2014/01/30 14:36:35, borenet wrote:
> I'd prefer having these as separate arguments. I think it's much clearer that
> way (eg. you use options.chromium_path below, but it's not listed here), and
it
> doesn't force the caller to create an options object.

Done.

https://codereview.chromium.org/141483011/diff/20001/tools/add_codereview_mes...
tools/add_codereview_message.py:104: ])
On 2014/01/30 14:36:35, borenet wrote:
> I'd prefer these global variables declared before any functions.

Done.

https://codereview.chromium.org/141483011/diff/20001/tools/add_codereview_mes...
tools/add_codereview_message.py:119: ' variable is set.')
On 2014/01/30 14:36:35, borenet wrote:
> Personally, I think that this script could be useful outside of the DEPS roll
> process and therefore would like to see this option changed to "checkout_path"
> or something, and maybe default to CWD.

Done.

https://codereview.chromium.org/141483011/diff/20001/tools/add_codereview_mes...
tools/add_codereview_message.py:128: help='Git executable, defaults to "git".',)
On 2014/01/30 14:36:35, borenet wrote:
> Now that we have a git_utils module, we could add a "find_git" function which
> does something like:
> 
> for git in ('git', 'git.exe', 'git.bat'):
>   if test_git_executable(git):
>     return git
> raise Exception(...)
> 
> It could even take a "hint" parameter for those who have git in some weird
place
> not in PATH, although I think it makes sense not to care about that case.

Done.  git_utils.git_executable()

https://codereview.chromium.org/141483011/diff/20001/tools/git_utils.py
File tools/git_utils.py (right):

https://codereview.chromium.org/141483011/diff/20001/tools/git_utils.py#newco...
tools/git_utils.py:33: brach_name: (string) if not None, the name of the branch
to
On 2014/01/30 14:36:35, borenet wrote:
> "branch_name".
> 
> Also, please document that this exact name might not be used if the branch
> already exists.

Done.

https://codereview.chromium.org/141483011/diff/20001/tools/git_utils.py#newco...
tools/git_utils.py:37: commit to branch from.  If None, then use origin/master
On 2014/01/30 14:36:35, borenet wrote:
> I think upstream_branch or tracking_branch are better names for this.

Done.

[borenet, 2014-01-30 21:25:26]

LGTM. Thanks for the fixes.

[commit-bot: I haz the power, 2014-01-30 21:26:46]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/halcanary@google.com/141483011/40001

[commit-bot: I haz the power, 2014-01-30 22:12:35]

Change committed as 13251

[commit-bot: I haz the power, 2014-01-30 22:13:19]

CQ bit was unchecked on CL. Ignoring.