Page MenuHomeSoftware Heritage

gitlab: Allow ingestion of hg_git origins as hg ones
ClosedPublic

Authored by ardumont on Fri, Sep 17, 11:51 AM.

Diff Detail

Repository
rDLS Listers
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 D6286 (id=22772)

Rebasing onto 4e4edee478...

Current branch diff-target is up to date.
Changes applied before test
commit 1cf34613cbb903fd288b057c7f86e67ed9af9b00
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 17 11:50:58 2021 +0200

    gitlab: Allow ingestion of hg_git origins as hg ones
    
    Related to T3581#70593

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

Build is green

Patch application report for D6286 (id=22773)

Rebasing onto 4e4edee478...

Current branch diff-target is up to date.
Changes applied before test
commit 2973e841fbc166e7f02da1f553e40a279e54635e
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 17 11:50:58 2021 +0200

    gitlab: Allow ingestion of hg_git origins as hg ones
    
    Related to T3581#70593

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

anlambert added inline comments.
swh/lister/gitlab/lister.py
26

I would remove the hg to hg mapping here.

28

not sure if we need this global variable here.

211–212

How about splitting this into two instructions, something like:

visit_type = repo.get("vcs_type", "git")
visit_type = VCS_MAPPING.get(visit_type, visit_type)

This is more readable this way imho.

swh/lister/gitlab/tests/test_lister.py
74–75

I would rather check the visit_type is hg in the listed origins here.

ardumont added inline comments.
swh/lister/gitlab/lister.py
26

you can't otherwise, hg are mapped to git.

>           assert VCS_MAPPING[entry["vcs_type"]] == "hg"
E           KeyError: 'hg'
211–212

ok

swh/lister/gitlab/tests/test_lister.py
74–75

It's both hg_git and hg.
Hency why i added this and the check below becomes clearer.

But i can think of something else then ;)

ardumont marked 2 inline comments as done.

Adapt according to suggestions to make code clearer! Thanks.

ardumont added inline comments.
swh/lister/gitlab/lister.py
28

ok, dropped.

Build is green

Patch application report for D6286 (id=22774)

Rebasing onto 4e4edee478...

Current branch diff-target is up to date.
Changes applied before test
commit 0c5d9a5306f6adb26a07a188fec6ac66da2d1321
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 17 11:50:58 2021 +0200

    gitlab: Allow ingestion of hg_git origins as hg ones
    
    Related to T3581#70593

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

Drop the hg to hg mapping, it's indeed not needed. I must have cross wired my mind and
fingers when testing that.

swh/lister/gitlab/lister.py
26

never mind this, i dropped it indeed.

Build is green

Patch application report for D6286 (id=22775)

Rebasing onto 4e4edee478...

Current branch diff-target is up to date.
Changes applied before test
commit fdb420238cc861e6d55e7f8a0ea73b938f83ae47
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 17 11:50:58 2021 +0200

    gitlab: Allow ingestion of hg_git origins as hg ones
    
    Related to T3581#70593

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

swh/lister/gitlab/tests/test_lister.py
74–75

I was thinking about that check instead:

scheduler_origins = lister.scheduler.get_listed_origins(
    lister.lister_obj.id
).results
assert all([origin.visit_type == "hg" for origin in scheduler_origins])
swh/lister/gitlab/tests/test_lister.py
74–75

That test is already done below line 86 in the original code though.

Here, i'm merely pointing out that the input does indeed have more vcs-type than usual.
And then that it's mapped to hg (with the existing check).

Isn't it enough?

85

Here is the test already done as mentioned above.

anlambert added inline comments.
swh/lister/gitlab/tests/test_lister.py
74–75

Ah right, it was hidden by Phabricator UI, I think we are good then.

This revision is now accepted and ready to land.Fri, Sep 17, 1:26 PM