I'm requesting some minor changes (+ some other changes to be submitted in a separate diff which I've noticed only now, sorry!).

awesome \o/
(diff accepted)

I'm accepting this diff, but note that I've added a few suggestions for improved language above. Please integrate them before this is final.

Wonderful, thanks for adding the order tests! LGTM.

thanks @KShivendu, this is a great start!

@zack
It is okay with you if I add it in the next diff? This one has become extremely long because of lots of build failures.

In D5990#154613, @zack wrote:

Can we have some documentation of the query language, included in this diff?
E.g., a file under docs/ which will then be rendered on docs.s.o as user documentation for how to use the query language.

Can we have some documentation of the query language, included in this diff?
E.g., a file under docs/ which will then be rendered on docs.s.o as user documentation for how to use the query language.

Thanks for this update, great work!

Looks great! I've noted down only some nits.

Please note down on the sides two remaining TODOs about all this:

• adding a test case with a deduplicated source tree, to make sure nodes that are deduplicated at the Merkle DAG level are present multiple times in the output
• adding a test case for a path that is not decodable in utf-8, to make sure it can be handled propertly

Thanks, both the general structure and implementation look OK.
I'm requesting changes to address two main issues:

only minor changes requested to the docstring on my part

In D5952#152586, @ardumont wrote:

I don't really have an ETA yet [1]. We are roughly 67% done for git and 84.6% for svn
[2]. For mercurial, it's not started as other blocking points are being worked on.
Bazaar and cvs origins are listed but we don't have any loader on that front yet.

Thanks a lot for this!

(3) should be ideally implemented in a way that guarantees that extid that were resolvable in previous versions of the mapping will always be resolvable in future versions

I don't understand. Option 3 is to remove relations between extids and SWHID, so it won't be resolvable anymore.

I've the feeling that option (1) will lead in the long run to an explosion on the size of the mapping which will make us eventually converge (slowly) toward option (3).

In D5899#150882, @DanSeraf wrote:

I'll wait @vlorentz and @olasd before to write the unit test (in the case they want to use a different approach)

@olasd @vlorentz: I've noted down only nits and "classics" in the above review. If you want to chime in on the approach (where the methods are, properties, caching, etc.) please do!

LGTM in general, but needs unit tests (in addition to the two nitpicks above about docstrings)

I think this also needs bumping the versioned dependency on swh-model (and a release of that).

Thanks @ardumont for following up to this task.

landed in d58bcb59a0999ae124de23db88fc9f73603d452a

Note: when this is (reasonably) done, we should document the addition of SourceForge to the archive coverage page at archive.s.o and also to the archive changelog.

Superseded by D5825. Abandoning this one.

LGTM, thanks !

In D5825#148571, @vlorentz wrote:

wouldn't it make sense to have a separate command (eg. recursive-identify instead of identify --recursive)?

In D5825#148548, @DanSeraf wrote:

But what was the process before? Did it ignore directory entries?

It checks only the given directories generating a from_disk.Directory object for each directory. Should it uses the same logic used for the recursive option?

In D5825#148516, @DanSeraf wrote:

2. If relevant, could you implement --verify too?

Sure, if it is useful i could open another diff for it

I'm adding here a note about something to consider in terms of pros/cons: accessibility. As for the most part we are archiving textual information, we really want it to be accessible for all users. Right now we go further than that, ensuring that the Web UI can be browser with a textual browser: so, for instance, w3m https://archive.softwareheritage.org/swh:1:cnt:c839dea9e8e6f0528b468214348fee8669b305b2 just works out of the box. I'm not up to date on what's the accessibility impact of current JS frameworks, nor that we should have as a requirement that the archive is browsable without JavaScript enabled (as per today's standards "browsable with free javascript" is probably good enough for us, and we have a curl-able API anyway), but accessibility per se is definitely going to be a requirement.

how about just collecting all raw timings in an output CSV file (or several files if needed) and compute the stats downstream (e.g., with pandas)?
that would allow changing the percentiles later on as well as compute different stats, without having to rerun the benchmarks

Minor request, which can also be implemented in a subsequent commit, can we have the mapping documented somewhere? As a first approximation even a docstring would do, so that it will show up at docs.s.o. (Not sure if the files being modified here are the most relevant place for it though, it can also go at the root of the mercurial loader Python hierarchy, up to you !)

That explains it, and it's good enough for me, thanks :)

My remaining question then is: how about, instead of branch-{tip,heads,closed-heads}/name we use branches/{heads,closed,tip}/name ?

In T3352#65753, @marmoute wrote:

They are all branch heads (git "branch" are about heads too, bookmarks too), so a heads/ prefix does not bring much.

My point here is for user looking at the structure to easily distinguish between the different mapping format. Something based on the "visit data" and associated documentation seems quite fragile.

(i'm marking this as on hold until we have reached a decision on T3352, just to avoid this gets deployed by mistake. But feel free to go ahead with the rest of the review or even override, if you think there is a better safeguard to avoid this gets deployed)

Understood. To explain my thinking here, the refs/... structure is something we picked to represent git branch names as faithfully as possible, adding as little as possible on top of it. In trying to represent branch names from another VCS, as a first approximation I'd rather reuse the same *approach* than a *result* that is similar, if that makes sense. So, to pivot the question around, what is the minimal (also in the sense that it is shorter / has less cruft) naming scheme that would allow us to represent without ambiguity all the Mercurial naming aspects that you want to capture?

Is the ability to recognize that a snapshot comes from Mercurial an actual goal here? I don't think we care about "clashes" between snapshot created from different VCS, but maybe I'm missing something.