Page MenuHomeSoftware Heritage

utils: Add a function to parse a subversion external definition
ClosedPublic

Authored by anlambert on Dec 14 2021, 5:55 PM.

Details

Summary

Add a function to parse an external definition according to official
specifications in order to extract or compute:

  • the relative path where the external should be exported
  • the URL of the external
  • the optional revision of the external to export

Related to T611

Depends on D6838

Diff Detail

Repository
rDLDSVN Subversion (SVN) loader
Branch
svn-loader-parse-external
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25971
Build 40591: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 40590: arc lint + arc unit

Event Timeline

Build is green

Patch application report for D6839 (id=24786)

Could not rebase; Attempt merge onto 0ae91422d8...

Updating 0ae9142..42e4b27
Fast-forward
 swh/loader/svn/loader.py           |  19 ++--
 swh/loader/svn/ra.py               | 106 ++++++++++----------
 swh/loader/svn/tests/test_utils.py | 191 +++++++++++++++++++++++++++++++++++++
 swh/loader/svn/utils.py            | 116 ++++++++++++++++++++++
 4 files changed, 366 insertions(+), 66 deletions(-)
Changes applied before test
commit 42e4b27b17a4f50aa984e76b9b7ef9a0811526ff
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Tue Dec 14 17:39:07 2021 +0100

    utils: Add a function to parse a subversion external definition
    
    Add a function to parse an external definition according to official
    specifications in order to extract or compute:
    
      - the relative path where the external should be exported
    
      - the URL of the external
    
      - the optional revision of the external to export
    
    Related to T611

commit c56712550aa4597935e9798bceefb5c8e7b8a860
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Wed Dec 8 19:26:13 2021 +0100

    loader: Simplify fetch_data implementation

commit f069043f70901218c13fcf08883e3f9c9e56617e
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Wed Dec 8 17:23:22 2021 +0100

    ra: Add debug logging when setting svn:externals property on a path
    
    Related to T611

commit 6a889e041a4039b8c2a213e8b0146a48fe10b233
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Wed Dec 8 13:40:58 2021 +0100

    ra: Add path parameter to DirEditor
    
    It enables to get the path of the currently processed directory
    which can be useful for logging and future improvements of the
    loader (svn:externals property support for instance).
    
    Related to T611

commit f6f0c5bb3703ea1b2ba271eca43e64b51948c6d2
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Wed Dec 8 11:55:04 2021 +0100

    ra: Merge BaseDirEditor and DirEditor into a single DirEditor class
    
    DirEditor being the only class derived from the BaseDirEditor one,
    let's merge their code into a single DirEditor class.

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

Gawd this is horrible! (not your fault!)

swh/loader/svn/utils.py
172

I don't understand the usage of 'absolute' in this docstring. Wouldn't it better to describe it as "according SVN external definitions" or so?

