Do not exclude the last revision in the range returned by gitiles.

Do not exclude the last revision in the range returned by gitiles.

Originally we excluded the last revision in the range, because that
operation included the revision given as the end. When expanding the
revision range the 'bad' revision is the end revision, and we don't want
to instantiate it again, since we already do that when starting the
bisect jobs. When reusing this code for expanding rolls, we ended up
also discarding the end revision, which makes for one-revision rolls to
break and also excludes the last revision in the roll.

BUG=641133
R=prasadv,dtu,sullivan

Committed: https://chromium.googlesource.com/chromium/tools/build/+/59a238ef7b257e8a6bbe1583774cc2d84b4ced72

[sullivan, 2016-08-25 23:26:56]

lgtm

https://codereview.chromium.org/2282563004/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/auto_bisect/bisector.py (right):

https://codereview.chromium.org/2282563004/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/bisector.py:397: A list of
RevisionState objects, not including the given start or end.
Need to add documentation for exclude_end argument here. Please explain why the
different callers do/don't need to exclude the end rev in the documentation.

[dtu, 2016-08-26 00:52:11]

Oh, I totally thought this was intentional, because the last revision in the
roll is theoretically the same as the chromium revision with the entire roll in
it.

[dtu, 2016-08-26 01:16:56]

https://codereview.chromium.org/2282563004/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/auto_bisect/bisector.py (right):

https://codereview.chromium.org/2282563004/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/bisector.py:386: step_name=None,
exclude_end=False):
The skeleton has a similar parameter, but phrased in the opposite way,
"include_first_commit." I think either way makes sense in the context of this
particular code, though.

https://codereview.chromium.org/2282563004/diff/1/scripts/slave/recipe_module...
File
scripts/slave/recipe_modules/auto_bisect/resources/fetch_intervening_revisions.py
(right):

https://codereview.chromium.org/2282563004/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/resources/fetch_intervening_revisions.py:44:
A list of pairs (commit hash, commit position), from earliest to latest,
This comment needs to be updated.

[RobertoCN, 2016-08-26 18:17:35]

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

[RobertoCN, 2016-08-26 18:17:36]

Done.

https://codereview.chromium.org/2282563004/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/auto_bisect/bisector.py (right):

https://codereview.chromium.org/2282563004/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/bisector.py:386: step_name=None,
exclude_end=False):
On 2016/08/26 01:16:56, dtu wrote:
> The skeleton has a similar parameter, but phrased in the opposite way,
> "include_first_commit." I think either way makes sense in the context of this
> particular code, though.

Acknowledged.

https://codereview.chromium.org/2282563004/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/bisector.py:397: A list of
RevisionState objects, not including the given start or end.
On 2016/08/25 23:26:55, sullivan wrote:
> Need to add documentation for exclude_end argument here. Please explain why
the
> different callers do/don't need to exclude the end rev in the documentation.

Done.

https://codereview.chromium.org/2282563004/diff/1/scripts/slave/recipe_module...
File
scripts/slave/recipe_modules/auto_bisect/resources/fetch_intervening_revisions.py
(right):

https://codereview.chromium.org/2282563004/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/resources/fetch_intervening_revisions.py:44:
A list of pairs (commit hash, commit position), from earliest to latest,
On 2016/08/26 01:16:56, dtu wrote:
> This comment needs to be updated.

Done.

[prasadv, 2016-08-29 18:04:22]

lgtm with nits

https://codereview.chromium.org/2282563004/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/bisector.py (right):

https://codereview.chromium.org/2282563004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:401: exclude_end (boold):
Whether to exclude the last revision in the range,
boold-> bool

https://codereview.chromium.org/2282563004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:413:
self.api.resource('fetch_intervening_revisions.py'),
I think we should replace this to use gitiles recipe module.
This can be done in separate CL but, I think we should add TODO to track this.

[RobertoCN, 2016-08-29 18:14:13]

