Assume 'master' when remote branch not specified.

Assume 'master' when remote branch not specified.

This prevents apply_patch from attempting to check out a branch tracking 'origin/None'.

R=pgervais@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=257763

[pgervais, 2014-03-18 15:28:23]

On 2014/03/18 01:27:38, agable wrote:

lgtm, but I do not fully understand the consequences if this.

[agable, 2014-03-18 17:15:19]

The CQ bit was checked by agable@chromium.org

[commit-bot: I haz the power, 2014-03-18 17:15:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agable@chromium.org/202483008/1

[commit-bot: I haz the power, 2014-03-18 17:15:35]

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

[commit-bot: I haz the power, 2014-03-18 17:15:37]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[agable, 2014-03-18 17:22:12]

On 2014/03/18 17:15:37, I haz the power (commit-bot) wrote:
> No LGTM from a valid reviewer yet. Only full committers are accepted.
> Even if an LGTM may have been provided, it was from a non-committer or
> a lowly provisional committer, _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

maruel@, please take a look.

[pgervais, 2014-03-18 17:55:06]

This patch breaks if there is an existing local 'master' branch (and it's
not up-to-date).

apply_issue.py creates a 'working_branch' based on 'master', then checks
that it worked properly by diffing origin/master and working_branch.
In addition, if working_branch already exists, it fails. That makes sense
for a human, but screws up the bots if the branch is a leftover from a
previous failure.


On Tue, Mar 18, 2014 at 10:22 AM, <agable@chromium.org> wrote:

> On 2014/03/18 17:15:37, I haz the power (commit-bot) wrote:
>
>> No LGTM from a valid reviewer yet. Only full committers are accepted.
>> Even if an LGTM may have been provided, it was from a non-committer or
>> a lowly provisional committer, _not_ a full super star committer.
>> See http://www.chromium.org/getting-involved/become-a-committer
>> Note that this has nothing to do with OWNERS files.
>>
>
> maruel@, please take a look.
>
> https://codereview.chromium.org/202483008/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[M-A Ruel, 2014-03-18 19:14:40]

On 2014/03/18 17:55:06, pgervais wrote:
> This patch breaks if there is an existing local 'master' branch (and it's
> not up-to-date).
> 
> apply_issue.py creates a 'working_branch' based on 'master', then checks
> that it worked properly by diffing origin/master and working_branch.
> In addition, if working_branch already exists, it fails. That makes sense
> for a human, but screws up the bots if the branch is a leftover from a
> previous failure.

I prefer "assert remote_branch" than defaulting to "master". E.g. fix the
callers instead of papering over.

In practice, the code should checkout from origin/master, not master.

[agable, 2014-03-18 20:44:49]

On 2014/03/18 19:14:40, M-A Ruel wrote:
> On 2014/03/18 17:55:06, pgervais wrote:
> > This patch breaks if there is an existing local 'master' branch (and it's
> > not up-to-date).
> > 
> > apply_issue.py creates a 'working_branch' based on 'master', then checks
> > that it worked properly by diffing origin/master and working_branch.
> > In addition, if working_branch already exists, it fails. That makes sense
> > for a human, but screws up the bots if the branch is a leftover from a
> > previous failure.
> 
> I prefer "assert remote_branch" than defaulting to "master". E.g. fix the
> callers instead of papering over.
> 
> In practice, the code should checkout from origin/master, not master.

I agree -- apply_issue.py passes 'None' to this method. Will send a CL to that
effect now.

[agable, 2014-03-18 21:18:15]

On 2014/03/18 20:44:49, agable wrote:
> On 2014/03/18 19:14:40, M-A Ruel wrote:
> > On 2014/03/18 17:55:06, pgervais wrote:
> > > This patch breaks if there is an existing local 'master' branch (and it's
> > > not up-to-date).
> > > 
> > > apply_issue.py creates a 'working_branch' based on 'master', then checks
> > > that it worked properly by diffing origin/master and working_branch.
> > > In addition, if working_branch already exists, it fails. That makes sense
> > > for a human, but screws up the bots if the branch is a leftover from a
> > > previous failure.
> > 
> > I prefer "assert remote_branch" than defaulting to "master". E.g. fix the
> > callers instead of papering over.
> > 
> > In practice, the code should checkout from origin/master, not master.
> 
> I agree -- apply_issue.py passes 'None' to this method. Will send a CL to that
> effect now.

PTAL, this patch will resolve the issues being seen by Ravi on the skiabuildbot
CQ.

[agable, 2014-03-18 21:19:08]

https://codereview.chromium.org/202483008/diff/20001/checkout.py
File checkout.py (right):

https://codereview.chromium.org/202483008/diff/20001/checkout.py#newcode630
checkout.py:630: '-t', '%s/%s' % (self.pull_remote, self.remote_branch),
Ravi: apply_issue was checking out a branch tracking local master rather than
origin/master, which is why the skiabuildbot CQ was failing.

[rmistry, 2014-03-18 21:22:15]

LGTM

[cmp, 2014-03-18 21:24:32]

lgtm

[agable, 2014-03-18 21:24:57]

The CQ bit was checked by agable@chromium.org

[commit-bot: I haz the power, 2014-03-18 21:25:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agable@chromium.org/202483008/20001

[commit-bot: I haz the power, 2014-03-18 21:28:04]

Change committed as 257763

[Ryan Tseng, 2014-03-19 01:52:57]

This actually broke bot_update on linux_clang (or well, elsewhere too) for cases
that a revision is specified because if you specify a remote_branch, GitCheckout
tries to check out a "working_branch" tracking master instead of committing on
top of HEAD.

[Ryan Tseng, 2014-03-19 01:54:28]

https://codereview.chromium.org/202483008/diff/20001/checkout.py
File checkout.py (right):

https://codereview.chromium.org/202483008/diff/20001/checkout.py#newcode626
checkout.py:626: if self.remote_branch:
Maybe "if self.remote_branch and not self.base_ref"?

Since if we have a base_ref set, thats actually the ref we want to base this
patch on top of, rather than the remote branch.

We can't use remote branch because a ref isn't always a branch.