Page MenuHomeSoftware Heritage
Differential D6342

loader: Add support for dumb HTTP transfer protocol
ClosedPublic
Actions

Authored by anlambert on Sep 24 2021, 2:42 PM.

Details

Reviewers
ardumont
vlorentz
Group Reviewers
Reviewers
Maniphest Tasks
T2489: Git origin without smart transfer protocol support cannot be loaded
Commits
rDLDGd3976ca6504c: loader: Add support for dumb HTTP transfer protocol
Summary

Git supports two HTTP based transfer protocols to exchange data
between two repositories: the dumb protocol and the smart protocol.

Nowadays, the smart protocol is a common method of transferring
data because it is more efficient but there is still some git
servers in the wild (mostly cgit) that only support the dumb protocol.

Unfortunately the dulwich package does not support such protocol
so this kind of git repository could not be loaded into the archive.

That commit adds support to load such git repository by fetching
objects according to the dumb HTTP transfer protocol specification.

Below are some useful resources on the subject:

I have successfully tested the loading with numerous git repositories
only supporting dumb protocol, below are some of them found by inspecting
the failed save code now requests for git in production:

Debian packaging has also been successfully tested, git must added
as build dependency though as it is used in newly added test suite.

Related to T2489

Test Plan

A new test suite for the dumb protocol support has been added.
The sample repository used in tests is bare cloned and served
by a local HTTP server as specified by the dumb protocol.

Diff Detail

Repository
rDLDG Git loader
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

anlambert created this revision.Sep 24 2021, 2:42 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptSep 24 2021, 2:42 PM
swh-public-ci added a comment.Sep 24 2021, 2:44 PM

Build is green

Patch application report for D6342 (id=23057)

Rebasing onto 05d6d762d0...

Current branch diff-target is up to date.
Changes applied before test
commit 6fa2636f9f99890f83b57baedd0cbb9495632c1c
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Thu Sep 16 16:00:15 2021 +0200

    loader: Add support for dumb HTTP transfer protocol
    
    Git supports two HTTP based transfer protocols to exchange data
    between two repositories: the dumb protocol and the smart protocol.
    
    Nowadays, the smart protocol is a common method of transferring
    data because it is more efficient but there is still some git
    servers in the wild that only support the dumb protocol.
    
    Unfortunately the dulwich package does not support such protocol
    so this kind of git repository could not be loaded into the archive.
    
    That commit adds support to load such git repository by fetching
    objects according to the dumb HTTP transfer protocol specification.
    
    Related to T2489

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

Harbormaster completed remote builds in B23984: Diff 23057.Sep 24 2021, 2:44 PM
anlambert requested review of this revision.Sep 24 2021, 2:44 PM
anlambert edited the test plan for this revision. (Show Details)Sep 24 2021, 2:51 PM
vlorentz added a subscriber: vlorentz.Sep 24 2021, 2:54 PM
vlorentz added inline comments.
swh/loader/git/loader.py
256–258

Why move it in the try block?

259–262

Can you remove this from the try block?

swh/loader/git/tests/test_loader.py
161–170

I still believe it would be simpler to patch DumbHttpGitClient to return objects directly instead of spawning an HTTP server

anlambert added inline comments.Sep 24 2021, 3:03 PM
swh/loader/git/loader.py
256–258

ack, will move above

259–262

sure, will move it after the exceptions handling

swh/loader/git/tests/test_loader.py
161–170

I think it is simpler to proceed like this, I prefer to avoid mocking things when it is possible.
Plus tests performance are more than acceptable.

vlorentz added a comment.Sep 24 2021, 3:08 PM

LGTM overall

swh/loader/git/dumb.py
97

Any reason for it to be a BFS rather than DFS? deque is a linked list, so not very efficient; you could use a simple list as a stack instead.
(plus, you can spare the casts)

Probably doesn't matter in practice though.

103–107
121–122

I'd do it like this, but it doesn't really matter

163

is it always UTF-8?

194–195

what is this?

swh/loader/git/loader.py
272–285
swh/loader/git/tests/test_loader.py
161–170

did you check it would work in Debian tests? They block networking so it might affect it.

anlambert added inline comments.Sep 24 2021, 3:24 PM
swh/loader/git/dumb.py
97

No particular reason, I pick the first graph traversal I had in mind.
I will check the DFS approach then.

103–107

better indeed, thanks

121–122

Ack, will update

163

Yes, this is the kind of response we get for the packs list:

anlambert@carnavalet:/tmp$ curl -i https://git.openembedded.org/openembedded-core/objects/info/packs
HTTP/1.1 200 OK
Server: nginx/1.10.3 (Ubuntu)
Date: Fri, 24 Sep 2021 13:15:23 GMT
Content-Type: text/plain; charset=UTF-8
Transfer-Encoding: chunked
Connection: keep-alive
Content-Disposition: inline; filename="objects/info/packs"
Last-Modified: Fri, 24 Sep 2021 13:15:23 GMT
Expires: Fri, 24 Sep 2021 13:16:23 GMT

