Page MenuHomeSoftware Heritage

ra: Fix directory removal when processing a commit
ClosedPublic

Authored by anlambert on Tue, Sep 8, 3:10 PM.

Details

Summary

Due to an invalid isinstance type check (swh.model.model.Directory instead
of swh.model.from_disk.Directory), the loader ended up with error when a
directory was removed in a subversion commit.

Sentry related issue: https://sentry.softwareheritage.org/organizations/swh/issues/2563/?project=14&query=is%3Aunresolved

Test Plan

I added a new test processing a sample repository where a directory is added in
a commit and removed in another commit.

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

anlambert created this revision.Tue, Sep 8, 3:10 PM

Build is green

Patch application report for D3889 (id=13725)

Rebasing onto 34565424f5...

Current branch diff-target is up to date.
Changes applied before test
commit 0d12e3273b6892842a874991a48f93d651d3a745
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Tue Sep 8 15:07:04 2020 +0200

    ra: Fix directory removal when processing a commit
    
    Summary:
    
    Due to an invalid isinstance type check (`swh.model.model.Directory` instead
    of `swh.model.from_disk.Directory`), the loader ended up with error when a
    directory was removed in a subversion commit.
    
    Test Plan:
    
    I added a new test processing a sample repository where a directory is added in
    a commit and removed in another commit.
    
    Reviewers: ardumont, #reviewers
    
    Subscribers:

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

anlambert edited the summary of this revision. (Show Details)Tue, Sep 8, 3:37 PM

Could you create a minimal repository to trigger the issue? And preferably accompany it with a shell script to recreate it, should the need arise.

Thanks!

vlorentz requested changes to this revision.Tue, Sep 8, 4:00 PM
This revision now requires changes to proceed.Tue, Sep 8, 4:00 PM

Could you create a minimal repository to trigger the issue? And preferably accompany it with a shell script to recreate it, should the need arise.

Thanks!

I already added a simple svn repository triggering the issue in the tests data. All svn loader tests are proceeding this way. I agree the overall tests could be improved but that's out of scope for that diff.

damn!

Good catch!

Thanks.

ardumont accepted this revision.Tue, Sep 8, 5:28 PM

And preferably accompany it with a shell script to recreate it, should the need arise.

That was the initial intent behind the test_loader.org within that repository...
But its maintenance never kept up...
(we could even consider dropping it now...)

I agree the overall tests could be improved but that's out of scope for that diff.

@vlorentz, one possible way to improve tests implementation would be to create sample subversion repositories using the subvertpy API triggering the issues discovered so far.

Right now, this should be landed, tagged and deployed to production as numerous save code now requests for subversion ended up with the partial visit state due to that bug.

@vlorentz, one possible way to improve tests implementation would be to create sample subversion repositories using the subvertpy API triggering the issues discovered so far.

yes, i think some git loader tests are done that way (with "dulwich" for git heh ;)

Right now, this should be landed, tagged and deployed to production as numerous save code now requests for subversion ended up with the partial visit state due to that bug.

yes, i think it's fine if you push it right now and tag.

Cheers,

anlambert updated this revision to Diff 13736.Tue, Sep 8, 5:48 PM

Update commit message (forgot to execute arc set-config history.immutable true on my new laptop).

Build is green

Patch application report for D3889 (id=13736)

Rebasing onto 34565424f5...

Current branch diff-target is up to date.
Changes applied before test
commit 510c02644745363ef87baa255180bc0ccc874526
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Tue Sep 8 15:07:04 2020 +0200

    ra: Fix directory removal when processing a commit
    
    Due to an invalid isinstance type check, the loader ended up with error when a
    directory was removed in a subversion commit.

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

This revision was not accepted when it landed; it landed in state Needs Review.Tue, Sep 8, 5:50 PM
This revision was automatically updated to reflect the committed changes.

yes, i think it's fine if you push it right now and tag.

Pushed and tagged as v0.4.1.

but that's out of scope for that diff

it's not, I didn't ask to change existing tests, but to change the data we're using for new tests