Fix revision extra header value type
ClosedPublic
Actions

Authored by anlambert on Sep 10 2019, 4:31 PM.

Details

Reviewers

olasd
vlorentz

Commits

rDLDHG2e0f4708f134: loader: Fix revision extra header value type

Summary

Fix issue introduced in D1507, extra header values must be of bytes type and not str one.

Related D1976

Diff Detail

Repository

rDLDHG Mercurial 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 10 2019, 4:31 PM

Build is green
See https://jenkins.softwareheritage.org/job/DLDHG/job/tox/31/ for more details.

Harbormaster completed remote builds in B7751: Diff 6665.Sep 10 2019, 4:32 PM

vlorentz added inline comments.Sep 10 2019, 4:33 PM

swh/loader/mercurial/loader.py
435	Why not hash_to_bytes?
swh/loader/mercurial/tests/test_loader.py
314	if you want to go the bytehex way, you only need `.decode('ascii')`

anlambert added inline comments.Sep 10 2019, 4:39 PM

swh/loader/mercurial/loader.py
435	@olasd suggested to use this function on `#swh-devel`
swh/loader/mercurial/tests/test_loader.py
314	Ack

Decode from ascii in updated test

Build is green
See https://jenkins.softwareheritage.org/job/DLDHG/job/tox/32/ for more details.

Harbormaster completed remote builds in B7752: Diff 6666.Sep 10 2019, 4:41 PM

I'm still not quite sure why we're translating this data specifically (and nothing else), but not to the point that I want to block this.

My feeling is that this is working around a presentation issue, and that if anything, we should be teaching the frontend how to interpret and present the raw value instead.

The whole logic of splitting the extra data on nul bytes will also yield surprising results if the transplant references a nodeid with a nul byte in it; but that's another issue altogether.

This revision is now accepted and ready to land.Sep 10 2019, 5:45 PM

Rebase

This revision was landed with ongoing or failed builds.Sep 10 2019, 6:12 PM

Closed by commit rDLDHG2e0f4708f134: loader: Fix revision extra header value type (authored by anlambert). · Explain Why

This revision was automatically updated to reflect the committed changes.

Build is green
See https://jenkins.softwareheritage.org/job/DLDHG/job/tox/36/ for more details.

Harbormaster completed remote builds in B7757: Diff 6671.Sep 10 2019, 6:13 PM

I'm still not quite sure why we're translating this data specifically (and nothing else), but not to the point that I want to block this.

I think I was misled by the webapp conversion code for revision metadata in a sense that metadata should be JSON convertible.

My feeling is that this is working around a presentation issue, and that if anything, we should be teaching the frontend how to interpret and present the raw value instead.

As revision metadata already contain Mercurial node ids in hexadecimal format, I thought it will be better to get the same representation for the transplant_source extra
(which is the only ChangesetExtra storing a nodeid in binary format) instead of patching the conversion code of the webapp.

The whole logic of splitting the extra data on nul bytes will also yield surprising results if the transplant references a nodeid with a nul byte in it; but that's another issue altogether.

Hopefully, this mercurial feature does not seem widely used. Nevertheless, further investigations are needed to check if we can encounter that corner case.