Page MenuHomeSoftware Heritage
Differential D5652

identifiers: Expose git_object instead of manifest
ClosedPublic
Actions

Authored by vlorentz on Apr 30 2021, 12:25 PM.

Details

Reviewers
olasd
Group Reviewers
Reviewers
Maniphest Tasks
T843: Vault: Add a "git bare" tarball cooker
Commits
rDMOD8c904dc6af61: identifiers: Expose git_object instead of manifest
Summary

The git_object is what will be actually useful to the vault.

It's also easier to test, because test_identifier.py has the
entire git_object in its test data.

However, it makes the code messier, and it's quite a big change,
so I'm not sure it's worth it.
Instead, I could slightly rewrite the tests, and make the vault
re-add the header itself (but it would duplicate this code...)

Depends on D5650

Diff Detail

Event Timeline

vlorentz created this revision.Apr 30 2021, 12:25 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptApr 30 2021, 12:25 PM
swh-public-ci added a comment.Apr 30 2021, 12:26 PM

Build has FAILED

Patch application report for D5652 (id=20186)

Could not rebase; Attempt merge onto f7e9d5c290...

Updating f7e9d5c..eaa5daa
Fast-forward
 swh/model/hashutil.py               |  59 +++----------------
 swh/model/identifiers.py            | 113 +++++++++++++++++++++++-------------
 swh/model/tests/test_identifiers.py |  56 +++++++++++++-----
 3 files changed, 124 insertions(+), 104 deletions(-)
Changes applied before test
commit eaa5daa3d5f225f6a4e9894f8b73798ea3e75a48
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Fri Apr 30 12:24:00 2021 +0200

    identifiers: Expose git_object instead of manifest
    
    The git_object is what will be actually useful to the vault.
    
    It's also easier to test, because test_identifier.py has the
    entire git_object in its test data.

commit 3a4f484feaccd569a0b9dc37531d7af50bc3871e
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Fri Apr 30 11:26:32 2021 +0200

    identifiers: Expose manifest computation
    
    Before this commit, manifests were only computed internally before
    hashing, so they were not available to outside modules.
    
    This makes testing the module very painful, because identifier
    functions can only be tested by checking the hash; so test failures
    did not show mismatches between the computed manifest and
    the expected one.
    
    Additionally, the 'git bare cooker' of the vault is likely to use
    these as well, as it needs to format git objects in the same format.

Link to build: https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/326/
See console output for more information: https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/326/console

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 30 2021, 12:26 PM
Harbormaster failed remote builds in B21176: Diff 20186!
vlorentz updated this revision to Diff 20191.Apr 30 2021, 1:26 PM

fix manifest formatting

swh-public-ci added a comment.Apr 30 2021, 1:27 PM

Build has FAILED

Patch application report for D5652 (id=20191)

Could not rebase; Attempt merge onto f7e9d5c290...

Updating f7e9d5c..8062ed9
Fast-forward
 swh/model/hashutil.py               |  59 +++----------------
 swh/model/identifiers.py            | 113 +++++++++++++++++++++++-------------
 swh/model/tests/test_identifiers.py |  56 +++++++++++++-----
 3 files changed, 124 insertions(+), 104 deletions(-)
Changes applied before test
commit 8062ed9a4bc314ee019765033bf985e30be579c2
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Fri Apr 30 12:24:00 2021 +0200

    identifiers: Expose git_object instead of manifest
    
    The git_object is what will be actually useful to the vault.
    
    It's also easier to test, because test_identifier.py has the
    entire git_object in its test data.

commit 3a4f484feaccd569a0b9dc37531d7af50bc3871e
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Fri Apr 30 11:26:32 2021 +0200

    identifiers: Expose manifest computation
    
    Before this commit, manifests were only computed internally before
    hashing, so they were not available to outside modules.
    
    This makes testing the module very painful, because identifier
    functions can only be tested by checking the hash; so test failures
    did not show mismatches between the computed manifest and
    the expected one.
    
    Additionally, the 'git bare cooker' of the vault is likely to use
    these as well, as it needs to format git objects in the same format.

Link to build: https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/329/
See console output for more information: https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/329/console

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 30 2021, 1:27 PM
Harbormaster failed remote builds in B21180: Diff 20191!
vlorentz updated this revision to Diff 20192.Apr 30 2021, 1:28 PM

restore check of git_type.

swh-public-ci added a comment.Apr 30 2021, 1:29 PM

Build has FAILED

Patch application report for D5652 (id=20192)

