Setup git-svn and handle detached HEAD state in auto-rebaseline script.

Setup git-svn and handle detached HEAD state in auto-rebaseline script.

These enable auto-rebaseline to work from a fresh blink checkout on buildbot.

BUG=474273
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197280

[joelo, 2015-06-12 17:28:17]

joelo@chromium.org changed reviewers:
+ dpranke@chromium.org, jsbell@chromium.org, ojan@chromium.org

[Dirk Pranke, 2015-06-12 19:51:30]

lgtm

https://codereview.chromium.org/1187483002/diff/20001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/tool/commands/rebaseline.py (right):

https://codereview.chromium.org/1187483002/diff/20001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/tool/commands/rebaseline.py:807:
tool.executive.run_command(['git', 'auto-svn'])
I wonder if there's much of a cost to running this every time, and if we should
only run this if svn isn't set up.

[joelo, 2015-06-17 18:37:37]

Sorry for the delay, ptal?

https://codereview.chromium.org/1187483002/diff/20001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/tool/commands/rebaseline.py (right):

https://codereview.chromium.org/1187483002/diff/20001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/tool/commands/rebaseline.py:807:
tool.executive.run_command(['git', 'auto-svn'])
On 2015/06/12 19:51:30, Dirk Pranke wrote:
> I wonder if there's much of a cost to running this every time, and if we
should
> only run this if svn isn't set up.

Good point, done.

[Dirk Pranke, 2015-06-17 18:42:21]

lgtm

[joelo, 2015-06-17 18:45:50]

The CQ bit was checked by joelo@chromium.org

[commit-bot: I haz the power, 2015-06-17 18:46:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1187483002/40001

[commit-bot: I haz the power, 2015-06-17 18:55:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-17 18:55:23]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/59303)

[joelo, 2015-06-17 19:08:09]

The CQ bit was checked by joelo@chromium.org

[commit-bot: I haz the power, 2015-06-17 19:08:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1187483002/40001

[commit-bot: I haz the power, 2015-06-17 20:38:15]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197280

[Xianzhu, 2015-07-14 20:40:04]

wangxianzhu@chromium.org changed reviewers:
+ wangxianzhu@chromium.org

[Xianzhu, 2015-07-14 20:40:05]

https://codereview.chromium.org/1187483002/diff/40001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/tool/commands/rebaseline.py (left):

https://codereview.chromium.org/1187483002/diff/40001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/tool/commands/rebaseline.py:832: old_branch_name =
tool.scm().current_branch()
What will tool.scm().current_branch() return when HEAD is detached? I think we
can just set old_branch_name to 'master' in the case because git will
automatically create 'master' when it doesn't exist.

I think we should also checkout master and set old_branch_name to master if
old_branch_name is self.AUTO_REBASELINE_BRANCH_NAME.

The right-side code also has a problem that old_branch_name has a '\n' in it and
the later checkout will crash because of it.

The following code should work:
  old_branch_name = tool.scm().current_branch()
  if not old_branch_name or old_branch_name == self.AUTO_REBASELINE_BRANCH_NAME:
    old_branch_name = 'master'
    tool.scm().checkout_branch(old_branch_name)

Perhaps we should make the above code a function because we also save/restore
current branch in RebaselineOMatic.

[joelo, 2015-07-14 23:51:01]

https://codereview.chromium.org/1187483002/diff/40001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/tool/commands/rebaseline.py (left):

https://codereview.chromium.org/1187483002/diff/40001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/tool/commands/rebaseline.py:832: old_branch_name =
tool.scm().current_branch()
Is it possible to create a branch with '\n' in it's name??

[Xianzhu, 2015-07-15 15:57:13]

https://codereview.chromium.org/1187483002/diff/40001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/tool/commands/rebaseline.py (left):

https://codereview.chromium.org/1187483002/diff/40001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/tool/commands/rebaseline.py:832: old_branch_name =
tool.scm().current_branch()
On 2015/07/14 23:51:01, joelo wrote:
> Is it possible to create a branch with '\n' in it's name??

Just tried:
$ git checkout -tb 'a
> ' origin/master

fatal: 'a
' is not a valid branch name.

The right side code misses a strip, so will include the ending '\n' of the git
rev-parse command in old_branch_name. I think we should use scm methods if
possible.

BTW, As jsbell@ said, checking out 'master' might not be the best solution.

[Xianzhu, 2015-07-15 15:59:38]

On 2015/07/15 15:57:13, Xianzhu wrote:
>
https://codereview.chromium.org/1187483002/diff/40001/Tools/Scripts/webkitpy/...
> File Tools/Scripts/webkitpy/tool/commands/rebaseline.py (left):
> 
>
https://codereview.chromium.org/1187483002/diff/40001/Tools/Scripts/webkitpy/...
> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:832: old_branch_name =
> tool.scm().current_branch()
> On 2015/07/14 23:51:01, joelo wrote:
> > Is it possible to create a branch with '\n' in it's name??
> 
> Just tried:
> $ git checkout -tb 'a
> > ' origin/master
> 
> fatal: 'a
> ' is not a valid branch name.
> 
> The right side code misses a strip, so will include the ending '\n' of the git
> rev-parse command in old_branch_name. I think we should use scm methods if
> possible.
> 
> BTW, As jsbell@ said, checking out 'master' might not be the best solution.

s/jsbell@/you/ :)

[joelo, 2015-07-15 20:57:09]

> > The right side code misses a strip, so will include the ending '\n' of the
git
> > rev-parse command in old_branch_name. I think we should use scm methods if
> > possible.

Ah, that makes sense. Sent http://crrev.com/1235353002 for review.

tool.scm().current_branch() crashes when HEAD is detached, that caused the
original bug for the change..

[Xianzhu, 2015-07-15 21:12:43]

On 2015/07/15 20:57:09, joelo wrote:
> > > The right side code misses a strip, so will include the ending '\n' of the
> git
> > > rev-parse command in old_branch_name. I think we should use scm methods if
> > > possible.
> 
> Ah, that makes sense. Sent http://crrev.com/1235353002 for review.
> 
> tool.scm().current_branch() crashes when HEAD is detached, that caused the
> original bug for the change..

I don't think it crashes in current_branch(), but crashes somewhere else. I
think it just returns an empty string.

[joelo, 2015-07-16 07:04:41]

> I don't think it crashes in current_branch(), but crashes somewhere else. I
> think it just returns an empty string.

It returns a non-zero exit code which causes a crash -- our scm tools use
executive.run_command which checks the exit code and throws an error:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Xianzhu, 2015-07-16 16:04:42]

On 2015/07/16 07:04:41, joelo wrote:
> > I don't think it crashes in current_branch(), but crashes somewhere else. I
> > think it just returns an empty string.
> 
> It returns a non-zero exit code which causes a crash -- our scm tools use
> executive.run_command which checks the exit code and throws an error:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Tested again proving you are right :) I did test this before saying #19 but I
must have done sth wrong.

However, I still think we should fix git.py instead of duplicating the logic
here (for https://codereview.chromium.org/1235353002/).

[joelo, 2015-07-16 20:18:37]

> However, I still think we should fix git.py instead of duplicating the logic
> here (for https://codereview.chromium.org/1235353002/).

Will do, thanks for investigating ;)