P pack-5422adfb2f751e0a68908e78bcbc2b64da05aeee.pack
P pack-6eeecf497baac9fbf038439a4d6d744239f46c5d.pack
P pack-8cc751bf481805b703aa952a1895188d1f17ab76.pack
P pack-94073fd7b717046e62ee98407afec2d7053df264.pack
P pack-c95f4d0ab59a40084209281317356f7b32aca171.pack
P pack-c97afddc6240cc65acf0bb0b8deed553346c263e.pack
P pack-7eacd96820ba5e7d59e12b2cd5bb6ad8cefed593.pack
P pack-46515b894bd9e84e83ccb6f99ef29b0a7bf41611.pack
P pack-dada3bb0e007956d6c91c6bd8d037a2f090d7a36.pack
P pack-1195ec9c538bff2fe8f09dcd2c7a5d8e1663725b.pack
P pack-1cd35da490cef9ec46dfe9a9696e4aded2a3e383.pack
P pack-fe0be52545a2f95de32dc0fe613bc939cc75cca0.pack
P pack-f585f3a38ce7836bbe916fa92c16433812c0470d.pack
P pack-e51701de8cf701947763209e24bbf104e1bc135c.pack
P pack-1f3a65f2b481b6040978160e97739bac43b439cf.pack
P pack-c10b904d5c4c2ceeda4d2683e8f98090ae857c0f.pack
P pack-587bc9e2864af9411e0938ffe57fa1cb3bf332e9.pack
P pack-c0b1099e7faf2b4604b9ed09cb40d1b288aff562.pack
P pack-21ae990c3753469aca8531b66a5763d50a691382.pack
P pack-5a229741a8da560a00d713498ac3597d033b3f19.pack
P pack-b7d73c1882ff5c82d23906a6ff75b81fe54f96e3.pack
P pack-1027a60b270b168c29f12fc0041b840c29aaec46.pack
P pack-46e9b109657fbd4eca28bd5a4538f81d10f63257.pack
P pack-ff88065963b0bce2822f46913a7e42fb7beb3f7d.pack
P pack-da9b86e62250f1025d03bd987d7cea8036660ec4.pack
P pack-1c6de8793c2074b4429984d239cdc0533df19484.pack
P pack-a667d8977a2a4b2ce23117e156c3fd4a8a8adc5a.pack
P pack-04a9da669b4187a5bbe276434d69275f77a4b00f.pack
P pack-7061abeaab5c6bd5befcbe0e5b9456d98d57bae0.pack
P pack-018260388c9cdd5fbb1234212a6d0f100ea1e2c2.pack
P pack-a7a8db6093ca339ff83ff37782d9ffdff8e20a42.pack
P pack-7104febf3aa00b6f0b59aacb7ea9c2a54c87748e.pack
P pack-9a95f313831680977128ddfc5215e5f0c691e497.pack
P pack-dae1ea96cdd5fd421da7aa4bfc9dfdc187503c42.pack
P pack-2e28a3dbb0d35b9d766549dd45503c9b8a4a09ef.pack
P pack-8f94e403e4588588c8c04a9553e65b5663d3419b.pack
P pack-4fb16e27ea809417b3f125bbd0b9466f7e16a391.pack
194–195

A reference to an external tree object (git submodule), we must skip it as it is not hosted on the git server.

swh/loader/git/loader.py
272–285

No this must be kept that way.

Depending on dulwich version the dumb protocol can be detected during the call to fetch_pack without errors (upstream) or exceptions can be raised (dulwich version used in production, see sentry reports), so we must handle both cases.

swh/loader/git/tests/test_loader.py
161–170

Yes, of course.

vlorentz added inline comments.Sep 24 2021, 3:33 PM
swh/loader/git/dumb.py
163

alright

swh/loader/git/loader.py
272–285

Oh, I missed that. Could you add a comment to explain this?

anlambert added inline comments.Sep 24 2021, 3:36 PM
swh/loader/git/loader.py
272–285

There is already one in the exception block above, I will make it clearer.

anlambert updated this revision to Diff 23060.Sep 24 2021, 4:01 PM

Update:

  • address most of @vlorentz comments
  • use a DFS to walk on the commits graph instead of a BFS
  • improve some comments
  • add missing docstring for new test suite
swh-public-ci added a comment.Sep 24 2021, 4:03 PM

Build is green

Patch application report for D6342 (id=23060)

Rebasing onto 05d6d762d0...

Current branch diff-target is up to date.
Changes applied before test
commit 0d89e6c66ad4a31fc0b75bec7a9bcb881cbf32d6
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Thu Sep 16 16:00:15 2021 +0200

    loader: Add support for dumb HTTP transfer protocol
    
    Git supports two HTTP based transfer protocols to exchange data
    between two repositories: the dumb protocol and the smart protocol.
    
    Nowadays, the smart protocol is a common method of transferring
    data because it is more efficient but there is still some git
    servers in the wild that only support the dumb protocol.
    
    Unfortunately the dulwich package does not support such protocol
    so this kind of git repository could not be loaded into the archive.
    
    That commit adds support to load such git repository by fetching
    objects according to the dumb HTTP transfer protocol specification.
    
    Related to T2489

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

