Details

Reviewers

anlambert

Group Reviewers

Reviewers

Maniphest Tasks

T376: ingest git.eclipse.org repositories
T2999: Optimize the number of HTTP requests sent by the cgit lister

Commits

rDLS2e2207355815: cgit: Compute origin urls out of a base git url when provided.

Summary

This adds a second behavior to the cgit lister to actually compute origin urls instead
of parsing them out of another http request on git detailed page (per repository listed).

This new behavior is expected to be the default behavior.

The old behavior is kept for now and is expected to be used as fallback if too much
false negatives are returned.

(This is a requisite for the git.eclipse.org instance for example which cannot be listed otherwise [1]).

Related to T2999

[1] Related to T376#57560

Diff Detail

Repository

rDLS Listers

Branch

master

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 18890
Build 29265: Phabricator diff pipeline on jenkins	Jenkins console · Jenkins
Build 29264: arc lint + arc unit

Event Timeline

ardumont created this revision.Jan 29 2021, 11:41 AM

Herald added a reviewer: Reviewers. · View Herald TranscriptJan 29 2021, 11:41 AM

Build is green

Patch application report for D4968 (id=17741)

Rebasing onto 5aa7c8f2b2...

Current branch diff-target is up to date.

Changes applied before test

commit 14d47f1074ca16f1632b24fd416e86e51fee0354
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Jan 29 11:38:26 2021 +0100

    cgit: Compute origin urls out of a base git url when provided.
    
    This adds a second behavior to the cgit lister to actually compute origin urls instead
    of parsing them out of another http request on git detailed page.
    
    This new behavior is expected to be the default behavior.
    
    The old behavior is kept for now and is expected to be used as fallback if too much
    false negatives are returned.
    
    Related to T2999

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

Harbormaster completed remote builds in B18882: Diff 17741.Jan 29 2021, 11:44 AM

ardumont requested review of this revision.Jan 29 2021, 11:44 AM

ardumont mentioned this in T376: ingest git.eclipse.org repositories.Jan 29 2021, 11:51 AM

Update docstrings

ardumont edited the summary of this revision. (Show Details)Jan 29 2021, 11:58 AM

Build is green

Patch application report for D4968 (id=17742)

Rebasing onto 5aa7c8f2b2...

Current branch diff-target is up to date.

Changes applied before test

commit 9a56097e1dfb1725a48371f208e89408eaaa7049
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Jan 29 11:38:26 2021 +0100

    cgit: Compute origin urls out of a base git url when provided.
    
    This adds a second behavior to the cgit lister to actually compute origin urls instead
    of parsing them out of another http request on git detailed page.
    
    This new behavior is expected to be the default behavior.
    
    The old behavior is kept for now and is expected to be used as fallback if too much
    false negatives are returned.
    
    Related to T2999

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

Harbormaster completed remote builds in B18883: Diff 17742.Jan 29 2021, 12:01 PM

ardumont edited the summary of this revision. (Show Details)Jan 29 2021, 12:21 PM

ardumont added projects: Lister, CGit lister.

ardumont added a task: T376: ingest git.eclipse.org repositories.

The current implementation will not work for most cases where the base cgit URL and the base clone URL differs (see inline comment).

Also tasks parameters should be updated to handle the new one.

swh/lister/cgit/lister.py
101–114	The `urljoin` approach will not work for most of cases. For instance with that cgit instance with base URL `https://forge.frm2.tum.de/cgit/cgit.cgi/` and base clone URL `https://forge.frm2.tum.de/review/` , the computed clone URLs will be in the form `https://forge.frm2.tum.de/cgit/cgit.cgi/<path_to_repo>`. The approach I was thinking of is the following: repo_url = urljoin(self.url, repository_link) if self.base_git_url: # mapping provided, we compute the git url repo_url = urljoin(self.url, repository_link) git_url = repo_url.replace(self.url, self.base_git_url) logger.debug("%s => %s", repo_url, git_url) This way it is guaranteed to compute the good clone URLs.

This revision now requires changes to proceed.Jan 29 2021, 12:26 PM

ardumont added inline comments.Jan 29 2021, 12:54 PM

swh/lister/cgit/lister.py
101–114	oh, right. The current implem is not enough and the test assertion needs to be improved. Will adapt.
swh/lister/cgit/tests/test_lister.py
205	not that important but i meant it as `/r` here ;)

Adapt according to review

ardumont marked an inline comment as done.Jan 29 2021, 2:16 PM

Build is green

Patch application report for D4968 (id=17743)

Rebasing onto 5aa7c8f2b2...

Current branch diff-target is up to date.

Changes applied before test

commit e22d81d78d3e7a5fa30a6b0a2d025fa2375346f7
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Jan 29 11:38:26 2021 +0100

    cgit: Compute origin urls out of a base git url when provided.
    
    This adds a second behavior to the cgit lister to actually compute origin urls instead
    of parsing them out of another http request on git detailed page.
    
    This new behavior is expected to be the default behavior.
    
    The old behavior is kept for now and is expected to be used as fallback if too much
    false negatives are returned.
    
    Related to T2999

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

