It looks great to me, I just added some remarks on places it wasn't immediately obvious what the code was doing, and some other little nitpicks.

I really like this module, the API is very clean and what I'd like to use.

My only nit is using symbolic constants for perms (see comments).

sounds good.

I've updated the approach after looking at some of the call sites for swh.model.git:

• swh.loader.dir: only uses compute_hashes_from_directory -> Directory.from_disk
• swh.loader.svn:
  • uses compute_revision_sha1_git -> identifiers.revision_identifier
  • uses compute_blob_metadata -> Content.from_file
  • however, ra.SWHDirEditor uses the guts of swh.model.git to minimize updates. It should be reasonably easy to port to the Merkle datastructure now.

This sounds quite fine.

For information, i've tested locally the loader-dir/tar migration with this (using an adapter like the following code - swh.loader.dir.loader):

Python
   ...
        from swh.model.from_disk import Content, Directory
        directory = Directory.from_disk(path=dir_path)

        def all_entries_from(directory):
            """Retrieve recursively all entries from the top directory.

            """
            files = []
            dirs = []
            for name, child in directory.items():
                if isinstance(child, Content):
                    files.append(directory.child_to_directory_entry(
                        name, child))
                    continue
                elif isinstance(child, Directory):
                    dirs.append(directory.child_to_directory_entry(
                        name, child))
                    subfiles, subdirs = all_entries_from(child)
                    files.extend(subfiles)
                    dirs.extend(subdirs)
                else:
                    raise ValueError('Unknown child')

            return files, dirs

        files, dirs = all_entries_from(directory)

        tree_hash = directory.hash
        full_rev = _revision_from(tree_hash, revision)

        objects = {
            BLOB: files,
            TREE: dirs,
            COMM: [full_rev],
            RELE: []
        }

  ...

prior version:

Python
       objects_per_path = git.compute_hashes_from_directory(dir_path)

       tree_hash = objects_per_path[dir_path]['checksums']['sha1_git']
       full_rev = _revision_from(tree_hash, revision)

       objects = {
           GitType.BLOB: list(
               git.objects_per_type(GitType.BLOB, objects_per_path)),
           GitType.TREE: list(
               git.objects_per_type(GitType.TREE, objects_per_path)),
           GitType.COMM: [full_rev],
           GitType.RELE: []
       }

And the loader-dir/tar tests were fine after that.

In D248#5132, @ardumont wrote:

This sounds quite fine.

For information, i've tested locally the loader-dir/tar migration with this (using an adapter like the following code - swh.loader.dir.loader):

Python
   ...
        from swh.model.from_disk import Content, Directory
        directory = Directory.from_disk(path=dir_path)

        def all_entries_from(directory):
            """Retrieve recursively all entries from the top directory.

            """
            files = []
            dirs = []
            for name, child in directory.items():
                if isinstance(child, Content):
                    files.append(directory.child_to_directory_entry(
                        name, child))
                    continue
                elif isinstance(child, Directory):
                    dirs.append(directory.child_to_directory_entry(
                        name, child))
                    subfiles, subdirs = all_entries_from(child)
                    files.extend(subfiles)
                    dirs.extend(subdirs)
                else:
                    raise ValueError('Unknown child')

            return files, dirs

        files, dirs = all_entries_from(directory)

        tree_hash = directory.hash
        full_rev = _revision_from(tree_hash, revision)

        objects = {
            BLOB: files,
            TREE: dirs,
            COMM: [full_rev],
            RELE: []
        }

  ...

prior version:

Python
       objects_per_path = git.compute_hashes_from_directory(dir_path)

       tree_hash = objects_per_path[dir_path]['checksums']['sha1_git']
       full_rev = _revision_from(tree_hash, revision)

       objects = {
           GitType.BLOB: list(
               git.objects_per_type(GitType.BLOB, objects_per_path)),
           GitType.TREE: list(
               git.objects_per_type(GitType.TREE, objects_per_path)),
           GitType.COMM: [full_rev],
           GitType.RELE: []
       }

And the loader-dir/tar tests were fine after that.

You should use the collect method on the root directory to generate all the objects recursively; you can then use the hash method to get the identifier for the toplevel directory (to be passed to the revision) and you should be golden.

Clearly this needs to be better documented :)

In D248#5133, @olasd wrote:

In D248#5132, @ardumont wrote:

This sounds quite fine.

For information, i've tested locally the loader-dir/tar migration with this (using an adapter like the following code - swh.loader.dir.loader):

Python
   ...
        from swh.model.from_disk import Content, Directory
        directory = Directory.from_disk(path=dir_path)

        def all_entries_from(directory):
            """Retrieve recursively all entries from the top directory.

            """
            files = []
            dirs = []
            for name, child in directory.items():
                if isinstance(child, Content):
                    files.append(directory.child_to_directory_entry(
                        name, child))
                    continue
                elif isinstance(child, Directory):
                    dirs.append(directory.child_to_directory_entry(
                        name, child))
                    subfiles, subdirs = all_entries_from(child)
                    files.extend(subfiles)
                    dirs.extend(subdirs)
                else:
                    raise ValueError('Unknown child')

            return files, dirs

        files, dirs = all_entries_from(directory)

        tree_hash = directory.hash
        full_rev = _revision_from(tree_hash, revision)

        objects = {
            BLOB: files,
            TREE: dirs,
            COMM: [full_rev],
            RELE: []
        }

  ...

prior version:

Python
       objects_per_path = git.compute_hashes_from_directory(dir_path)

       tree_hash = objects_per_path[dir_path]['checksums']['sha1_git']
       full_rev = _revision_from(tree_hash, revision)

       objects = {
           GitType.BLOB: list(
               git.objects_per_type(GitType.BLOB, objects_per_path)),
           GitType.TREE: list(
               git.objects_per_type(GitType.TREE, objects_per_path)),
           GitType.COMM: [full_rev],
           GitType.RELE: []
       }

And the loader-dir/tar tests were fine after that.

You should use the collect method on the root directory to generate all the objects recursively; you can then use the hash method to get the identifier for the toplevel directory (to be passed to the revision) and you should be golden.

way better!

Clearly this needs to be better documented :)

Oh, well, it is documented, i just did not make the right connections there.
At the time i read typeA, typeB, it did not yet make sense to me...
Now it does.

Thanks.

I've re-looked at this after the switch to the Merkle AST, and it looks good to me.

Only a question/comment: it'd be nice to have in test_from_disk.py a unit test that the resulting checksums of 1) a content, and 2) a directory match what we can obtain by hand using something else (e.g., git itself, or the current implementation before switching to from_disk.py). Unless I'm missing something, there are no such tests ATM. (Possible mismatches there should be caught by unit tests of loaders that use from_disk, but having them in the inner circle would be nice too.)

In D248#5138, @zack wrote:

Only a question/comment: it'd be nice to have in test_from_disk.py a unit test that the resulting checksums of 1) a content, and 2) a directory match what we can obtain by hand using something else (e.g., git itself, or the current implementation before switching to from_disk.py). Unless I'm missing something, there are no such tests ATM. (Possible mismatches there should be caught by unit tests of loaders that use from_disk, but having them in the inner circle would be nice too.)

I'll pull the test logic from swh.model.git which tries to do that. I'm not too excited about the dependency on swh-storage-testdata but apart from that it's sensible to do.

Big round of test updates for from_disk : cleaned up the equality tests to address @seirl's comments.

Add some toplevel documentation to make it more obvious how to use this.

Merge expected tomorrow.