Harbormaster completed remote builds in B23987: Diff 23060.Sep 24 2021, 4:03 PM
ardumont accepted this revision as: ardumont.Sep 27 2021, 10:51 AM
ardumont added 1 blocking reviewer(s): Reviewers.
ardumont added a subscriber: ardumont.

lgtm

vlorentz added inline comments.Sep 27 2021, 1:25 PM
swh/loader/git/dumb.py
181–184

Can you add a test for this?

194–195

could you add a comment?

anlambert added inline comments.Sep 28 2021, 11:20 AM
swh/loader/git/dumb.py
181–184

It seems the bare clone of the test repository put all objects in a single pack file so that code is not covered.
I think I can serve the .git folder of the test repository, need to check.

194–195

sure

anlambert updated this revision to Diff 23125.Sep 28 2021, 1:48 PM

Update:

  • Increase tests coverage: handle bare repo without pack files and check dumb repo can still be loaded if dulwich raises an exception in fetch_pack
  • Improve comment
swh-public-ci added a comment.Sep 28 2021, 1:51 PM

Build is green

Patch application report for D6342 (id=23125)

Rebasing onto 05d6d762d0...

Current branch diff-target is up to date.
Changes applied before test
commit 87ff9be90567beec0bfcbefe39ed63907f1cbfbd
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Thu Sep 16 16:00:15 2021 +0200

    loader: Add support for dumb HTTP transfer protocol
    
    Git supports two HTTP based transfer protocols to exchange data
    between two repositories: the dumb protocol and the smart protocol.
    
    Nowadays, the smart protocol is a common method of transferring
    data because it is more efficient but there is still some git
    servers in the wild that only support the dumb protocol.
    
    Unfortunately the dulwich package does not support such protocol
    so this kind of git repository could not be loaded into the archive.
    
    That commit adds support to load such git repository by fetching
    objects according to the dumb HTTP transfer protocol specification.
    
    Related to T2489

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

Harbormaster completed remote builds in B24041: Diff 23125.Sep 28 2021, 1:51 PM
vlorentz added inline comments.Sep 28 2021, 2:08 PM
swh/loader/git/dumb.py
181–184

hmm you could try moving the packfile out of the repository, and use git unpack-objects

anlambert added inline comments.Sep 28 2021, 2:22 PM
swh/loader/git/dumb.py
181–184

I have implemented my first idea and it worked, so we now have a test suite processing pack files only and another one fetching objects from the objects/ folder.

vlorentz accepted this revision.Sep 28 2021, 2:39 PM
vlorentz added inline comments.
swh/loader/git/dumb.py
191–193

sorry, last nitpick

This revision is now accepted and ready to land.Sep 28 2021, 2:39 PM
anlambert updated this revision to Diff 23132.Sep 28 2021, 2:44 PM

Update: move comment inside if block

swh-public-ci added a comment.Sep 28 2021, 2:46 PM

Build is green

Patch application report for D6342 (id=23132)

Rebasing onto 05d6d762d0...

Current branch diff-target is up to date.
Changes applied before test
commit d3976ca6504c57d35467daf443d435f9d329f9ec
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Thu Sep 16 16:00:15 2021 +0200

    loader: Add support for dumb HTTP transfer protocol
    
    Git supports two HTTP based transfer protocols to exchange data
    between two repositories: the dumb protocol and the smart protocol.
    
    Nowadays, the smart protocol is a common method of transferring
    data because it is more efficient but there is still some git
    servers in the wild that only support the dumb protocol.
    
    Unfortunately the dulwich package does not support such protocol
    so this kind of git repository could not be loaded into the archive.
    
    That commit adds support to load such git repository by fetching
    objects according to the dumb HTTP transfer protocol specification.
    
    Related to T2489

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

Harbormaster completed remote builds in B24047: Diff 23132.Sep 28 2021, 2:46 PM
Closed by commit rDLDGd3976ca6504c: loader: Add support for dumb HTTP transfer protocol (authored by anlambert). · Explain WhySep 28 2021, 3:11 PM
This revision was automatically updated to reflect the committed changes.
anlambert added a commit: rDLDGd3976ca6504c: loader: Add support for dumb HTTP transfer protocol.

Revision Contents
Changeset List

PathSize
3 lines
swh/
loader/
git/
197 lines
67 lines
tests/
153 lines

Diff 23139

View Options

mypy.ini

Loading...
View Options

swh/loader/git/dumb.py

Loading...
View Options

swh/loader/git/loader.py

Loading...
View Options

swh/loader/git/tests/test_loader.py

Loading...
About · Developer information · Legal