Page MenuHomeSoftware Heritage

SvnLoaderFromRemoteDump: Fix failed visit which should be an uneventful visit
ClosedPublic

Authored by ardumont on Wed, Sep 29, 7:38 PM.

Details

Summary

Due to a miscomputation of svn commit range at startup, the second visit of a SvnLoaderFromRemoteDump fails.
It should be caught into an uneventful visit as nothing new happened in between visits.

Related to T3622

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

Build is green

Patch application report for D6376 (id=23201)

Rebasing onto 666f32a01c...

Current branch diff-target is up to date.
Changes applied before test
commit 1e5a25c92a6ba8dfc8d2e8ee0d80483267e117c0
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 29 19:35:38 2021 +0200

    svn: Incremental loading fails sometimes with SvnLoaderFromRemoteDump
    
    Related to T3622

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

ardumont retitled this revision from svn: Incremental loading fails sometimes with SvnLoaderFromRemoteDump to wip: svn: Incremental loading fails sometimes with SvnLoaderFromRemoteDump.Wed, Sep 29, 7:42 PM
ardumont edited the summary of this revision. (Show Details)
ardumont retitled this revision from wip: svn: Incremental loading fails sometimes with SvnLoaderFromRemoteDump to svn: Fix missed uneventful visits on some edge case.Wed, Sep 29, 8:06 PM
ardumont edited the summary of this revision. (Show Details)

Build is green

Patch application report for D6376 (id=23202)

Rebasing onto 666f32a01c...

Current branch diff-target is up to date.
Changes applied before test
commit 79fa16c1ce18fabe27f910ea71b1793222c326e0
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 29 19:35:38 2021 +0200

    svn: Fix missed uneventful visits on some edge case
    
    Due to an off-by-one mistake.
    
    Related to T3622

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

Assert snapshot is not null

Build is green

Patch application report for D6376 (id=23203)

Rebasing onto 666f32a01c...

Current branch diff-target is up to date.
Changes applied before test
commit b57bdbd84c9c03ba8f8fd2244fe2c69898559319
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 29 19:35:38 2021 +0200

    svn: Fix missed uneventful visits on some edge case
    
    Due to an off-by-one mistake.
    
    Related to T3622

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

ardumont retitled this revision from svn: Fix missed uneventful visits on some edge case to Fix failed visit as uneventful one for the SvnLoaderFromRemoteDump.
ardumont edited the summary of this revision. (Show Details)

Rework commit message/diff description

Build is green

Patch application report for D6376 (id=23204)

Rebasing onto 666f32a01c...

Current branch diff-target is up to date.
Changes applied before test
commit 406c777dfa229989643f9bd029e53bacb9c40a11
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 29 19:35:38 2021 +0200

    Fix failed visit as uneventful one for the SvnLoaderFromRemoteDump
    
    Due to a miscomputation at startup on the second visit for the SvnLoaderFromRemoteDump,
    this had a range of svn commit [1-0]. That in turn made the svn loader resolve an
    unknown revision number. This should have been an uneventful visit from the start.
    
    This commit fixes it.
    
    Related to T3622

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

douardda added a subscriber: douardda.

Looks ok (not sure I really understand the fix however, more precisely, what was the purpose of the revision_start != 1 condition), but I really don't understand the commit message:

Fix failed visit as uneventful one for the SvnLoaderFromRemoteDump

the "one" here is at best useless (and actually confusing IMHO)

Due to a miscomputation

miscomputation of what?

at startup on the second visit for the SvnLoaderFromRemoteDump, this

What does "this" refer to?

had a range of svn commit [1-0].

is the [1-0] the actual (mistakenly) computed range of revisions?

That in turn made the svn loader resolve an unknown revision number.

What does "resolve [a] revision number" means for a loader?

This should have been an uneventful visit from the start.

This revision is now accepted and ready to land.Thu, Sep 30, 9:40 AM

Looks ok (not sure I really understand the fix however, more precisely, what was the purpose of the revision_start != 1 condition), but I really don't understand the commit message:

I don't recall why i added that revision_start != 1 check at all.
Our extended and mostly complete tests seems to not care at all about that check.

And the new test reproduced out of the reality of the sourceforge ingestion makes it a problem (see the stacktrace in the task).
Drop my new code and enjoy the same stacktrace of that task.

Fix failed visit as uneventful one for the SvnLoaderFromRemoteDump

the "one" here is at best useless (and actually confusing IMHO)

That's actually a correct sentence though.
Read 'Fix failed visit as uneventful visit'.

Due to a miscomputation

miscomputation of what?

The loader svn computes a range of svn commits to determine what commits to load.
It does it badly in that case. I explicitely mentioned the wrong range of svn commit [1-0].
that *this* particular situation boils down to.

at startup on the second visit for the SvnLoaderFromRemoteDump, this

What does "this" refer to?

had a range of svn commit [1-0].

is the [1-0] the actual (mistakenly) computed range of revisions?

That in turn made the svn loader resolve an unknown revision number.

What does "resolve [a] revision number" means for a loader?

svn commit number, i should have used commit.
svn commit identifier are integers.


I completely rewrote everything, hopefully it's way better.

ardumont retitled this revision from Fix failed visit as uneventful one for the SvnLoaderFromRemoteDump to SvnLoaderFromRemoteDump: Fix failed visit which should be an uneventful visit.Thu, Sep 30, 9:53 AM

Realign commit message and diff description

Build is green

Patch application report for D6376 (id=23207)

Rebasing onto 666f32a01c...

Current branch diff-target is up to date.
Changes applied before test
commit 7ee2795d5cc2c768c15a2e00190071fe2929fc70
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 29 19:35:38 2021 +0200

    SvnLoaderFromRemoteDump: Fix failed visit which should be an uneventful visit
    
    Due to a miscomputation of svn commit range at startup, the second visit of a
    SvnLoaderFromRemoteDump fails. It should be caught into an uneventful visit as nothing
    new happened in between visits.
    
    Related to T3622

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