Could not rebase; Attempt merge onto f7e9d5c290...

Updating f7e9d5c..64829e0
Fast-forward
 swh/model/hashutil.py               |  71 ++++++++--------------
 swh/model/identifiers.py            | 113 +++++++++++++++++++++++-------------
 swh/model/tests/test_identifiers.py |  56 +++++++++++++-----
 3 files changed, 138 insertions(+), 102 deletions(-)
Changes applied before test
commit 64829e0d9ea398bd2bf289bf1c57ea1fddd7a6af
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Fri Apr 30 12:24:00 2021 +0200

    identifiers: Expose git_object instead of manifest
    
    The git_object is what will be actually useful to the vault.
    
    It's also easier to test, because test_identifier.py has the
    entire git_object in its test data.

commit 3a4f484feaccd569a0b9dc37531d7af50bc3871e
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Fri Apr 30 11:26:32 2021 +0200

    identifiers: Expose manifest computation
    
    Before this commit, manifests were only computed internally before
    hashing, so they were not available to outside modules.
    
    This makes testing the module very painful, because identifier
    functions can only be tested by checking the hash; so test failures
    did not show mismatches between the computed manifest and
    the expected one.
    
    Additionally, the 'git bare cooker' of the vault is likely to use
    these as well, as it needs to format git objects in the same format.

Link to build: https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/330/
See console output for more information: https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/330/console

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 30 2021, 1:29 PM
Harbormaster failed remote builds in B21181: Diff 20192!
vlorentz requested review of this revision.Apr 30 2021, 1:51 PM
vlorentz edited the test plan for this revision. (Show Details)
olasd mentioned this in D5650: identifiers: Expose manifest computation.Apr 30 2021, 5:39 PM
vlorentz updated this revision to Diff 20204.Apr 30 2021, 6:27 PM

rebase

swh-public-ci added a comment.Apr 30 2021, 6:31 PM

Build has FAILED

Patch application report for D5652 (id=20204)

Could not rebase; Attempt merge onto df036ef1c3...

Updating df036ef..60b138a
Fast-forward
 swh/model/hashutil.py               |  71 ++++++++--------------
 swh/model/identifiers.py            | 113 +++++++++++++++++++++++-------------
 swh/model/tests/test_identifiers.py |  56 +++++++++++++-----
 3 files changed, 138 insertions(+), 102 deletions(-)
Changes applied before test
commit 60b138a0e843ce22a2efa7ee6ddef0ec12ea4a4f
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Fri Apr 30 12:24:00 2021 +0200

    identifiers: Expose git_object instead of manifest
    
    The git_object is what will be actually useful to the vault.
    
    It's also easier to test, because test_identifier.py has the
    entire git_object in its test data.

commit 8221147ce6e74b3f685baead1906675b3454065c
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Fri Apr 30 11:26:32 2021 +0200

    identifiers: Expose manifest computation
    
    Before this commit, manifests were only computed internally before
    hashing, so they were not available to outside modules.
    
    This makes testing the module very painful, because identifier
    functions can only be tested by checking the hash; so test failures
    did not show mismatches between the computed manifest and
    the expected one.
    
    Additionally, the 'git bare cooker' of the vault is likely to use
    these as well, as it needs to format git objects in the same format.

Link to build: https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/332/
See console output for more information: https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/332/console

Harbormaster failed remote builds in B21194: Diff 20204!Apr 30 2021, 6:31 PM
vlorentz added a child revision: D5656: Add a naive git bare cooker.May 6 2021, 2:46 PM
vlorentz added a task: T843: Vault: Add a "git bare" tarball cooker.May 10 2021, 9:48 AM
vlorentz edited the test plan for this revision. (Show Details)May 11 2021, 10:34 AM
vlorentz updated this revision to Diff 20476.May 11 2021, 10:34 AM

rebase + restore hash_git_data

swh-public-ci added a comment.May 11 2021, 10:38 AM

Build is green

Patch application report for D5652 (id=20476)

Could not rebase; Attempt merge onto 31cb72e9f0...

Updating 31cb72e..6ca45e4
Fast-forward
 swh/model/hashutil.py               |  55 ++++++++----------
 swh/model/identifiers.py            | 113 +++++++++++++++++++++++-------------
 swh/model/tests/test_identifiers.py |  56 +++++++++++++-----
 3 files changed, 140 insertions(+), 84 deletions(-)
