Summary

Provide diff metadata and more to Repository.get_file()/get_file_exists().

Review Request #11805 — Created Sept. 1, 2021 and submitted Oct. 4, 2021, 7:13 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Originally, repository file lookups required a path within the
repository and a revision, which was fine for a lot of SCMs but wasn't
sufficient for others. Later, we added base_commit_id, a somewhat
loosely-defined identifier that represented some commit that the helped
locate the files on some SCMs/hosting services.

While working to integrate with another new SCM, we've found that these
three identifiers really aren't enough, and we need other information
parsed in a diff, made available through the DiffX parser and stored in
DiffSet.extra_data and FileDiff.extra_data. However, there was no
way of getting access to this information, and bolting on several new
parameters to get_file()/get_file_exists() (and SCM and hosting
service backends) seemed like an unsustainable approach.

This change introduces a new FileLookupContext object, which is a
container for contextual information that might be useful for file
lookups. It effectively replaces the base_commit_id and request
on the Repository functions, and adds extra_data attributes for the
FileDiff, DiffCommit, and DiffSet (or the Parsed* equivalents
during diff parsing/validation).

This gives implementations access to any metadata parsed from the diff,
along with the user accessing the repository, the active HTTP request,
and the base commit ID. In the future, we'll be able to add new context
(or even functions for permanently caching computed lookup state from
these functions) without having to funnel new parameters everywhere.

base_commit_id and request are now soft-deprecated. The Repository
functions still accept them and will pass them on, and the SCMTool
implementations still receive them. We'll probably want to include
deprecation warnings in a future major version.

Testing Done

Unit tests pass.

This is pending extensive testing with the development of a new SCM
integration.

Commits

Summary	ID
Provide diff metadata and more to Repository.get_file()/get_file_exists(). Originally, repository file lookups required a path within the repository and a revision, which was fine for a lot of SCMs but wasn't sufficient for others. Later, we added `base_commit_id`, a somewhat loosely-defined identifier that represented some commit that the helped locate the files on some SCMs/hosting services. While working to integrate with another new SCM, we've found that these three identifiers really aren't enough, and we need other information parsed in a diff, made available through the DiffX parser and stored in `DiffSet.extra_data` and `FileDiff.extra_data`. However, there was no way of getting access to this information, and bolting on several new parameters to `get_file()`/`get_file_exists()` (and SCM and hosting service backends) seemed like an unsustainable approach. This change introduces a new `FileLookupContext` object, which is a container for contextual information that might be useful for file lookups. It effectively replaces the `base_commit_id` and `request` on the `Repository` functions, and adds `extra_data` attributes for the `FileDiff`, `DiffCommit`, and `DiffSet` (or the `Parsed*` equivalents during diff parsing/validation). This gives implementations access to any metadata parsed from the diff, along with the user accessing the repository, the active HTTP request, and the base commit ID. In the future, we'll be able to add new context (or even functions for permanently caching computed lookup state from these functions) without having to funnel new parameters everywhere. `base_commit_id` and `request` are now soft-deprecated. The `Repository` functions still accept them and will pass them on, and the `SCMTool` implementations still receive them. We'll probably want to include deprecation warnings in a future major version.	c3df973942b43b0c344c347bc2d5c6e1edc82a07

Summary

Provide diff metadata and more to Repository.get_file()/get_file_exists().

Originally, repository file lookups required a path within the repository and a revision, which was fine for a lot of SCMs but wasn't sufficient for others. Later, we added `base_commit_id`, a somewhat loosely-defined identifier that represented some commit that the helped locate the files on some SCMs/hosting services. While working to integrate with another new SCM, we've found that these three identifiers really aren't enough, and we need other information parsed in a diff, made available through the DiffX parser and stored in `DiffSet.extra_data` and `FileDiff.extra_data`. However, there was no way of getting access to this information, and bolting on several new parameters to `get_file()`/`get_file_exists()` (and SCM and hosting service backends) seemed like an unsustainable approach. This change introduces a new `FileLookupContext` object, which is a container for contextual information that might be useful for file lookups. It effectively replaces the `base_commit_id` and `request` on the `Repository` functions, and adds `extra_data` attributes for the `FileDiff`, `DiffCommit`, and `DiffSet` (or the `Parsed*` equivalents during diff parsing/validation). This gives implementations access to any metadata parsed from the diff, along with the user accessing the repository, the active HTTP request, and the base commit ID. In the future, we'll be able to add new context (or even functions for permanently caching computed lookup state from these functions) without having to funnel new parameters everywhere. `base_commit_id` and `request` are now soft-deprecated. The `Repository` functions still accept them and will pass them on, and the `SCMTool` implementations still receive them. We'll probably want to include deprecation warnings in a future major version.

c3df973942b43b0c344c347bc2d5c6e1edc82a07

Issues