I mean 'join("url://path/", "/toto")' giving "url://path/toto" does not make much sense to me (being used to urllib's behavior) but qulifying this as "absolute" does not make much sense, so "join url the SVN way" seems better to me :-)

222

why does't external.split() suffice for the job?

282

I see no test for this case (url@toto), what is the expected result there?

Gawd this is horrible! (not your fault!)

Yeah, I had a lot of fun writing this one but subversion externals specification can have so many forms.

swh/loader/svn/utils.py
172

Ack, I can rename it to svn_urljoin, this will be less confusing.

222

yeah right, it is enough to do so.

282

We can have some URLs like http://anon:anon@svn.example.com/skin-maker (it is in a test case), so that workaround.

Rebase and adapt according to @douardda reviews

Build is green

Patch application report for D6839 (id=24930)

Rebasing onto 196c9d596b...

Current branch diff-target is up to date.
Changes applied before test
commit bf0c228d00eb32a11291047e42b7c62f9be4e779
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Tue Dec 14 17:39:07 2021 +0100

    utils: Add a function to parse a subversion external definition
    
    Add a function to parse an external definition according to official
    specifications in order to extract or compute:
    
      - the relative path where the external should be exported
    
      - the URL of the external
    
      - the optional revision of the external to export
    
    Related to T611

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

Build is green

Patch application report for D6839 (id=24931)

Rebasing onto 196c9d596b...

Current branch diff-target is up to date.
Changes applied before test
commit f78f71aecfda81f4518e13921e38508624f82925
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Tue Dec 14 17:39:07 2021 +0100

    utils: Add a function to parse a subversion external definition
    
    Add a function to parse an external definition according to official
    specifications in order to extract or compute:
    
      - the relative path where the external should be exported
    
      - the URL of the external
    
      - the optional revision of the external to export
    
    Related to T611

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

ardumont added a subscriber: ardumont.

lgtm

I got the feeling the more we will dig into those old vcs the more will get code like this in our loaders...
(I recall having read some similar parsing code for the cvs one).

I've changed the reviewers to blocking:reviewers so david has the last word on your good adaptations.

Note:
Sorry, it took me some time to actually reading through it (I started in december...).

This revision is now accepted and ready to land.Jan 5 2022, 10:01 AM
This revision now requires review to proceed.Jan 5 2022, 10:01 AM
ardumont removed a reviewer: ardumont. ardumont added 1 blocking reviewer(s): Reviewers.

lather, rinse, repeat (correctly accepting it this time)

Build is green

Patch application report for D6839 (id=24955)

Rebasing onto 1b13c5c19e...

Current branch diff-target is up to date.
Changes applied before test
commit aaaa4bc8dc3cb65fdca2220307bbd80f1415f84e
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Tue Dec 14 17:39:07 2021 +0100

    utils: Add a function to parse a subversion external definition
    
    Add a function to parse an external definition according to official
    specifications in order to extract or compute:
    
      - the relative path where the external should be exported
    
      - the URL of the external
    
      - the optional revision of the external to export
    
    Related to T611

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

Fix edge case encountered while testing on real world repositories.

Build is green

Patch application report for D6839 (id=25007)

Rebasing onto 1b13c5c19e...

Current branch diff-target is up to date.
Changes applied before test
commit 217a2d07811bd49912b776a7419e3f0135e2643c
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Tue Dec 14 17:39:07 2021 +0100

    utils: Add a function to parse a subversion external definition
    
    Add a function to parse an external definition according to official
    specifications in order to extract or compute:
    
      - the relative path where the external should be exported
    
      - the URL of the external
    
      - the optional revision of the external to export
    
    Related to T611

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

This revision is now accepted and ready to land.Jan 10 2022, 10:42 AM

Build is green

Patch application report for D6839 (id=25097)

Rebasing onto 93b4f2fdd8...

Current branch diff-target is up to date.
Changes applied before test
commit 2e7a0c23200404d815601dbb83bdb5bcc3c6c4be
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Tue Dec 14 17:39:07 2021 +0100

    utils: Add a function to parse a subversion external definition
    
    Add a function to parse an external definition according to official
    specifications in order to extract or compute:
    
      - the relative path where the external should be exported
    
      - the URL of the external
    
      - the optional revision of the external to export
    
    Related to T611

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

Add a boolean in the tuple returned by the parse function indicating
if the external URL was defined as relative to the repository one
bu targets a path outside the repository.

Build is green

Patch application report for D6839 (id=25167)

Rebasing onto 93b4f2fdd8...

Current branch diff-target is up to date.
Changes applied before test
commit 2f5fd60ab91f5af90c3333f369cb7b72b28b3fbe
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Tue Dec 14 17:39:07 2021 +0100

    utils: Add a function to parse a subversion external definition
    
    Add a function to parse an external definition according to official
    specifications in order to extract or compute:
    
      - the relative path where the external should be exported
    
      - the URL of the external
    
      - the optional revision of the external to export
    
    Related to T611

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

Update: Handle a couple of edge cases found when testing with real world repositories.

Build is green

Patch application report for D6839 (id=25222)

Rebasing onto 93b4f2fdd8...

Current branch diff-target is up to date.
Changes applied before test
commit 30b3c8427391edc0f88dce202dcb8abb07bf548b
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Tue Dec 14 17:39:07 2021 +0100

    utils: Add a function to parse a subversion external definition
    
    Add a function to parse an external definition according to official
    specifications in order to extract or compute:
    
      - the relative path where the external should be exported
    
      - the URL of the external
    
      - the optional revision of the external to export
    
    Related to T611

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

Build is green

Patch application report for D6839 (id=25237)

Rebasing onto cb4bf60c0e...

Current branch diff-target is up to date.
Changes applied before test
commit f1913512a5faa0c99d23607b9d63fc6003c729fb
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Tue Dec 14 17:39:07 2021 +0100

    utils: Add a function to parse a subversion external definition
    
    Add a function to parse an external definition according to official
    specifications in order to extract or compute:
    
      - the relative path where the external should be exported
    
      - the URL of the external
    
      - the optional revision of the external to export
    
    Related to T611

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