Page MenuHomeSoftware Heritage
Differential D6745

fix expansion of the Log keyword with rsync origins
ClosedPublic
Actions

Authored by stsp on Dec 4 2021, 5:39 PM.

Details

Reviewers
ardumont
Group Reviewers
Reviewers
Maniphest Tasks
T3691: Implement CVS loader
Commits
rDLDCVS099959bbfa73: fix expansion of the Log keyword with rsync origins
Summary

Align our expansion of Log keywords with the behaviour of a real
CVS server. With this, such keywords expand the same way over
the pserver and rsync access methods.

This is the last change required to consistently ingest CVS's own
CVS repository over both pserver and rsync. Otherwise we get commit
hash mis-matches due to differently expanded Log keywords.

Diff Detail

Repository
rDLDCVS CVS Loader
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

stsp created this revision.Dec 4 2021, 5:39 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptDec 4 2021, 5:39 PM
stsp added a task: T3691: Implement CVS loader.Dec 4 2021, 5:39 PM
swh-public-ci added a comment.Dec 4 2021, 5:40 PM

Build has FAILED

Patch application report for D6745 (id=24494)

Rebasing onto f36332c7e3...

Current branch diff-target is up to date.
Changes applied before test
commit 97a215a68a89403b9097e308dd0c696209a0cc4e
Author: Stefan Sperling <stsp@stsp.name>
Date:   Wed Nov 24 12:13:11 2021 +0100

    fix expansion of the Log keyword with rsync origins
    
    Align our expansion of Log keywords with the behaviour of a real
    CVS server. With this, such keywords expand the same way over
    the pserver and rsync access methods.
    
    This is the last change required to consistently ingest CVS's own
    CVS repository over both pserver and rsync. Otherwise we get commit
    hash mis-matches due to differently expanded Log keywords.

Link to build: https://jenkins.softwareheritage.org/job/DLDCVS/job/tests-on-diff/69/
See console output for more information: https://jenkins.softwareheritage.org/job/DLDCVS/job/tests-on-diff/69/console

Harbormaster returned this revision to the author for changes because remote builds failed.Dec 4 2021, 5:40 PM
Harbormaster failed remote builds in B25368: Diff 24494!
stsp updated this revision to Diff 24495.Dec 4 2021, 7:04 PM

fix formatting for flake8

swh-public-ci added a comment.Dec 4 2021, 7:06 PM

Build is green

Patch application report for D6745 (id=24495)

Rebasing onto f36332c7e3...

Current branch diff-target is up to date.
Changes applied before test
commit 1b62b5b58dba58189b064736212cc8dea5db3454
Author: Stefan Sperling <stsp@stsp.name>
Date:   Wed Nov 24 12:13:11 2021 +0100

    fix expansion of the Log keyword with rsync origins
    
    Align our expansion of Log keywords with the behaviour of a real
    CVS server. With this, such keywords expand the same way over
    the pserver and rsync access methods.
    
    This is the last change required to consistently ingest CVS's own
    CVS repository over both pserver and rsync. Otherwise we get commit
    hash mis-matches due to differently expanded Log keywords.

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

Harbormaster completed remote builds in B25369: Diff 24495.Dec 4 2021, 7:06 PM
stsp requested review of this revision.Dec 4 2021, 7:06 PM
ardumont accepted this revision.Dec 6 2021, 10:05 AM
ardumont added a subscriber: ardumont.

Thanks.

lgtm. But, I mostly understand the rationale and the tests.

Note: There is still the question of the high number of snapshots which should really be 1 (1 snapshot per visit).
I guess you'll look into it when you got the chance to.

swh/loader/cvs/cvs2gitdump/cvs2gitdump.py
578

At some point when you get a chance to it, please roughly describe the expansion
algorithm in the docstring.

So the code below gets a bit more understandable.

669

This new branching part is missing coverage.

This revision is now accepted and ready to land.Dec 6 2021, 10:05 AM
douardda added a subscriber: douardda.Dec 6 2021, 10:05 AM
douardda added inline comments.
swh/loader/cvs/cvs2gitdump/cvs2gitdump.py
585

Not sure it makes a big difference, but you could keep ret a list and return b''.join(ret) (no idea how big this can get and how many lines are expected, but in doubt, I would stuck with the "proper" list-based version).

634

could you explain what this is needed?

vlorentz added a subscriber: vlorentz.Dec 6 2021, 11:05 AM
vlorentz added inline comments.
swh/loader/cvs/cvs2gitdump/cvs2gitdump.py
585

BTW, the motivation for this is that by default, appending to a string in Python requires copying the string, making the whole function quadratic in the number of inputs (n copies of a string of size O(n)).