Description	From	Last Updated
F811 redefinition of unused 'test_with_filediff_with_repository_encoding_set' from line 3799	reviewbot	Sept. 1, 2021, 11:47 p.m.
F401 'json' imported but unused	reviewbot	Sept. 1, 2021, 11:47 p.m.
F401 'reviewboard.scmtools.core.HEAD' imported but unused	reviewbot	Sept. 1, 2021, 11:47 p.m.
TODO is more appropriate than FIXME I think	david	Oct. 4, 2021, 7:13 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 1)

Show all issues

F811 redefinition of unused 'test_with_filediff_with_repository_encoding_set' from line 3799

reviewboard/scmtools/models.py (Diff revision 1)
Show all issues
```
F401 'json' imported but unused
```
reviewboard/scmtools/tests/test_repository.py (Diff revision 1)
Show all issues
```
F401 'reviewboard.scmtools.core.HEAD' imported but unused
```

Change Summary:


Removed unused imports.
Fixed a duplicate test name.

Commits:

	Summary	ID
	Provide diff metadata and more to Repository.get_file()/get_file_exists(). Originally, repository file lookups required a path within the repository and a revision, which was fine for a lot of SCMs but wasn't sufficient for others. Later, we added `base_commit_id`, a somewhat loosely-defined identifier that represented some commit that the helped locate the files on some SCMs/hosting services. While working to integrate with another new SCM, we've found that these three identifiers really aren't enough, and we need other information parsed in a diff, made available through the DiffX parser and stored in `DiffSet.extra_data` and `FileDiff.extra_data`. However, there was no way of getting access to this information, and bolting on several new parameters to `get_file()`/`get_file_exists()` (and SCM and hosting service backends) seemed like an unsustainable approach. This change introduces a new `FileLookupContext` object, which is a container for contextual information that might be useful for file lookups. It effectively replaces the `base_commit_id` and `request` on the `Repository` functions, and adds `extra_data` attributes for the `FileDiff`, `DiffCommit`, and `DiffSet` (or the `Parsed*` equivalents during diff parsing/validation). This gives implementations access to any metadata parsed from the diff, along with the user accessing the repository, the active HTTP request, and the base commit ID. In the future, we'll be able to add new context (or even functions for permanently caching computed lookup state from these functions) without having to funnel new parameters everywhere. `base_commit_id` and `request` are now soft-deprecated. The `Repository` functions still accept them and will pass them on, and the `SCMTool` implementations still receive them. We'll probably want to include deprecation warnings in a future major version.	8cffd344d860641993c91751cfffeba23c51a7d4
	Provide diff metadata and more to Repository.get_file()/get_file_exists(). Originally, repository file lookups required a path within the repository and a revision, which was fine for a lot of SCMs but wasn't sufficient for others. Later, we added `base_commit_id`, a somewhat loosely-defined identifier that represented some commit that the helped locate the files on some SCMs/hosting services. While working to integrate with another new SCM, we've found that these three identifiers really aren't enough, and we need other information parsed in a diff, made available through the DiffX parser and stored in `DiffSet.extra_data` and `FileDiff.extra_data`. However, there was no way of getting access to this information, and bolting on several new parameters to `get_file()`/`get_file_exists()` (and SCM and hosting service backends) seemed like an unsustainable approach. This change introduces a new `FileLookupContext` object, which is a container for contextual information that might be useful for file lookups. It effectively replaces the `base_commit_id` and `request` on the `Repository` functions, and adds `extra_data` attributes for the `FileDiff`, `DiffCommit`, and `DiffSet` (or the `Parsed*` equivalents during diff parsing/validation). This gives implementations access to any metadata parsed from the diff, along with the user accessing the repository, the active HTTP request, and the base commit ID. In the future, we'll be able to add new context (or even functions for permanently caching computed lookup state from these functions) without having to funnel new parameters everywhere. `base_commit_id` and `request` are now soft-deprecated. The `Repository` functions still accept them and will pass them on, and the `SCMTool` implementations still receive them. We'll probably want to include deprecation warnings in a future major version.	c3df973942b43b0c344c347bc2d5c6e1edc82a07

Diff:

Revision 2 (+1756 -426)

Show changes

	reviewboard/diffviewer/commit_utils.py
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/filediff_creator.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/tests/test_diffcommit_manager.py
	reviewboard/diffviewer/tests/test_diffset_manager.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/scmtools/core.py
	6 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

reviewboard/diffviewer/filediff_creator.py (Diff revision 2)

Show all issues

TODO is more appropriate than FIXME I think

chipx86 Sept. 9, 2021, 12:59 p.m.

Probably. Older comment (from you! :)) that I just moved.

chipx86 Sept. 9, 2021, 12:59 p.m.

I'm actually going to remove the comment entirely, because permission's probably bigger than this area.

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (5c44d91)