https://codereview.chromium.org/2282563004/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/bisector.py (right):

https://codereview.chromium.org/2282563004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:401: exclude_end (boold):
Whether to exclude the last revision in the range,
On 2016/08/29 18:04:22, prasadv wrote:
> boold-> bool

Done.

https://codereview.chromium.org/2282563004/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector.py:413:
self.api.resource('fetch_intervening_revisions.py'),
On 2016/08/29 18:04:22, prasadv wrote:
> I think we should replace this to use gitiles recipe module.
> This can be done in separate CL but, I think we should add TODO to track this.

File a bug: https://bugs.chromium.org/p/chromium/issues/detail?id=642043
TODOs tend to take a lot longer to get addressed than bugs.

[RobertoCN, 2016-08-29 18:14:46]

The CQ bit was checked by robertocn@chromium.org

[RobertoCN, 2016-08-29 18:14:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
sullivan@chromium.org, prasadv@chromium.org
Link to the patchset: https://codereview.chromium.org/2282563004/#ps40001
(title: "typo")

[commit-bot: I haz the power, 2016-08-29 18:14:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-29 18:23:06]

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

[commit-bot: I haz the power, 2016-08-29 18:23:08]

Try jobs failed on following builders:
  Build Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/30f05490c445a310)

[sullivan, 2016-08-29 18:26:43]

Why don't Roberto/Prasad have OWNERS for these files?

Missing LGTM from an OWNER for these files:
    scripts/slave/recipes/bisect.expected/basic.json
    scripts/slave/recipes/bisect.expected/basic_return_code_test.json
    scripts/slave/recipes/bisect.expected/broken_bad_revision_test.json
    scripts/slave/recipes/bisect.expected/broken_good_revision_test.json

[RobertoCN, 2016-08-29 18:44:24]

On 2016/08/29 18:26:43, sullivan wrote:
> Why don't Roberto/Prasad have OWNERS for these files?
> 
> Missing LGTM from an OWNER for these files:
>     scripts/slave/recipes/bisect.expected/basic.json
>     scripts/slave/recipes/bisect.expected/basic_return_code_test.json
>     scripts/slave/recipes/bisect.expected/broken_bad_revision_test.json
>     scripts/slave/recipes/bisect.expected/broken_good_revision_test.json

Oh, it seems I uploaded this CL with my google account instead of my chromium
one. LGTM

[dtu, 2016-08-29 18:44:26]

lgtm fwiw

[RobertoCN, 2016-08-29 18:44:31]

The CQ bit was checked by robertocn@chromium.org

[commit-bot: I haz the power, 2016-08-29 18:44:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-29 18:50:36]

Description was changed from

==========
Do not exclude the last revision in the range returned by gitiles.

Originally we excluded the last revision in the range, because that
operation included the revision given as the end. When expanding the
revision range the 'bad' revision is the end revision, and we don't want
to instantiate it again, since we already do that when starting the
bisect jobs. When reusing this code for expanding rolls, we ended up
also discarding the end revision, which makes for one-revision rolls to
break and also excludes the last revision in the roll.

BUG=641133
R=prasadv,dtu,sullivan
==========

to

==========
Do not exclude the last revision in the range returned by gitiles.

Originally we excluded the last revision in the range, because that
operation included the revision given as the end. When expanding the
revision range the 'bad' revision is the end revision, and we don't want
to instantiate it again, since we already do that when starting the
bisect jobs. When reusing this code for expanding rolls, we ended up
also discarding the end revision, which makes for one-revision rolls to
break and also excludes the last revision in the roll.

BUG=641133
R=prasadv,dtu,sullivan

Committed:
https://chromium.googlesource.com/chromium/tools/build/+/59a238ef7b257e8a6bbe...
==========

[commit-bot: I haz the power, 2016-08-29 18:50:37]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/tools/build/+/59a238ef7b257e8a6bbe...