Page MenuHomeSoftware Heritage
Differential D4216

add swh-hg-identify a cli to identify hg objects
ClosedPublic
Actions

Authored by acezar on Oct 9 2020, 12:40 PM.

Details

Reviewers
marmoute
ardumont
douardda
Group Reviewers
Reviewers
Commits
rDLDHGa2e9cf16919a: add swh-hg-identify a cli to identify hg objects
Summary

Depends on D4193

Test Plan

tox

Diff Detail

Repository
rDLDHG Mercurial loader
Branch
identify
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 16319
Build 25121: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 25120: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
acezar added a comment.Oct 19 2020, 6:10 PM

Add tests using an existing repository whose top level hashes are already known

acezar marked 4 inline comments as done.Oct 19 2020, 6:11 PM
swh-public-ci added a comment.Oct 19 2020, 6:11 PM

Build has FAILED

Patch application report for D4216 (id=15231)

Rebasing onto bd914dec39...

Current branch diff-target is up to date.
Changes applied before test
commit 25cd91475dc038d69c5bcc64d6e0ce586dc23c6a
Author: Antoine Cezar <antoine.cezar@octobus.net>
Date:   Thu Oct 8 18:07:50 2020 +0200

    add swh-hg-identify a cli to identify hg objects

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

Harbormaster failed remote builds in B16464: Diff 15231!Oct 19 2020, 6:11 PM
ardumont added inline comments.Oct 19 2020, 6:12 PM
setup.py
57

thanks for the clarification ;)

acezar updated this revision to Diff 15232.Oct 19 2020, 6:19 PM

Last minute changes before push is a good way to break working tests ^_^. Fixed

swh-public-ci added a comment.Oct 19 2020, 6:20 PM

Build has FAILED

Patch application report for D4216 (id=15232)

Rebasing onto bd914dec39...

Current branch diff-target is up to date.
Changes applied before test
commit b543125f433ab3022cc80c5b76e3a2eab0162b61
Author: Antoine Cezar <antoine.cezar@octobus.net>
Date:   Thu Oct 8 18:07:50 2020 +0200

    add swh-hg-identify a cli to identify hg objects

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

Harbormaster failed remote builds in B16465: Diff 15232!Oct 19 2020, 6:20 PM
acezar added a child revision: D4311: Add testing repository builder.Oct 19 2020, 10:30 PM
ardumont added a comment.Oct 21 2020, 9:44 AM

Thanks for adding tests. It would be best that they go green now ;)

The idea was to lift jenkins' coverage report which alongside the diff
can show us what coverage is missing here (if any). The diff is large
enough that this kind of help is greatly appreciated for reviewers.

Nonetheless, I had a pass to your code and added some remarks or questions.

Hope this helps,

swh/loader/mercurial/identify.py
62

you can use docstring """raw bytes...""" just below the instance variable.

72

extra space.

112
fields["description"] = description
return HgRevision(**fields)

?

242

I recall the hgtags can be noisy in its content (possibly due to merge badly dealt with by users).
The following might interest you

Related to T970 rDLDHGe0b48c6c6e9aa3329c9058e1a39da38ff801714a

251

extra space

290

might as well use swh.model.identifiers.swhid function [1] instead of hard-coding values:

even if that's giving a mouthful

from swh.model.identifiers import swhid
return f"{swhid("directory", self.directory_swhid.hex())}\t{self.node_id.decode()}"

(or use an intermediary variable, as you wish...)

[1] https://forge.softwareheritage.org/source/swh-model/browse/master/swh/model/identifiers.py$767-772

294

same

326

curious me, is that a form of destructuring to say, give me the first element from the result, and discard the rest?
(without having to mention how many "rest" there is that is)

it looks like it, TIL ;)

(oh and that works because we are dealing with NamedTuple, neat)

350

same about the model swhid function use.

394

snapshot identifiers.

you might want to align to use identities instead of identifiers as you use elsewhere or use identifiers everywhere... ;)

433

as a heads up, I recall some modifications is currently happening on the module swh.model.identifiers. [1]
We are moving away from calling it directly.

To avoid some breakage on this in a near future, prefer using the swh data model object instead.

from swh.model.model import Snapshot, SnapshotBranch

branches[b"HEAD"] = SnapshotBranch(target=tip.branch(), target_type=TargetType.ALIAS)
...
# adapt the other 2 loops with this ^
...

return Snapshot(branches=branches).id

That remark is also valid for revision_identifier and release_identifier (earlier), it only hit me now ¯\_(ツ)_/¯

[1] T2715, specifically T2713

460

same use swhid function.

marmoute requested changes to this revision.Oct 21 2020, 9:55 AM
marmoute added inline comments.
swh/loader/mercurial/identify.py
242

that same tag name can appears multiple time in the tag file. In this case, the latest value should be used. The previous entries are here to indicate which older value have been overwritten.

(We should add a case of this in the new tests (when it gets to the new tests))

This revision now requires changes to proceed.Oct 21 2020, 9:55 AM
acezar updated this revision to Diff 15267.Oct 21 2020, 11:16 AM

