We should add a comment about the fact the internal Mercurial API is not guaranteed to be stable. In practice I do not expect the API we use to change much, but we better warn the future people looking into this.

You can use the enum values in swh.model.from_disk.DentryPerms

why is this a comment rather than a properly crafted docstring?

This could use a small docstring.

s/data/date/ I guess

why do we need to use urlsplit on a directory path? If we expect the directory argument to (possibly) be a file:// url, then it should be told explicitly.

You should document the class purpose and scope.

please explain why the "unfiltered" is there

small nits: could simply be visit_and_status[1].snapshot

... here: explain unfiltered()

Maybe add a comment about visibility here.

What is this function about? And why cannot we have it now?

note: the docstring seems to need an update.

But the local clone might still need update, right? What would be the final state of this can we document the TODO here?

Same question (mostly for me to understand the situation better) what is this function about and what is the long terme goal for it?

What's this _hashes for?

Also, why don't we build the hashes_caches during this step?

Same question, why are we getting the filename from the hashcache ?

I find it a bit weird to have the _get_revisions store all the underlying content but the revision itself. Maybe we should return already store revision there.

What do you think ?

small typo: "An filtered" → "A filtered"

shouldn't the repo_directory attribute be "declared" here too?

Same question as for the other method that need to be implemented for incremental loading.

I may be missing something but why is this a property? why not just initialize a self.repo in the __fetch_data__ method (where the repo_directory is initialized)?

Same question (and I assume, same answer)

(idem)

(idem)

Is this an attribute declaration ? If so, this should be in the __init__.

could we have a bit of comment here (for the non-mercurial expert)?

also a comment here to explain it's fore backward compat

Here we seems to assume that the existing repository is up to date. In which case we we already have repository and why don't wee need to pull it ?

It is still unclear to me when/why we have this special case. Can you clarify that point ?

Small nits: If they are no other usage than the cli call, maybe pulling is not necessary.

we should probably call this hg_nodeid for clarity.

I may be wrong but belive there is no need for computing the revision id "by hand", something like:

rev_obj = Revision.from_dict(revision)
self._node_hashes[ctx.node()] = rev_obj.id
yield rev_obj

should work (the id for a model.HashableObject is automatically computed in a post-init hook if not provided)

Same question, is this a new attribute initialisation ?

I'm not very fond of instance attributes being created all over the methods...

Why do we need to yield the (mercurial) nodei here ?

I don't what's this is doing

If we are to store directory one by one, could we simply store them as we build them ? It would make the flow simpler and help us with future optimisation (eg: not recreating directory that already exists)

This method could use a docstring.

Not doing any optimization here at first is good. However, it would be nice to add comment about which optimization are planned where we plan them. It would also help to make sure we have the right structure for these optimization to come to play.

I suspect the same feedback as for revision apply here, could we create the Release directly instead ?

I assume the sub directory will be created automatically by the TreeDirectory API, it might be worth a small comment.

Exception is quite wide. We need something more accurate. If SWH does not have something more accurate already, using at least ValueError seem appropriate.

Why do have this recursivity ? In which case do we expect parent to not be stored because trying to store their children ?

it would be nice to have a bit of explanation on how mercurial's branch/tag model is mapped into swh's (git-like) one here.

I would move these utility higher in the module to have them defined before the code that use them.

why is this for loop needed? why not just self.tags_target['tip'] ?

Should we pass rev_ctx here instead ?

Why do we need a HEAD ? and why is it tip ?

I am a bit warry about the reuse of internal attribute to fetch these information. Especially because they semantic is not documented so far.

• "incremental loading" is probably an overloaded ambiguous terms, lets stop using it.
• What's you plan to change the way data are fed to this class when we implement something more efficient than recreating the full manifest for each revision.

I don't understand the way offsets is managed here. Why is it always negative? the negative_utc flag is supposed to be set true only for very rare cases coming from git, IIRC, in which one can use a date with UTC-0000 (which make no sense, but is possible and used for hash computation in git).

same comment as above, you can build the snapshot directly instead of using from_dict:

for ...:
    branches[name] = SnapshotBranch(
        target=target,
        target_type=TargetType.REVISION,
    )
    ...

snapshot = Snapshot(branches=branches)

should automatically compute the id

Same question about why we need a two steps approach here.

We probably want this to be a function on the object. This will abstract the in memory cache and allow collaboration with the SWH database when we implement incremental loading in the future.

Unless I'm wrong, this cache will be (potentially) huge, since it loads the whole hg repo (including file content) in RAM. This is a problem.

still not satisfied by this "hide this under the carpet" path

In practice, the current setup is not a cache, since it must be fully populated for the code qui already went over to work. So this is a mapping. In addition, the as I said before, the current execution order is not going to be usable as a base for future work. And we need a more revision centric approach (cf previous email).

In addition, the current cache/mapping structure is wrong. What we need is a`cache[filename][filerevision(or filenode)] → SWHID cache. So that we don't have to recompute the hash of everything for every revision. The current cache[changelog-rev][filename] → SWHID` is mostly loading all revision+manifest in memory is does not add much information compared to what is on disk.

As already pointed, how will this behave with say mozilla or a linux repo?

It would be nice to document what a SWH's Release maps to in terms of Mercurial concept.

I'm a bit worried how this is supposed to work: do we really recompute the hashes of file already seen (eg. a file present from rev 1 and then unmodified for all the revisions) in each revision? Or did I miss something?

We need to keep the tip tags because the previous loader was loading it ? Or is there another reason ? (It would be nice if the doc was clearer about that.)

Why can't we simply split on os.sep ? If we can't we need to document why.

Same here, if would be nice to Document what's a SWH's content map to in terms of Mercurial concept (here: a file revision + some other stuff)

Why is this done "by hand" (i.e. reading the .hgtags file) instead of using the mercurial repo object?

You should add a small inline comment to mention that caching is simple and will come in the next revision.

It's unclear to me what this tree structure is really for. Couldn't swh.model.from_disk data structure be used for this instead of reimplementing it?

"compute recursively compute "

typos:

• "when [...] is implemented, it will [...]"
• "recaclulate"

Also I don't get how the "recalculate only" is foreseen here.

what? a function located after the if __name__ == "__main__"?

prefer the type Sha1Git (from swh.model.model), it's an alias of bytes

you may want to use errors='replace' here, in case the name can't be decoded

Mercurial author field are free form. Assuming git formatted author field is going to create issue. What do we actually need to store in SWH ?

Since we don't use data/author on tag (release) yet. I recommend we split this in a different changeset.

Why do we need to implement our own things for this ? doesn't SWH already have an utility for this ? If we need something special here, you need to document what it is? And if something special is needed, we should consider implementing it in the upstream module instead.

(please move this before the if __name__ == "__main__" block.)

The semantic of this function needs to be document, What is the actual information we fetch, and why do we needs it.

Same here, please document what a Directory maps to in terms of Mercurial concepts.

.pop(0) is quite expensive, could we reverse the list andusing .pop() ? if not, lets use a better datastructure for this.

What does to_model do ? Is it dropping the revision to the binary content of the file ? Why do we need the "isinstance" call for ?

What is this class about ?

what is different compared to the base class ?

same question: what is different compared to the base class ?

please define the data we are trying to acquire here.

As we discussed before, we need to better define Archive here.