Paths

Table of Contentst

Differential D3828

svn.loader: Simplify and align incremental visit algorithm...
ClosedPublic
Actions

Authored by ardumont on Aug 24 2020, 11:18 AM.

Details

Reviewers

vlorentz

Group Reviewers

Reviewers

Commits

rDLDSVN9a2f4b45dc6d: svn.loader: Simplify and align incremental visit algorithm...

Summary

... with other loaders.

Prior to this commit, the pre-snapshot existing code to resolve the starting
point of a visit was convoluted and hard to follow.

This simplifies it to actually always use the snapshot to start a new visit as other
loaders do.

This also:

adds more type on impacted methods
rework slightly docstrings on impacted methods
adds more coverage on the remote dump loader implementation
fixes an eventful assertion which should have been uneventful

Test Plan

tox

Diff Detail

Repository

rDLDSVN Subversion (SVN) loader

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ardumont created this revision.Aug 24 2020, 11:18 AM

Herald added a reviewer: Reviewers. · View Herald TranscriptAug 24 2020, 11:18 AM

Build is green

Patch application report for D3828 (id=13475)

Rebasing onto 1d3afffda0...

Current branch diff-target is up to date.

Changes applied before test

commit 728500e14414d89eb6d0dbbbd3ca093f07177061
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Aug 24 11:17:36 2020 +0200

    svn.loader: Simplify and align incremental visit algorithm...
    
    ... with other loaders.
    
    Prior to this commit, the pre-snapshot existing code to resolve the starting
    point of a visit was convoluted and hard to follow.
    
    This simplifies it to actually use the snapshot to start a new visit as other
    loaders do.

See https://jenkins.softwareheritage.org/job/DLDSVN/job/tests-on-diff/87/ for more details.

Harbormaster completed remote builds in B14736: Diff 13475.Aug 24 2020, 11:20 AM

ardumont added inline comments.Aug 24 2020, 11:22 AM

swh/loader/svn/loader.py
116	I might have clean up to do. Actually open the diff to see more clearly the code from afar (kinda).
621–622	This is wrong...
swh/loader/svn/tests/test_loader.py
124	That now actually makes sense. Nothing should have happened initially \m/

Fix issue on remote dump loader and add an assertion for it

Build is green

Patch application report for D3828 (id=13477)

Rebasing onto 1d3afffda0...

Current branch diff-target is up to date.

Changes applied before test

commit bd43db156d5c723668d53be9d07ff73635450be3
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Aug 24 11:17:36 2020 +0200

    svn.loader: Simplify and align incremental visit algorithm...
    
    ... with other loaders.
    
    Prior to this commit, the pre-snapshot existing code to resolve the starting
    point of a visit was convoluted and hard to follow.
    
    This simplifies it to actually use the snapshot to start a new visit as other
    loaders do.

See https://jenkins.softwareheritage.org/job/DLDSVN/job/tests-on-diff/88/ for more details.

Harbormaster completed remote builds in B14737: Diff 13477.Aug 24 2020, 11:38 AM

ardumont added inline comments.Aug 24 2020, 11:38 AM

swh/loader/svn/loader.py
621–622	fixed.

ardumont edited the summary of this revision. (Show Details)Aug 24 2020, 11:40 AM

What was all the code you removed doing? Why was it needed, and why isn't it anymore?

swh/loader/svn/loader.py
241	`as opposed to starting from...`

vlorentz added inline comments.Aug 24 2020, 1:03 PM

swh/loader/svn/loader.py
184	if branch.target_type != TargetType.REVISION:

What was all the code you removed doing?

Like i said (in the description), the same thing as now but in a convoluted way... (because it existed prior to snapshot,
it initially started using "occurrence" objects which no longer exist).

Well, it was also possible to pass revision (id) as parameter but this was never used aside from previous version tests (ones prior to the pytest refactoring).

Why was it needed, and why isn't it anymore?

It was not, i'm dropping the noise is all.
(it was never used nor covered as we can see here, i did not touch the tests aside from the 'uneventful' fix)

vlorentz added inline comments.Aug 24 2020, 1:10 PM

swh/loader/svn/loader.py
112–113	you can remove this attribute, I believe

Adapt according to exchange:

Rework docstring
Rework conditional to use TargetType.REVISION
Drop unused internal attribute
(bonus) Add one more type

Build is green

Patch application report for D3828 (id=13480)

Rebasing onto 1d3afffda0...

Current branch diff-target is up to date.

Changes applied before test

commit 9a2f4b45dc6d91c1b0a33f30dd563f6eac69070f
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Aug 24 11:17:36 2020 +0200

    svn.loader: Simplify and align incremental visit algorithm...
    
    ... with other loaders.
    
    Prior to this commit, the pre-snapshot existing code to resolve the starting
    point of a visit was convoluted and hard to follow.
    
    This simplifies it to actually use the snapshot to start a new visit as other
    loaders do.

See https://jenkins.softwareheritage.org/job/DLDSVN/job/tests-on-diff/89/ for more details.

Harbormaster completed remote builds in B14740: Diff 13480.Aug 24 2020, 1:16 PM

ardumont edited the summary of this revision. (Show Details)Aug 24 2020, 1:17 PM

vlorentz accepted this revision.Aug 24 2020, 1:40 PM

This revision is now accepted and ready to land.Aug 24 2020, 1:40 PM

Closed by commit rDLDSVN9a2f4b45dc6d: svn.loader: Simplify and align incremental visit algorithm... (authored by ardumont). · Explain WhyAug 24 2020, 1:57 PM

This revision was automatically updated to reflect the committed changes.

ardumont added a commit: rDLDSVN9a2f4b45dc6d: svn.loader: Simplify and align incremental visit algorithm....