Followup

Note that CI won't pass until D4193 is merged.
Directory identities rely on the added --exclude of swh-identify

swh-public-ci added a comment.Oct 21 2020, 11:17 AM

Build has FAILED

Patch application report for D4216 (id=15267)

Rebasing onto bd914dec39...

Current branch diff-target is up to date.
Changes applied before test
commit 67647aeba8573a720cb53f55ed25a9a318f882b0
Author: Antoine Cezar <antoine.cezar@octobus.net>
Date:   Thu Oct 8 18:07:50 2020 +0200

    add swh-hg-identify a cli to identify hg objects

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

Harbormaster failed remote builds in B16500: Diff 15267!Oct 21 2020, 11:17 AM
acezar marked 11 inline comments as done.Oct 21 2020, 11:20 AM
acezar added inline comments.
swh/loader/mercurial/identify.py
326

Made this cleared and not relying on the field declaration order.

acezar updated this revision to Diff 15269.Oct 21 2020, 11:35 AM
acezar marked an inline comment as done.

Followup for tag parsing

swh-public-ci added a comment.Oct 21 2020, 11:36 AM

Build has FAILED

Patch application report for D4216 (id=15269)

Rebasing onto bd914dec39...

Current branch diff-target is up to date.
Changes applied before test
commit c3d959fe1cf5bee461ff7366697f907a578d9288
Author: Antoine Cezar <antoine.cezar@octobus.net>
Date:   Thu Oct 8 18:07:50 2020 +0200

    add swh-hg-identify a cli to identify hg objects

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

Harbormaster failed remote builds in B16502: Diff 15269!Oct 21 2020, 11:36 AM
acezar marked 2 inline comments as done.Oct 21 2020, 11:37 AM
ardumont edited the test plan for this revision. (Show Details)Oct 21 2020, 11:57 AM
ardumont accepted this revision.EditedOct 21 2020, 12:02 PM
ardumont edited the test plan for this revision. (Show Details)

Thanks!

Overall lgtm.

Don't land this yet though.
Let's wait for @marmoute to validate it as well (he started the review ;)

Let's also wait for D4193 to be accepted, landed and the model to be tagged
Then we shall be able to trigger back the build here to check the coverage.