(Here it's probably fine because CPython has an optimization to reallocate strings when appending when they have a single reference pointing to them, but I wouldn't bet my life on it; it's easy to accidentally add stray references (eg. in closures))

stsp added inline comments.Dec 6 2021, 11:23 AM
swh/loader/cvs/cvs2gitdump/cvs2gitdump.py
669

This new branching part is missing coverage.

I might have added this without good reason. I will check whether there was a case in CVS's CVS repository which requires this. Otherwise we can just drop the statement.

However, I have already addressed several other comments, and running test conversions will take a while. So I will soon upload what I have to address the other comments, with this new branch still in place.

stsp added inline comments.Dec 6 2021, 11:27 AM
swh/loader/cvs/cvs2gitdump/cvs2gitdump.py
578

At some point when you get a chance to it, please roughly describe the expansion
algorithm in the docstring.

It is not much of an "algorithm" , it is more like "whatever CVS does".
I will be adding a link to relevant CVS docs. For most details however the CVS implementation behaviour dictates what we need to do.

585

Thanks, this makes sense. I will be switching back to a list in the next version of the patch.

634

could you explain what this is needed?

This is hard to explain without an example. I will add a comment in the next iteration of the patch which should hopefully make things more clear.

stsp updated this revision to Diff 24502.Dec 6 2021, 11:29 AM

addressed review comments; see my responses to various comments for details

swh-public-ci added a comment.Dec 6 2021, 11:30 AM

Build is green

Patch application report for D6745 (id=24502)

Rebasing onto f36332c7e3...

Current branch diff-target is up to date.
Changes applied before test
commit 3a3d6a1050a78fdacfb253226c7a9f3fadbb49a2
Author: Stefan Sperling <stsp@stsp.name>
Date:   Wed Nov 24 12:13:11 2021 +0100

    fix expansion of the Log keyword with rsync origins
    
    Align our expansion of Log keywords with the behaviour of a real
    CVS server. With this, such keywords expand the same way over
    the pserver and rsync access methods.
    
    This is the last change required to consistently ingest CVS's own
    CVS repository over both pserver and rsync. Otherwise we get commit
    hash mis-matches due to differently expanded Log keywords.

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

Harbormaster completed remote builds in B25376: Diff 24502.Dec 6 2021, 11:30 AM
stsp updated this revision to Diff 24503.Dec 6 2021, 11:31 AM

fix typo in comment

swh-public-ci added a comment.Dec 6 2021, 11:33 AM

Build is green

Patch application report for D6745 (id=24503)

Rebasing onto f36332c7e3...

Current branch diff-target is up to date.
Changes applied before test
commit 37fbd0932e6aaf33f8c9b7da291c3249ab24fe97
Author: Stefan Sperling <stsp@stsp.name>
Date:   Wed Nov 24 12:13:11 2021 +0100

    fix expansion of the Log keyword with rsync origins
    
    Align our expansion of Log keywords with the behaviour of a real
    CVS server. With this, such keywords expand the same way over
    the pserver and rsync access methods.
    
    This is the last change required to consistently ingest CVS's own
    CVS repository over both pserver and rsync. Otherwise we get commit
    hash mis-matches due to differently expanded Log keywords.

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

Harbormaster completed remote builds in B25377: Diff 24503.Dec 6 2021, 11:33 AM
stsp added inline comments.Dec 7 2021, 9:29 AM
swh/loader/cvs/cvs2gitdump/cvs2gitdump.py
669

Tweaking of the final \n was indeed not necessary in order to convert CVS's own history properly. I will remove these two lines in the next version of the patch.

stsp updated this revision to Diff 24532.Dec 7 2021, 9:30 AM

remove unnecessary tweaking of final \n in log buffer

swh-public-ci added a comment.Dec 7 2021, 9:31 AM

Build is green

Patch application report for D6745 (id=24532)

Rebasing onto f36332c7e3...

Current branch diff-target is up to date.
Changes applied before test
commit 099959bbfa73438eda5c08ebfa0f1f766808fcc7
Author: Stefan Sperling <stsp@stsp.name>
Date:   Wed Nov 24 12:13:11 2021 +0100

    fix expansion of the Log keyword with rsync origins
    
    Align our expansion of Log keywords with the behaviour of a real
    CVS server. With this, such keywords expand the same way over
    the pserver and rsync access methods.
    
    This is the last change required to consistently ingest CVS's own
    CVS repository over both pserver and rsync. Otherwise we get commit
    hash mis-matches due to differently expanded Log keywords.

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

Harbormaster completed remote builds in B25406: Diff 24532.Dec 7 2021, 9:31 AM
Closed by commit rDLDCVS099959bbfa73: fix expansion of the Log keyword with rsync origins (authored by stsp). · Explain WhyDec 7 2021, 9:53 AM
This revision was automatically updated to reflect the committed changes.
stsp added a commit: rDLDCVS099959bbfa73: fix expansion of the Log keyword with rsync origins.
stsp mentioned this in D6758: make CVS loader create one snapshot per visit.Dec 7 2021, 10:59 AM
stsp mentioned this in rDLDCVS5298a8f9500e: make CVS loader create one snapshot per visit.Dec 7 2021, 11:26 AM

Revision Contents
Changeset List

PathSize
swh/
loader/
cvs/
cvs2gitdump/
80 lines
tests/
data/
88 lines

Diff 24533

View Options

swh/loader/cvs/cvs2gitdump/cvs2gitdump.py

Loading...
View Options

swh/loader/cvs/tests/data/greek-repository8.tgz

Loading...
View Options

swh/loader/cvs/tests/test_loader.py

Loading...
About · Developer information · Legal