New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 677224 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 644372



Sign in to add a comment

[Predator] Remove mutation from ``get_repository`` factory functions

Project Member Reported by wrengr@chromium.org, Dec 28 2016

Issue description

In CL https://codereview.chromium.org/2608483002/ we reorganize the FindSuspects function to take a factory function ``get_repository``. This fixes half the problem discussed there: with that CL, the FindSuspects function does not depend on the ``repository`` object having a settable ``repo_url`` property (as it shouldn't, since the ``Repository`` interface doesn't guarantee the existence of such a property).

There remains the other half of the problem: mutating the main repository object in place can have unforeseen consequences. As of that CL, the actual functions we pass to FindSuspects still mutate the main repo object rather than generating a new one. That CL doesn't try to solve this half of the problem because doing so would make for an overly large CL.

A future CL should get rid of the mutation and pass actual factory functions to FindSuspects, eliminating the worry about unexpected side-effects of classifying CLs. Of particular note, the factory functions will have to work appropriately for both the AppEngine code (which uses the GitilesRepository implementation of Repository) and for delta-testing code (which uses the LocalGitRepository implementation).
 

Comment 1 by wrengr@chromium.org, Dec 28 2016

Owner: wrengr@chromium.org
Status: Started (was: Available)
CL for fixing this is at https://codereview.chromium.org/2605943002
Project Member

Comment 2 by bugdroid1@chromium.org, Jan 3 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra.git/+/49521996b621b7cdb4c070f15b171c38630f6f34

commit 49521996b621b7cdb4c070f15b171c38630f6f34
Author: wrengr <wrengr@chromium.org>
Date: Tue Jan 03 20:16:13 2017

Removing the mutation in the factories for getting dep repositories

This CL also removes passing the main repository as an argument, since
it was never actually used for the main repo it was only ever used as
an ad-hoc factory. This CL addresses the first issues described in
https://crrev.com/2608483002

BUG= 677224 
TBR=stgao@chromium.org

Review-Url: https://codereview.chromium.org/2605943002

[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/common/chrome_dependency_fetcher.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/common/test/chrome_dependency_fetcher_test.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/crash/changelist_classifier.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/crash/crash_pipeline.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/crash/findit.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/crash/findit_for_chromecrash.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/crash/findit_for_clusterfuzz.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/crash/loglinear/changelist_classifier.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/crash/loglinear/test/changelist_classifier_test.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/crash/test/changelist_classifier_test.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/crash/test/findit_for_chromecrash_test.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/crash/test/findit_test.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/handlers/crash/crash_handler.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/handlers/crash/test/crash_handler_test.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/libs/gitiles/gitiles_repository.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/libs/gitiles/test/gitiles_repository_test.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/util_scripts/crash_queries/delta_test/delta_util.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/util_scripts/crash_queries/delta_test/run-delta-test.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/util_scripts/git_checkout/local_git_repository.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/util_scripts/git_checkout/test/local_git_parsers_test.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/util_scripts/git_checkout/test/local_git_repository_test.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/waterfall/extract_deps_info_pipeline.py

Project Member

Comment 3 by bugdroid1@chromium.org, Jan 3 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra.git/+/49521996b621b7cdb4c070f15b171c38630f6f34

commit 49521996b621b7cdb4c070f15b171c38630f6f34
Author: wrengr <wrengr@chromium.org>
Date: Tue Jan 03 20:16:13 2017

Removing the mutation in the factories for getting dep repositories

This CL also removes passing the main repository as an argument, since
it was never actually used for the main repo it was only ever used as
an ad-hoc factory. This CL addresses the first issues described in
https://crrev.com/2608483002

BUG= 677224 
TBR=stgao@chromium.org

Review-Url: https://codereview.chromium.org/2605943002

[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/common/chrome_dependency_fetcher.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/common/test/chrome_dependency_fetcher_test.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/crash/changelist_classifier.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/crash/crash_pipeline.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/crash/findit.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/crash/findit_for_chromecrash.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/crash/findit_for_clusterfuzz.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/crash/loglinear/changelist_classifier.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/crash/loglinear/test/changelist_classifier_test.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/crash/test/changelist_classifier_test.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/crash/test/findit_for_chromecrash_test.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/crash/test/findit_test.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/handlers/crash/crash_handler.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/handlers/crash/test/crash_handler_test.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/libs/gitiles/gitiles_repository.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/libs/gitiles/test/gitiles_repository_test.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/util_scripts/crash_queries/delta_test/delta_util.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/util_scripts/crash_queries/delta_test/run-delta-test.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/util_scripts/git_checkout/local_git_repository.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/util_scripts/git_checkout/test/local_git_parsers_test.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/util_scripts/git_checkout/test/local_git_repository_test.py
[modify] https://crrev.com/49521996b621b7cdb4c070f15b171c38630f6f34/appengine/findit/waterfall/extract_deps_info_pipeline.py

Status: Fixed (was: Started)

Sign in to add a comment