(i've updated the test plan to mention the current limitation on D4193 which i forgot prior to my remark on green tests ¯\_(ツ)_/¯)

Cheers,

swh/loader/mercurial/identify.py
197

Note to self, add the hg runtime dependency in the debian branch (if not there already, don't remember).

acezar updated this revision to Diff 15403.Oct 26 2020, 5:00 PM

Trigger CI, now that D4193 has landed

swh-public-ci added a comment.Oct 26 2020, 5:01 PM

Build has FAILED

Patch application report for D4216 (id=15403)

Rebasing onto bd914dec39...

Current branch diff-target is up to date.
Changes applied before test
commit c3d959fe1cf5bee461ff7366697f907a578d9288
Author: Antoine Cezar <antoine.cezar@octobus.net>
Date:   Thu Oct 8 18:07:50 2020 +0200

    add swh-hg-identify a cli to identify hg objects

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

Harbormaster failed remote builds in B16632: Diff 15403!Oct 26 2020, 5:01 PM
acezar updated this revision to Diff 15420.Oct 27 2020, 11:15 AM

Trigger CI

swh-public-ci added a comment.Oct 27 2020, 11:16 AM

Build has FAILED

Patch application report for D4216 (id=15420)

Rebasing onto bd914dec39...

Current branch diff-target is up to date.
Changes applied before test
commit c3d959fe1cf5bee461ff7366697f907a578d9288
Author: Antoine Cezar <antoine.cezar@octobus.net>
Date:   Thu Oct 8 18:07:50 2020 +0200

    add swh-hg-identify a cli to identify hg objects

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

Harbormaster failed remote builds in B16647: Diff 15420!Oct 27 2020, 11:16 AM
acezar updated this revision to Diff 15447.Oct 27 2020, 4:46 PM

Update

swh-public-ci added a comment.Oct 27 2020, 4:47 PM

Build has FAILED

Patch application report for D4216 (id=15447)

Rebasing onto bd914dec39...

Current branch diff-target is up to date.
Changes applied before test
commit c3d959fe1cf5bee461ff7366697f907a578d9288
Author: Antoine Cezar <antoine.cezar@octobus.net>
Date:   Thu Oct 8 18:07:50 2020 +0200

    add swh-hg-identify a cli to identify hg objects

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

Harbormaster failed remote builds in B16675: Diff 15447!Oct 27 2020, 4:47 PM
acezar updated this revision to Diff 15451.Oct 27 2020, 4:49 PM

Update

swh-public-ci added a comment.Oct 27 2020, 4:50 PM

Build has FAILED

Patch application report for D4216 (id=15451)

Rebasing onto bd914dec39...

Current branch diff-target is up to date.
Changes applied before test
commit e75b99287ba58bdbc6cb95703f11303c561a8f34
Author: Antoine Cezar <antoine.cezar@octobus.net>
Date:   Thu Oct 8 18:07:50 2020 +0200

    add swh-hg-identify a cli to identify hg objects

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

Harbormaster failed remote builds in B16679: Diff 15451!Oct 27 2020, 4:50 PM
acezar marked an inline comment as done.Oct 28 2020, 10:33 AM
acezar updated this revision to Diff 15469.Oct 28 2020, 10:46 AM

Trigger CI

swh-public-ci added a comment.Oct 28 2020, 10:47 AM

Build has FAILED

Patch application report for D4216 (id=15469)

Rebasing onto bd914dec39...

Current branch diff-target is up to date.
Changes applied before test
commit e75b99287ba58bdbc6cb95703f11303c561a8f34
Author: Antoine Cezar <antoine.cezar@octobus.net>
Date:   Thu Oct 8 18:07:50 2020 +0200

    add swh-hg-identify a cli to identify hg objects

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

Harbormaster failed remote builds in B16692: Diff 15469!Oct 28 2020, 10:47 AM
douardda added a comment.Oct 28 2020, 11:14 AM

So the test fails on jenkins because they use the hg command from the system (since mercurial is oddly enough, not a dependency of swh-loader-mercurial) and on stretch, mercurial is 4.8
Using mercurial 5.5 is ok.

So mercurial needs to be added to the requirements-test.txt file.

swh/loader/mercurial/identify.py
306

Why do you use the swh-identify command in a subprocess rather than just using the identify_object function from swh.model.cli?

douardda requested changes to this revision.Oct 28 2020, 11:14 AM

see comments

This revision now requires changes to proceed.Oct 28 2020, 11:14 AM
acezar updated this revision to Diff 15471.Oct 28 2020, 12:21 PM
acezar marked an inline comment as done.
  • Add mercurial to requirements.txt
  • use swh.model.cli.identify_object instead of swh-identify subprocess
swh-public-ci added a comment.Oct 28 2020, 12:23 PM

Build is green

Patch application report for D4216 (id=15471)

Rebasing onto bd914dec39...

Current branch diff-target is up to date.
Changes applied before test
commit 00b709bae8c01854fa9eb37e2b5d9989fa68a17d
Author: Antoine Cezar <antoine.cezar@octobus.net>
Date:   Thu Oct 8 18:07:50 2020 +0200

    add swh-hg-identify a cli to identify hg objects

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

Harbormaster completed remote builds in B16694: Diff 15471.Oct 28 2020, 12:23 PM
acezar marked an inline comment as done.Oct 28 2020, 12:44 PM
ardumont edited the test plan for this revision. (Show Details)Oct 28 2020, 1:10 PM
douardda accepted this revision.Oct 30 2020, 5:03 PM
douardda added a comment.Nov 4 2020, 9:22 AM

Any reason for not landing this? (@marmoute ? do you keep your request for changes?)

marmoute accepted this revision.Nov 9 2020, 7:30 PM

Looks overall good. I made various feedback that could be included in this patch or done as follow up.

swh/loader/mercurial/identify.py
150

"Actual" feel weird here. Maybe "to match historical implementation"

234

Can you explain the "\n\n" part in a small comment ?

255

Use "node-id" instead of revision here to make it clearer that we are talking about binary node ID

259

What does "at the current revision" means in this context ?

swh/loader/mercurial/tests/test_identify.py
35 ↗(On Diff #15471)

Should we put this in a level modul """text""" constant to avoid having to build it ?

58 ↗(On Diff #15471)

same feedback.

91 ↗(On Diff #15471)

idem

This revision is now accepted and ready to land.Nov 9 2020, 7:30 PM
acezar updated this revision to Diff 15845.Nov 12 2020, 3:17 PM
acezar edited the test plan for this revision. (Show Details)

Addresses:

swh-public-ci added a comment.Nov 12 2020, 3:18 PM

Build is green

Patch application report for D4216 (id=15845)

Rebasing onto bd914dec39...

Current branch diff-target is up to date.
Changes applied before test
commit a2e9cf16919a5f81a06f955a533a254a9b3c9689
Author: Antoine Cezar <antoine.cezar@octobus.net>
Date:   Thu Oct 8 18:07:50 2020 +0200

    add swh-hg-identify a cli to identify hg objects

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

Harbormaster completed remote builds in B17056: Diff 15845.Nov 12 2020, 3:18 PM
Closed by commit rDLDHGa2e9cf16919a: add swh-hg-identify a cli to identify hg objects (authored by acezar). · Explain WhyNov 27 2020, 5:18 PM
This revision was automatically updated to reflect the committed changes.
acezar added a commit: rDLDHGa2e9cf16919a: add swh-hg-identify a cli to identify hg objects.

Revision Contents
Changeset List

PathSize
2 lines
swh/
loader/
mercurial/
494 lines

Diff 15093

View Options

setup.py

Loading...
View Options

swh/loader/mercurial/identify.py

Loading...
About · Developer information · Legal