Details

Reviewers

ardumont

Group Reviewers

Reviewers

Maniphest Tasks

T611: support for external definitions in the svn/subversion loader

Commits

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

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 jenkins	Jenkins console · Jenkins
Build 40590: arc lint + arc unit

Event Timeline

anlambert created this revision.Dec 14 2021, 5:55 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptDec 14 2021, 5:55 PM

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.

Harbormaster completed remote builds in B25635: Diff 24786.Dec 14 2021, 5:57 PM

anlambert requested review of this revision.Dec 14 2021, 5:57 PM

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?

In D6839#178745, @douardda wrote:

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.

Harbormaster completed remote builds in B25801: Diff 24930.Jan 4 2022, 5:59 PM

Fix docstring

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.

Harbormaster completed remote builds in B25802: Diff 24931.Jan 4 2022, 6:15 PM

ardumont added 1 blocking reviewer(s): Reviewers.Jan 5 2022, 9:58 AM

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

ardumont resigned from this revision.Jan 5 2022, 10:01 AM

This revision now requires review to proceed.Jan 5 2022, 10:01 AM

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

Rebase

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.

Harbormaster completed remote builds in B25826: Diff 24955.Jan 5 2022, 4:20 PM

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.

Harbormaster completed remote builds in B25877: Diff 25007.Jan 7 2022, 6:55 PM

ardumont accepted this revision.Jan 10 2022, 10:42 AM

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

anlambert added a child revision: D6895: ra: Add support for subversion external definitions.Jan 10 2022, 2:19 PM

Rebase

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.

Harbormaster completed remote builds in B25971: Diff 25097.Jan 12 2022, 2:55 PM

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.

Harbormaster completed remote builds in B26031: Diff 25167.Jan 14 2022, 12:03 PM

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.

Harbormaster completed remote builds in B26083: Diff 25222.Jan 18 2022, 10:53 AM

Rebase

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.

Harbormaster completed remote builds in B26098: Diff 25237.Jan 18 2022, 11:11 AM

Closed by commit rDLDSVNf1913512a5fa: utils: Add a function to parse a subversion external definition (authored by anlambert). · Explain WhyJan 18 2022, 12:50 PM

This revision was automatically updated to reflect the committed changes.

anlambert added a commit: rDLDSVNf1913512a5fa: utils: Add a function to parse a subversion external definition.

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

Details

Diff Detail

Event Timeline

Patch application report for D6839 (id=24786)

Changes applied before test

Patch application report for D6839 (id=24930)

Changes applied before test

Patch application report for D6839 (id=24931)

Changes applied before test

Patch application report for D6839 (id=24955)

Changes applied before test

Patch application report for D6839 (id=25007)

Changes applied before test

Patch application report for D6839 (id=25097)

Changes applied before test

Patch application report for D6839 (id=25167)

Changes applied before test

Patch application report for D6839 (id=25222)

Changes applied before test

Patch application report for D6839 (id=25237)

Changes applied before test

Revision Contents
Changeset List

Diff 25097

swh/loader/svn/tests/test_utils.py

swh/loader/svn/utils.py

utils: Add a function to parse a subversion external definitionClosedPublicActions

Details

Diff Detail

Event Timeline

Patch application report for D6839 (id=24786)

Changes applied before test

Patch application report for D6839 (id=24930)

Changes applied before test

Patch application report for D6839 (id=24931)

Changes applied before test

Patch application report for D6839 (id=24955)

Changes applied before test

Patch application report for D6839 (id=25007)

Changes applied before test

Patch application report for D6839 (id=25097)

Changes applied before test

Patch application report for D6839 (id=25167)

Changes applied before test

Patch application report for D6839 (id=25222)

Changes applied before test

Patch application report for D6839 (id=25237)

Changes applied before test

Revision ContentsChangeset List

Diff 25097

swh/loader/svn/tests/test_utils.py

swh/loader/svn/utils.py

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

Revision Contents
Changeset List