[autoroller] All commits in updates(), only roll interesting ones.

[autoroller] All commits in updates(), only roll interesting ones.

Previously `updates` would only include the 'interesting' updates, which
meant that the new autoroller algorithm skips over chunks of the repo
history. If multiple repos have chunks of non-interesting history, the
autoroller can end up not proposing any good candidates.

This CL simplifies the repo interface by having `updates()` always
return the full list of commits, but adds a new field to the
CommitMetadata which indicates that a given commit was `interesting`.
This way the full history of every repo is available when picking
candidates (e.g. for `CommitList.advance_to`), but find_best_rev will
only consider rolling to interesting commits.

R=dnj@chromium.org, phajdan.jr@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2833723003
Committed: https://github.com/luci/recipes-py/commit/30925bd08c1f78e835f04a2a3d409cf027cde55e

[dnj, 2017-04-20 18:16:22]

lgtm, small comments

https://codereview.chromium.org/2833723003/diff/1/recipe_engine/autoroll_impl...
File recipe_engine/autoroll_impl/candidate_algorithm.py (right):

https://codereview.chromium.org/2833723003/diff/1/recipe_engine/autoroll_impl...
recipe_engine/autoroll_impl/candidate_algorithm.py:37: 'Interesting' means 'the
commit modifies one or more recipe related files',
nit: use double quotes for consistency

https://codereview.chromium.org/2833723003/diff/1/recipe_engine/autoroll_impl...
File recipe_engine/autoroll_impl/commit_list.py (right):

https://codereview.chromium.org/2833723003/diff/1/recipe_engine/autoroll_impl...
recipe_engine/autoroll_impl/commit_list.py:81: nxt_idx =
self._next_roll_candidate_idx
"nxt_idx" always == self._next_roll_candidate_idx. I think this would be cleaner
if you either used "self._next_roll_candidate_idx" exclusively or used "nxt_idx"
exclusively and set "self._next_roll_candidate_idx" at the end.

https://codereview.chromium.org/2833723003/diff/1/recipe_engine/fetch.py
File recipe_engine/fetch.py (right):

https://codereview.chromium.org/2833723003/diff/1/recipe_engine/fetch.py#newc...
recipe_engine/fetch.py:410: mod_files.add(entry['old_path'])
Not sure if all of these fields are guaranteed to exist for every commit. Maybe
use ".get" to be safe?

https://codereview.chromium.org/2833723003/diff/1/recipe_engine/fetch.py#newc...
recipe_engine/fetch.py:481: raw =
self._fetch_gitiles_json('+/%s?name-status=1&format=JSON', refspec)
Can we document what "name-status=1" does? format=JSON is simple enough, but
this showing the diff tree isn't obvious.

[iannucci, 2017-04-20 21:53:20]

https://codereview.chromium.org/2833723003/diff/1/recipe_engine/autoroll_impl...
File recipe_engine/autoroll_impl/candidate_algorithm.py (right):

https://codereview.chromium.org/2833723003/diff/1/recipe_engine/autoroll_impl...
recipe_engine/autoroll_impl/candidate_algorithm.py:37: 'Interesting' means 'the
commit modifies one or more recipe related files',
On 2017/04/20 18:16:21, dnj wrote:
> nit: use double quotes for consistency

Done.

https://codereview.chromium.org/2833723003/diff/1/recipe_engine/autoroll_impl...
File recipe_engine/autoroll_impl/commit_list.py (right):

https://codereview.chromium.org/2833723003/diff/1/recipe_engine/autoroll_impl...
recipe_engine/autoroll_impl/commit_list.py:81: nxt_idx =
self._next_roll_candidate_idx
On 2017/04/20 18:16:21, dnj wrote:
> "nxt_idx" always == self._next_roll_candidate_idx. I think this would be
cleaner
> if you either used "self._next_roll_candidate_idx" exclusively or used
"nxt_idx"
> exclusively and set "self._next_roll_candidate_idx" at the end.

Done.

https://codereview.chromium.org/2833723003/diff/1/recipe_engine/fetch.py
File recipe_engine/fetch.py (right):

https://codereview.chromium.org/2833723003/diff/1/recipe_engine/fetch.py#newc...
recipe_engine/fetch.py:410: mod_files.add(entry['old_path'])
On 2017/04/20 18:16:21, dnj wrote:
> Not sure if all of these fields are guaranteed to exist for every commit.
Maybe
> use ".get" to be safe?

They are, if there are entries in tree_diff.

https://codereview.chromium.org/2833723003/diff/1/recipe_engine/fetch.py#newc...
recipe_engine/fetch.py:481: raw =
self._fetch_gitiles_json('+/%s?name-status=1&format=JSON', refspec)
On 2017/04/20 18:16:21, dnj wrote:
> Can we document what "name-status=1" does? format=JSON is simple enough, but
> this showing the diff tree isn't obvious.

Done.

[iannucci, 2017-04-20 21:54:06]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2017-04-20 21:54:07]

The patchset sent to the CQ was uploaded after l-g-t-m from dnj@chromium.org
Link to the patchset: https://codereview.chromium.org/2833723003/#ps40001
(title: "fix nits")

[commit-bot: I haz the power, 2017-04-20 21:54:12]

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, 2017-04-20 21:57:44]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492725246578650,
"parent_rev": "de548b9dbccba9e27b9bdb1d5bb619fb786e87a9", "commit_rev":
"30925bd08c1f78e835f04a2a3d409cf027cde55e"}

[commit-bot: I haz the power, 2017-04-20 21:57:48]

Description was changed from

==========
[autoroller] All commits in updates(), only roll interesting ones.

Previously `updates` would only include the 'interesting' updates, which
meant that the new autoroller algorithm skips over chunks of the repo
history. If multiple repos have chunks of non-interesting history, the
autoroller can end up not proposing any good candidates.

This CL simplifies the repo interface by having `updates()` always
return the full list of commits, but adds a new field to the
CommitMetadata which indicates that a given commit was `interesting`.
This way the full history of every repo is available when picking
candidates (e.g. for `CommitList.advance_to`), but find_best_rev will
only consider rolling to interesting commits.

R=dnj@chromium.org, phajdan.jr@chromium.org
BUG=
==========

to

==========
[autoroller] All commits in updates(), only roll interesting ones.

Previously `updates` would only include the 'interesting' updates, which
meant that the new autoroller algorithm skips over chunks of the repo
history. If multiple repos have chunks of non-interesting history, the
autoroller can end up not proposing any good candidates.

This CL simplifies the repo interface by having `updates()` always
return the full list of commits, but adds a new field to the
CommitMetadata which indicates that a given commit was `interesting`.
This way the full history of every repo is available when picking
candidates (e.g. for `CommitList.advance_to`), but find_best_rev will
only consider rolling to interesting commits.

R=dnj@chromium.org, phajdan.jr@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2833723003
Committed:
https://github.com/luci/recipes-py/commit/30925bd08c1f78e835f04a2a3d409cf027c...
==========

[commit-bot: I haz the power, 2017-04-20 21:57:48]

Committed patchset #3 (id:40001) as
https://github.com/luci/recipes-py/commit/30925bd08c1f78e835f04a2a3d409cf027c...

[iannucci1, 2017-04-20 23:03:56]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2835593002/ by iannucci@google.com.

The reason for reverting is: '^!' apparently gets some crappy escape magic on
windows: https://luci-milo.appspot.com/swarming/task/35a662db00607910.