Harbormaster completed remote builds in B18884: Diff 17743.Jan 29 2021, 2:17 PM

anlambert added inline comments.Jan 29 2021, 2:28 PM

swh/lister/cgit/lister.py
32–34	the listed origin urls are computed out of the base git url and the ones listed in each page (resulting in less HTTP queries than the 2nd behavior below)
104–111	You could compute `repo_url` before the if and drop the else block.
swh/lister/cgit/tests/test_lister.py
200	maybe you could parametrize the test with more URLs (using edge case URLs listed in T2999).

ardumont added inline comments.Jan 29 2021, 3:15 PM

swh/lister/cgit/tests/test_lister.py
200	ok, but that means more html sample files in that diff (currenty adding one other origin)

anlambert added inline comments.Jan 29 2021, 3:22 PM

swh/lister/cgit/tests/test_lister.py
200	Ah right, you could isolate the code to compute URLs in a method and only test that then.

Adapt according to review:

Add more cgit instance samples
Rework docstring
Rework url computation a bit

Rework the lister docstring to clarify the 2nd behavior.

Build is green

Patch application report for D4968 (id=17749)

Rebasing onto 4cf0c7f765...

First, rewinding head to replay your work on top of it...
Applying: cgit: Compute origin urls out of a base git url when provided.

Changes applied before test

commit 8d989d3adabc5cf16c4397d09710b24a71836e8e
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Jan 29 11:38:26 2021 +0100

    cgit: Compute origin urls out of a base git url when provided.
    
    This adds a second behavior to the cgit lister to actually compute origin urls instead
    of parsing them out of another http request on git detailed page.
    
    This new behavior is expected to be the default behavior.
    
    The old behavior is kept for now and is expected to be used as fallback if too much
    false negatives are returned.
    
    Related to T2999

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

Harbormaster completed remote builds in B18890: Diff 17749.Jan 29 2021, 3:31 PM

ardumont marked an inline comment as done.Jan 29 2021, 3:31 PM

ardumont added inline comments.

swh/lister/cgit/tests/test_lister.py
200	well, it's done now ¯\_(ツ)_/¯ :)

Looks good to me !

This revision is now accepted and ready to land.Jan 29 2021, 3:31 PM

ardumont marked 2 inline comments as done.Jan 29 2021, 3:31 PM

Rebase

Build is green

Patch application report for D4968 (id=17750)

Rebasing onto 4cf0c7f765...

Current branch diff-target is up to date.

Changes applied before test

commit 2e220735581532f90755fc2fccefaf70a386ded0
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Jan 29 11:38:26 2021 +0100

    cgit: Compute origin urls out of a base git url when provided.
    
    This adds a second behavior to the cgit lister to actually compute origin urls instead
    of parsing them out of another http request on git detailed page.
    
    This new behavior is expected to be the default behavior.
    
    The old behavior is kept for now and is expected to be used as fallback if too much
    false negatives are returned.
    
    Related to T2999

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

Harbormaster completed remote builds in B18891: Diff 17750.Jan 29 2021, 3:36 PM

Closed by commit rDLS2e2207355815: cgit: Compute origin urls out of a base git url when provided. (authored by ardumont). · Explain WhyJan 29 2021, 3:37 PM

This revision was automatically updated to reflect the committed changes.

ardumont added a commit: rDLS2e2207355815: cgit: Compute origin urls out of a base git url when provided..

cgit: Compute origin urls out of a base git url when provided.
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Patch application report for D4968 (id=17741)

Changes applied before test

Patch application report for D4968 (id=17742)

Changes applied before test

Patch application report for D4968 (id=17743)

Changes applied before test

Patch application report for D4968 (id=17749)

Changes applied before test

Patch application report for D4968 (id=17750)

Changes applied before test

Revision Contents
Changeset List

Diff 17749

swh/lister/cgit/lister.py

swh/lister/cgit/tasks.py

swh/lister/cgit/tests/data/https_git.baserock.org/cgit

swh/lister/cgit/tests/data/https_git.eclipse.org/c

swh/lister/cgit/tests/data/https_jff.email/cgit

swh/lister/cgit/tests/test_lister.py

swh/lister/cgit/tests/test_tasks.py

cgit: Compute origin urls out of a base git url when provided.ClosedPublicActions

Details

Diff Detail

Event Timeline

Patch application report for D4968 (id=17741)

Changes applied before test

Patch application report for D4968 (id=17742)

Changes applied before test

Patch application report for D4968 (id=17743)

Changes applied before test

Patch application report for D4968 (id=17749)

Changes applied before test

Patch application report for D4968 (id=17750)

Changes applied before test

Revision ContentsChangeset List

Diff 17749

swh/lister/cgit/lister.py

swh/lister/cgit/tasks.py

swh/lister/cgit/tests/data/https_git.baserock.org/cgit

swh/lister/cgit/tests/data/https_git.eclipse.org/c

swh/lister/cgit/tests/data/https_jff.email/cgit

swh/lister/cgit/tests/test_lister.py

swh/lister/cgit/tests/test_tasks.py

cgit: Compute origin urls out of a base git url when provided.
ClosedPublic
Actions

Revision Contents
Changeset List