Changes applied before test
commit 6ca45e467df6a04a18c09e250ec030cc5598df7d
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Fri Apr 30 12:24:00 2021 +0200

    identifiers: Expose git_object instead of manifest
    
    The git_object is what will be actually useful to the vault.
    
    It's also easier to test, because test_identifier.py has the
    entire git_object in its test data.

commit 523ab642bb48491f8e09fb61cc32a9e11420b556
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Fri Apr 30 11:26:32 2021 +0200

    identifiers: Expose manifest computation
    
    Before this commit, manifests were only computed internally before
    hashing, so they were not available to outside modules.
    
    This makes testing the module very painful, because identifier
    functions can only be tested by checking the hash; so test failures
    did not show mismatches between the computed manifest and
    the expected one.
    
    Additionally, the 'git bare cooker' of the vault is likely to use
    these as well, as it needs to format git objects in the same format.

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

Harbormaster completed remote builds in B21449: Diff 20476.May 11 2021, 10:38 AM
olasd added a subscriber: olasd.May 11 2021, 10:43 AM

Overall, considering that the manifests have never been used anywhere, and that the headers don't make them harder to unpack (on the contrary, they make them less ambiguous), I don't mind having them fully compatible with "git objects".

I've made a bunch of stylistic comments but I think the gist of this change is fine.

swh/model/identifiers.py
32

Importing underscore functions looks like poor style. I guess the git_header function could just be public (and be called git_object_header).

159

should probably just be hashlib.new

425–428

Maybe call it format_kv_git_object (or format_git_object_with_headers) by opposition to the directory object which is special.

468

format_git_object_from_parts? from_chunks?

vlorentz updated this revision to Diff 20479.May 11 2021, 11:01 AM

apply comments:

  • rename _git_header to git_object_header
  • replace _git_object_to_identifier_str with direct call to hashlib.new, and remove import of _new_hash
  • rename format_git_object to format_git_object_from_headers
  • rename format_list_git_object to format_git_object_from_parts
vlorentz added inline comments.May 11 2021, 11:02 AM
swh/model/identifiers.py
425–428

I went for format_git_object_from_headers instead, for consistency with format_git_object_from_parts.

swh-public-ci added a comment.May 11 2021, 11:03 AM

Build is green

Patch application report for D5652 (id=20479)

Could not rebase; Attempt merge onto 31cb72e9f0...

Updating 31cb72e..8c904dc
Fast-forward
 swh/model/hashutil.py               |  55 +++++++++---------
 swh/model/identifiers.py            | 107 ++++++++++++++++++++++--------------
 swh/model/tests/test_identifiers.py |  56 ++++++++++++++-----
 3 files changed, 135 insertions(+), 83 deletions(-)
Changes applied before test
commit 8c904dc6af617d595c1295b80c3976ea1861b92f
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Fri Apr 30 12:24:00 2021 +0200

    identifiers: Expose git_object instead of manifest
    
    The git_object is what will be actually useful to the vault.
    
    It's also easier to test, because test_identifier.py has the
    entire git_object in its test data.

commit 523ab642bb48491f8e09fb61cc32a9e11420b556
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Fri Apr 30 11:26:32 2021 +0200

    identifiers: Expose manifest computation
    
    Before this commit, manifests were only computed internally before
    hashing, so they were not available to outside modules.
    
    This makes testing the module very painful, because identifier
    functions can only be tested by checking the hash; so test failures
    did not show mismatches between the computed manifest and
    the expected one.
    
    Additionally, the 'git bare cooker' of the vault is likely to use
    these as well, as it needs to format git objects in the same format.

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

Harbormaster completed remote builds in B21452: Diff 20479.May 11 2021, 11:03 AM
olasd accepted this revision.May 11 2021, 11:31 AM
olasd added inline comments.
swh/model/identifiers.py
425–428

Thing is, there's headers *and* a message (which is why I suggested with). But hey, I like consistency as much as the next person...

This revision is now accepted and ready to land.May 11 2021, 11:31 AM
Closed by commit rDMOD8c904dc6af61: identifiers: Expose git_object instead of manifest (authored by vlorentz). · Explain WhyMay 11 2021, 11:36 AM
This revision was automatically updated to reflect the committed changes.
vlorentz added a commit: rDMOD8c904dc6af61: identifiers: Expose git_object instead of manifest.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
swh/
model/
55 lines75%
Loading...
85 lines44%
Loading...
tests/
56 lines0%
Loading...

Diff 20476

View Options

swh/model/hashutil.py

Loading...
View Options

swh/model/identifiers.py

Loading...
View Options

swh/model/tests/test_identifiers.py

Loading...
About · Developer information · Legal