From 2436f56bca1b82356596d480e47841d9dcbce592 Mon Sep 17 00:00:00 2001
From: Benjamin Poldrack <benjaminpoldrack@gmail.com>
Date: Wed, 4 Dec 2024 12:40:57 +0100
Subject: [PATCH 1/4] git-log and operations record parsing

This patch implements retrieval of structured information from `git log`
and `git log --follow <path>`, including parsing of onyo's operations
record.
This is done in preparation of implementing pseudo-keys for
history-related metadata.
Currently operates on `Path` objects. In context of fully implementing
the pseudo-key and namespace concept, `Inventory.get_history()` will
need to change in order to take `Item` instances instead.
---
 onyo/lib/git.py                             |  87 ++++++++-
 onyo/lib/inventory.py                       |  63 +++++++
 onyo/lib/tests/test_inventory_operations.py | 190 ++++++++++++++++++--
 3 files changed, 324 insertions(+), 16 deletions(-)

diff --git a/onyo/lib/git.py b/onyo/lib/git.py
index 71f3c10b..8108327d 100644
--- a/onyo/lib/git.py
+++ b/onyo/lib/git.py
@@ -9,7 +9,10 @@
 from onyo.lib.ui import ui
 
 if TYPE_CHECKING:
-    from typing import Iterable
+    from typing import (
+        Generator,
+        Iterable,
+    )
 
 log: logging.Logger = logging.getLogger('onyo.git')
 
@@ -401,3 +404,85 @@ def check_ignore(self, ignore: Path, paths: list[Path]) -> list[Path]:
 
     # TODO: git check-ignore --no-index --stdin (or via command call)  ->  lazy, check GitRepo.files once. (Same invalidation)
     #       -> OnyoRepo would use it to maintain a ignore list from a (top-level .onyoignore)? .onyo/ignore ? Both?
+
+    def _parse_log_output(self, lines: list[str]) -> dict:
+        """Generate a dict from the output of ``git log`` for one commit.
+
+        Internal helper that gets a list of git-log-output lines,
+        that represent a single commit and generates a dictionary from it.
+        """
+        import datetime
+        import re
+        regex_author = re.compile(r"Author:\s+(?P<name>\b.*\b)\s+<(?P<email>[^\s]+)>$")
+        regex_committer = re.compile(r"Commit:\s+(?P<name>\b.*\b)\s+<(?P<email>[^\s]+)>$")
+        commit = dict()
+        for line in lines:
+            if line.startswith('commit '):
+                commit['commit'] = line.split()[1]
+                continue
+            elif line.startswith('Author:'):
+                try:
+                    commit['author'] = re.match(regex_author, line).groupdict()  # pyre-ignore [16] AttributeError is caught
+                except AttributeError as e:
+                    if str(e).endswith("'groupdict'"):
+                        raise RuntimeError(f"Unable to parse author-line:\n{line}") from e
+                    raise
+                continue
+            elif line.startswith('AuthorDate:'):
+                continue
+            elif line.startswith('Commit:'):
+                try:
+                    commit['committer'] = re.match(regex_committer, line).groupdict()
+                except AttributeError as e:
+                    if str(e).endswith("'groupdict'"):
+                        raise RuntimeError(f"Unable to parse committer-line:\n{line}") from e
+                    raise
+                continue
+            elif line.startswith('CommitDate:'):
+                commit['time'] = datetime.datetime.fromisoformat(line.split(maxsplit=1)[1])
+                continue
+            else:
+                # This line is part of the message.
+                if 'message' not in commit:
+                    commit['message'] = [line]
+                else:
+                    commit['message'].append(line)
+
+        return commit
+
+    def follow(self, path: Path | None = None, n: int | None = None) -> Generator[dict, None, None]:
+        """Yields commit dicts representing the history of ``path``.
+
+        History according to ``git log --follow`` if a path is given
+        or ``git log`` otherwise.
+
+        Parameters
+        ----------
+        path: Path, optional
+          What file to follow. If `None`, get the history of HEAD instead.
+        n: int, optional
+          Limit history going back ``n`` commits.
+          ``None`` for no limit (default).
+        """
+        # TODO: Do we want to distinguish methods for `git log` (path optional) vs `git log --follow` (requires a path)?
+        limit = [f'-n{n}'] if n is not None else []
+        pathspec = ['--follow', '--', str(path)] if path else []
+        cmd = ['log', '--date=iso-strict', '--pretty=fuller'] + limit + pathspec
+        output = self._git(cmd)
+
+        # yield output on a per-commit-basis
+        commit_output = []
+        for line in output.splitlines():
+            if line.startswith('commit '):
+                # This is the first line of a new commit.
+                # 1. store previous commit output
+                if commit_output:
+                    # we just finished the previous commit.
+                    yield self._parse_log_output(commit_output)
+                # 2. start new commit output
+                commit_output = [line]
+            else:
+                # add to current commit output
+                commit_output.append(line)
+        if commit_output:
+            yield self._parse_log_output(commit_output)
diff --git a/onyo/lib/inventory.py b/onyo/lib/inventory.py
index c163c3d4..7b192957 100644
--- a/onyo/lib/inventory.py
+++ b/onyo/lib/inventory.py
@@ -725,3 +725,66 @@ def raise_empty_keys(self, asset: dict | UserDict) -> None:
             # Note, that DotNotationWrapper.keys() delivers strings (and has to).
             # Hence, `None` as a key would show up here as 'None'.
             raise ValueError("Keys are not allowed to be empty or None-values.")
+
+    def _parse_operations_record(self, record: list[str]):
+
+        if not record[0].strip() == "--- Inventory Operations ---":
+            raise RuntimeError("Invalid operations record.")
+        parsed_record = {k: [] for k in OPERATIONS_MAPPING.keys()}
+        collecting_key = None
+        for line in record[1:]:
+            line = line.strip()
+            if not line:
+                continue
+            match line:
+                case "New assets:":
+                    collecting_key = "new_assets"
+                case "New directories:":
+                    collecting_key = "new_directories"
+                case "Removed assets:":
+                    collecting_key = "remove_assets"
+                case "Removed directories:":
+                    collecting_key = "remove_directories"
+                case "Moved assets:":
+                    collecting_key = "move_assets"
+                case "Moved directories:":
+                    collecting_key = "move_directories"
+                case "Renamed directories:":
+                    collecting_key = "rename_directories"
+                case "Renamed assets:":
+                    collecting_key = "rename_assets"
+                case "Modified assets:":
+                    collecting_key = "modify_assets"
+                case _:
+                    if not collecting_key or line and not line.startswith("- "):
+                        raise RuntimeError("Invalid operations record.")
+                    cleaned_line = line[2:].split(" -> ")
+                    if len(cleaned_line) > 2:
+                        raise RuntimeError(f"Invalid operations record:{line}")
+                    if len(cleaned_line) == 1:
+                        parsed_record[collecting_key].append(Path(cleaned_line[0]))
+                    else:
+                        parsed_record[collecting_key].append(
+                            (Path(cleaned_line[0]), Path(cleaned_line[1]))
+                        )
+        return parsed_record
+
+    def get_history(self, path: Path | None = None, n: int | None = None):
+        # TODO: This isn't quite right yet. operations records are defined in Inventory.
+        #       But Inventory shouldn't talk to GitRepo directly. So, either pass the record
+        #       from Inventory to OnyoRepo and turn it into a commit-message part only,
+        #       or have sort of a proxy in OnyoRepo.
+        #       -> May be: get_history(Item) in Inventory and get_history(path) in OnyoRepo.
+
+        for commit in self.repo.git.follow(path, n):
+            record = []
+            start = False
+            for line in commit['message']:
+                if line.strip() == "--- Inventory Operations ---":
+                    start = True
+                if start:
+                    record.append(line)
+            if record:
+
+                commit['operations'] = self._parse_operations_record(record)
+            yield commit
diff --git a/onyo/lib/tests/test_inventory_operations.py b/onyo/lib/tests/test_inventory_operations.py
index 85d9a201..29880050 100644
--- a/onyo/lib/tests/test_inventory_operations.py
+++ b/onyo/lib/tests/test_inventory_operations.py
@@ -83,7 +83,17 @@ def test_add_asset(repo: OnyoRepo) -> None:
     for k, v in asset.items():
         if k not in RESERVED_KEYS + list(PSEUDO_KEYS.keys()):
             assert asset_from_disc[k] == v
-    # TODO: check commit message
+
+    # check operations record:
+    commit = [c for c in inventory.get_history(asset_file, n=1)][0]
+    for k, v in commit['operations'].items():
+        if k == 'new_assets':
+            assert v == [asset_file.relative_to(inventory.root)]
+        elif k == 'new_directories':
+            assert newdir1.relative_to(inventory.root) in v
+            assert newdir2.relative_to(inventory.root) in v
+        else:
+            assert v == []
 
     # required keys must not be empty
     asset.update(dict(model=dict(name="")))
@@ -146,6 +156,14 @@ def test_remove_asset(inventory: Inventory) -> None:
     # but parent dir still is an inventory dir:
     assert inventory.repo.is_inventory_dir(asset_file.parent)
 
+    # check operations record:
+    commit = [c for c in inventory.get_history(n=1)][0]
+    for k, v in commit['operations'].items():
+        if k == 'remove_assets':
+            assert v == [asset_file.relative_to(inventory.root)]
+        else:
+            assert v == []
+
 
 def test_move_asset(repo: OnyoRepo) -> None:
     inventory = Inventory(repo)
@@ -188,6 +206,16 @@ def test_move_asset(repo: OnyoRepo) -> None:
     assert not asset_file.exists()
     assert (newdir1 / asset_file.name).is_file()
 
+    # check operations record:
+    commit = [c for c in inventory.get_history(newdir1 / asset_file.name, n=1)][0]
+    for k, v in commit['operations'].items():
+        if k == 'move_assets':
+            assert v == [(
+                asset_file.relative_to(inventory.root), (newdir1 / asset_file.name).relative_to(inventory.root)
+            )]
+        else:
+            assert v == []
+
 
 def test_rename_asset(repo: OnyoRepo) -> None:
     inventory = Inventory(repo)
@@ -291,6 +319,16 @@ def test_modify_asset(repo: OnyoRepo) -> None:
     assert asset_on_disc['onyo.path.parent'] == new_asset_file.parent.relative_to(inventory.root)
     assert asset_on_disc['onyo.is.directory'] is False
 
+    # check operations record:
+    commit = [c for c in inventory.get_history(new_asset_file, n=1)][0]
+    for k, v in commit['operations'].items():
+        if k == 'modify_assets':
+            assert v == [asset_file.relative_to(inventory.root)]
+        elif k == 'rename_assets':
+            assert v == [(asset_file.relative_to(inventory.root), new_asset_file.relative_to(inventory.root))]
+        else:
+            assert v == []
+
 
 def test_add_directory(repo: OnyoRepo) -> None:
     inventory = Inventory(repo)
@@ -317,17 +355,25 @@ def test_add_directory(repo: OnyoRepo) -> None:
     assert repo.is_inventory_dir(new_dir)
     assert (new_dir / repo.ANCHOR_FILE_NAME).is_file()
 
+    # check operations record:
+    commit = [c for c in inventory.get_history(new_dir / '.anchor', n=1)][0]
+    for k, v in commit['operations'].items():
+        if k == 'new_directories':
+            assert v == [new_dir.relative_to(inventory.root)]
+        else:
+            assert v == []
+
 
 def test_remove_directory(repo: OnyoRepo) -> None:
     inventory = Inventory(repo)
     newdir1 = repo.git.root / "somewhere"
     newdir2 = newdir1 / "new"
     emptydir = newdir1 / "empty"
-    asset_file = newdir2 / "asset_file"
+    asset_file = newdir2 / "TYPE_MAKE_MODEL.SERIAL"
     asset = Item(
         some_key="some_value",
         type="TYPE",
-        make="MAKER",
+        make="MAKE",
         model=dict(name="MODEL"),
         serial="SERIAL",
         other='1',
@@ -363,6 +409,18 @@ def test_remove_directory(repo: OnyoRepo) -> None:
     assert not newdir2.exists()
     assert not newdir1.exists()
 
+    # check operations record:
+    commit = [c for c in inventory.get_history(n=1)][0]
+    for k, v in commit['operations'].items():
+        if k == 'remove_directories':
+            assert newdir1.relative_to(inventory.root) in v
+            assert newdir2.relative_to(inventory.root) in v
+            assert len(v) == 2
+        elif k == 'remove_assets':
+            assert v == [asset_file.relative_to(inventory.root)]
+        else:
+            assert v == []
+
 
 def test_move_directory(repo: OnyoRepo) -> None:
     inventory = Inventory(repo)
@@ -403,6 +461,16 @@ def test_move_directory(repo: OnyoRepo) -> None:
     assert not newdir2.exists()
     assert repo.is_inventory_dir(emptydir / newdir2.name)
 
+    # check operations record:
+    commit = [c for c in inventory.get_history(n=1)][0]
+    for k, v in commit['operations'].items():
+        if k == 'move_directories':
+            assert v == [(
+                newdir2.relative_to(inventory.root), (emptydir / newdir2.name).relative_to(inventory.root)
+            )]
+        else:
+            assert v == []
+
 
 def test_rename_directory(repo: OnyoRepo) -> None:
     inventory = Inventory(repo)
@@ -446,6 +514,16 @@ def test_rename_directory(repo: OnyoRepo) -> None:
     assert not newdir2.exists()
     assert repo.is_inventory_dir(new_name)
 
+    # check operations record:
+    commit = [c for c in inventory.get_history(n=1)][0]
+    for k, v in commit['operations'].items():
+        if k == 'rename_directories':
+            assert v == [(
+                newdir2.relative_to(inventory.root), new_name.relative_to(inventory.root)
+            )]
+        else:
+            assert v == []
+
 
 def test_add_asset_dir(repo: OnyoRepo) -> None:
     inventory = Inventory(repo)
@@ -484,6 +562,15 @@ def test_add_asset_dir(repo: OnyoRepo) -> None:
     assert inventory.repo.is_asset_dir(asset_dir_path)
     # TODO: should the yaml file within be a valid asset path as well? Think not.
     # assert inventory.repo.is_asset_path(asset_dir_path / OnyoRepo.ASSET_DIR_FILE)
+    # check operations record:
+    commit = [c for c in inventory.get_history((asset_dir_path / OnyoRepo.ASSET_DIR_FILE_NAME), n=1)][0]
+    for k, v in commit['operations'].items():
+        if k == 'new_directories':
+            assert v == [asset_dir_path.relative_to(inventory.root)]
+        elif k == 'new_assets':
+            assert v == [asset_dir_path.relative_to(inventory.root)]
+        else:
+            assert v == []
 
     # add asset aspect to existing directory, which does not yet comply with asset naming scheme
     dir_path = inventory.root / "newdir"
@@ -531,6 +618,16 @@ def test_add_asset_dir(repo: OnyoRepo) -> None:
     assert inventory.repo.is_asset_dir(expected_path)
     assert inventory.repo.git.is_clean_worktree()
 
+    # check operations record:
+    commit = [c for c in inventory.get_history(expected_path / OnyoRepo.ASSET_DIR_FILE_NAME, n=1)][0]
+    for k, v in commit['operations'].items():
+        if k == 'rename_directories':
+            assert v == [(dir_path.relative_to(inventory.root), expected_path.relative_to(inventory.root))]
+        elif k == 'new_assets':
+            assert v == [expected_path.relative_to(inventory.root)]
+        else:
+            assert v == []
+
 
 def test_add_dir_asset(repo: OnyoRepo) -> None:
     inventory = Inventory(repo)
@@ -562,6 +659,14 @@ def test_add_dir_asset(repo: OnyoRepo) -> None:
     assert inventory.repo.is_asset_dir(asset_path)
     assert inventory.repo.git.is_clean_worktree()
 
+    # check operations record:
+    commit = [c for c in inventory.get_history(n=1)][0]
+    for k, v in commit['operations'].items():
+        if k == 'new_directories':
+            assert v == [asset_path.relative_to(inventory.root)]
+        else:
+            assert v == []
+
 
 def test_remove_asset_dir_directory(repo: OnyoRepo) -> None:
     inventory = Inventory(repo)
@@ -602,6 +707,16 @@ def test_remove_asset_dir_directory(repo: OnyoRepo) -> None:
     assert asset_dir_path.is_file()
     assert inventory.repo.git.is_clean_worktree()
 
+    # check operations record:
+    commit = [c for c in inventory.get_history(n=1)][0]
+    for k, v in commit['operations'].items():
+        if k == 'remove_directories':
+            assert v == [asset_dir_path.relative_to(inventory.root)]
+        elif k == 'remove_assets':
+            assert v == [(asset_dir_path / "a_b_c.1A").relative_to(inventory.root)]
+        else:
+            assert v == []
+
 
 def test_remove_asset_dir_asset(repo: OnyoRepo) -> None:
     inventory = Inventory(repo)
@@ -644,6 +759,14 @@ def test_remove_asset_dir_asset(repo: OnyoRepo) -> None:
     # asset within unaffected:
     assert inventory.repo.is_asset_path(asset_within['directory'] / "a_b_c.1A")
 
+    # check operations record:
+    commit = [c for c in inventory.get_history(n=1)][0]
+    for k, v in commit['operations'].items():
+        if k == 'remove_assets':
+            assert v == [asset_dir_path.relative_to(inventory.root)]
+        else:
+            assert v == []
+
 
 def test_move_asset_dir(repo: OnyoRepo) -> None:
     # An asset dir could be moved by either move_dir or move_asset. Since it's both, there's no difference when we treat
@@ -677,10 +800,20 @@ def test_move_asset_dir(repo: OnyoRepo) -> None:
     new_path = dir_path / asset_dir_path.name
     assert not asset_dir_path.exists()
     assert inventory.repo.is_asset_dir(new_path)
-    # Two operations recorded:
-    msg = inventory.repo.git.get_commit_msg()
-    assert "Moved assets" in msg
-    assert "Moved directories" in msg
+
+    # check operations record (two operations recorded):
+    commit = [c for c in inventory.get_history(n=1)][0]
+    for k, v in commit['operations'].items():
+        if k == 'move_assets':
+            assert v == [(
+                asset_dir_path.relative_to(inventory.root), (dir_path / asset_dir_path.name).relative_to(inventory.root)
+            )]
+        elif k == 'move_directories':
+            assert v == [(
+                asset_dir_path.relative_to(inventory.root), (dir_path / asset_dir_path.name).relative_to(inventory.root)
+            )]
+        else:
+            assert v == []
 
     # Now move back but via `move_directory` instead.
     inventory.move_directory(new_path, inventory.root)
@@ -694,10 +827,20 @@ def test_move_asset_dir(repo: OnyoRepo) -> None:
     inventory.commit("Move asset dir back")
     assert inventory.repo.is_asset_dir(asset_dir_path)
     assert not new_path.exists()
-    # Two operations recorded:
-    msg = inventory.repo.git.get_commit_msg()
-    assert "Moved assets" in msg
-    assert "Moved directories" in msg
+
+    # check operations record (two operations recorded):
+    commit = [c for c in inventory.get_history(n=1)][0]
+    for k, v in commit['operations'].items():
+        if k == 'move_assets':
+            assert v == [(
+                (dir_path / asset_dir_path.name).relative_to(inventory.root), asset_dir_path.relative_to(inventory.root)
+            )]
+        elif k == 'move_directories':
+            assert v == [(
+                (dir_path / asset_dir_path.name).relative_to(inventory.root), asset_dir_path.relative_to(inventory.root)
+            )]
+        else:
+            assert v == []
 
 
 def test_rename_asset_dir(repo: OnyoRepo) -> None:
@@ -747,10 +890,15 @@ def test_rename_asset_dir(repo: OnyoRepo) -> None:
     assert inventory.repo.is_inventory_dir(new_asset_dir_path)
     assert inventory.repo.git.is_clean_worktree()
 
-    # Two operations recorded:
-    msg = inventory.repo.git.get_commit_msg()
-    assert "Renamed assets" in msg
-    assert "Renamed directories" in msg
+    # check operations record (two operations recorded):
+    commit = [c for c in inventory.get_history(n=1)][0]
+    for k, v in commit['operations'].items():
+        if k == 'rename_assets':
+            assert v == [(asset_dir_path.relative_to(inventory.root), new_asset_dir_path.relative_to(inventory.root))]
+        elif k == 'rename_directories':
+            assert v == [(asset_dir_path.relative_to(inventory.root), new_asset_dir_path.relative_to(inventory.root))]
+        else:
+            assert v == []
 
 
 def test_modify_asset_dir(repo: OnyoRepo) -> None:
@@ -813,3 +961,15 @@ def test_modify_asset_dir(repo: OnyoRepo) -> None:
 
     assert asset_on_disc['onyo.path.absolute'] == new_asset_path
     assert asset_on_disc['onyo.path.parent'] == new_asset_path.parent.relative_to(inventory.root)
+
+    # check operations record:
+    commit = [c for c in inventory.get_history(n=1)][0]
+    for k, v in commit['operations'].items():
+        if k == 'modify_assets':
+            assert v == [asset_path.relative_to(inventory.root)]
+        elif k == 'rename_assets':
+            assert v == [(asset_path.relative_to(inventory.root), new_asset_path.relative_to(inventory.root))]
+        elif k == 'rename_directories':
+            assert v == [(asset_path.relative_to(inventory.root), new_asset_path.relative_to(inventory.root))]
+        else:
+            assert v == []

From 5d26295676f2d8f581adf41be213f6b39f98a3c9 Mon Sep 17 00:00:00 2001
From: Benjamin Poldrack <benjaminpoldrack@gmail.com>
Date: Mon, 9 Dec 2024 13:55:24 +0100
Subject: [PATCH 2/4] tmp: RF, amend

---
 onyo/lib/decoder.py   | 47 ++++++++++++++++++++++++++++++++
 onyo/lib/inventory.py | 63 ++-----------------------------------------
 onyo/lib/onyo.py      | 20 ++++++++++++++
 3 files changed, 69 insertions(+), 61 deletions(-)
 create mode 100644 onyo/lib/decoder.py

diff --git a/onyo/lib/decoder.py b/onyo/lib/decoder.py
new file mode 100644
index 00000000..361859a8
--- /dev/null
+++ b/onyo/lib/decoder.py
@@ -0,0 +1,47 @@
+from pathlib import Path
+
+from onyo.lib.inventory import OPERATIONS_MAPPING
+
+
+def parse_operations_record(record: list[str]):
+
+    if not record[0].strip() == "--- Inventory Operations ---":
+        raise RuntimeError("Invalid operations record.")
+    parsed_record = {k: [] for k in OPERATIONS_MAPPING.keys()}
+    collecting_key = None
+    for line in record[1:]:
+        line = line.strip()
+        if not line:
+            continue
+        match line:
+            case "New assets:":
+                collecting_key = "new_assets"
+            case "New directories:":
+                collecting_key = "new_directories"
+            case "Removed assets:":
+                collecting_key = "remove_assets"
+            case "Removed directories:":
+                collecting_key = "remove_directories"
+            case "Moved assets:":
+                collecting_key = "move_assets"
+            case "Moved directories:":
+                collecting_key = "move_directories"
+            case "Renamed directories:":
+                collecting_key = "rename_directories"
+            case "Renamed assets:":
+                collecting_key = "rename_assets"
+            case "Modified assets:":
+                collecting_key = "modify_assets"
+            case _:
+                if not collecting_key or line and not line.startswith("- "):
+                    raise RuntimeError("Invalid operations record.")
+                cleaned_line = line[2:].split(" -> ")
+                if len(cleaned_line) > 2:
+                    raise RuntimeError(f"Invalid operations record:{line}")
+                if len(cleaned_line) == 1:
+                    parsed_record[collecting_key].append(Path(cleaned_line[0]))
+                else:
+                    parsed_record[collecting_key].append(
+                        (Path(cleaned_line[0]), Path(cleaned_line[1]))
+                    )
+    return parsed_record
diff --git a/onyo/lib/inventory.py b/onyo/lib/inventory.py
index 7b192957..8d680b75 100644
--- a/onyo/lib/inventory.py
+++ b/onyo/lib/inventory.py
@@ -726,65 +726,6 @@ def raise_empty_keys(self, asset: dict | UserDict) -> None:
             # Hence, `None` as a key would show up here as 'None'.
             raise ValueError("Keys are not allowed to be empty or None-values.")
 
-    def _parse_operations_record(self, record: list[str]):
-
-        if not record[0].strip() == "--- Inventory Operations ---":
-            raise RuntimeError("Invalid operations record.")
-        parsed_record = {k: [] for k in OPERATIONS_MAPPING.keys()}
-        collecting_key = None
-        for line in record[1:]:
-            line = line.strip()
-            if not line:
-                continue
-            match line:
-                case "New assets:":
-                    collecting_key = "new_assets"
-                case "New directories:":
-                    collecting_key = "new_directories"
-                case "Removed assets:":
-                    collecting_key = "remove_assets"
-                case "Removed directories:":
-                    collecting_key = "remove_directories"
-                case "Moved assets:":
-                    collecting_key = "move_assets"
-                case "Moved directories:":
-                    collecting_key = "move_directories"
-                case "Renamed directories:":
-                    collecting_key = "rename_directories"
-                case "Renamed assets:":
-                    collecting_key = "rename_assets"
-                case "Modified assets:":
-                    collecting_key = "modify_assets"
-                case _:
-                    if not collecting_key or line and not line.startswith("- "):
-                        raise RuntimeError("Invalid operations record.")
-                    cleaned_line = line[2:].split(" -> ")
-                    if len(cleaned_line) > 2:
-                        raise RuntimeError(f"Invalid operations record:{line}")
-                    if len(cleaned_line) == 1:
-                        parsed_record[collecting_key].append(Path(cleaned_line[0]))
-                    else:
-                        parsed_record[collecting_key].append(
-                            (Path(cleaned_line[0]), Path(cleaned_line[1]))
-                        )
-        return parsed_record
-
     def get_history(self, path: Path | None = None, n: int | None = None):
-        # TODO: This isn't quite right yet. operations records are defined in Inventory.
-        #       But Inventory shouldn't talk to GitRepo directly. So, either pass the record
-        #       from Inventory to OnyoRepo and turn it into a commit-message part only,
-        #       or have sort of a proxy in OnyoRepo.
-        #       -> May be: get_history(Item) in Inventory and get_history(path) in OnyoRepo.
-
-        for commit in self.repo.git.follow(path, n):
-            record = []
-            start = False
-            for line in commit['message']:
-                if line.strip() == "--- Inventory Operations ---":
-                    start = True
-                if start:
-                    record.append(line)
-            if record:
-
-                commit['operations'] = self._parse_operations_record(record)
-            yield commit
+        # TODO: Not sure we need this one yet. We get the data from pseudo-keys.
+        yield from self.repo.get_history(path, n)
diff --git a/onyo/lib/onyo.py b/onyo/lib/onyo.py
index 7ebdea0b..901f2f45 100644
--- a/onyo/lib/onyo.py
+++ b/onyo/lib/onyo.py
@@ -738,3 +738,23 @@ def commit(self, paths: Iterable[Path] | Path, message: str):
         """
         self.git.commit(paths=paths, message=message)
         self.clear_cache()
+
+    def get_history(self, path: Path | None = None, n: int | None = None):
+        # TODO: This isn't quite right yet. operations records are defined in Inventory.
+        #       But Inventory shouldn't talk to GitRepo directly. So, either pass the record
+        #       from Inventory to OnyoRepo and turn it into a commit-message part only,
+        #       or have sort of a proxy in OnyoRepo.
+        #       -> May be: get_history(Item) in Inventory and get_history(path) in OnyoRepo.
+        from onyo.lib.decoder import parse_operations_record
+        for commit in self.git.follow(path, n):
+            record = []
+            start = False
+            for line in commit['message']:
+                if line.strip() == "--- Inventory Operations ---":
+                    start = True
+                if start:
+                    record.append(line)
+            if record:
+
+                commit['operations'] = parse_operations_record(record)
+            yield commit

From 6d3d6279ac82db6c4da755b42e9cc14b03447e3f Mon Sep 17 00:00:00 2001
From: Benjamin Poldrack <benjaminpoldrack@gmail.com>
Date: Wed, 11 Dec 2024 13:04:35 +0100
Subject: [PATCH 3/4] TMP: to be enhanced and broken into different commits

---
 onyo/lib/git.py                             |  21 +--
 onyo/lib/inventory.py                       |   3 +-
 onyo/lib/items.py                           |  74 ++++++++--
 onyo/lib/onyo.py                            |  15 +-
 onyo/lib/{decoder.py => parser.py}          |   2 +-
 onyo/lib/pseudokeys.py                      |  75 ++++++----
 onyo/lib/tests/test_commands_new.py         |   8 +-
 onyo/lib/tests/test_inventory_operations.py | 153 ++++++++++++++++++--
 onyo/lib/tests/test_item.py                 |   5 +-
 9 files changed, 285 insertions(+), 71 deletions(-)
 rename onyo/lib/{decoder.py => parser.py} (97%)

diff --git a/onyo/lib/git.py b/onyo/lib/git.py
index 8108327d..4cdedf29 100644
--- a/onyo/lib/git.py
+++ b/onyo/lib/git.py
@@ -408,8 +408,8 @@ def check_ignore(self, ignore: Path, paths: list[Path]) -> list[Path]:
     def _parse_log_output(self, lines: list[str]) -> dict:
         """Generate a dict from the output of ``git log`` for one commit.
 
-        Internal helper that gets a list of git-log-output lines,
-        that represent a single commit and generates a dictionary from it.
+        Internal helper that converts a list of git-log-output lines of
+        a single commit to a dictionary.
         """
         import datetime
         import re
@@ -418,7 +418,7 @@ def _parse_log_output(self, lines: list[str]) -> dict:
         commit = dict()
         for line in lines:
             if line.startswith('commit '):
-                commit['commit'] = line.split()[1]
+                commit['hexsha'] = line.split()[1]
                 continue
             elif line.startswith('Author:'):
                 try:
@@ -450,21 +450,24 @@ def _parse_log_output(self, lines: list[str]) -> dict:
 
         return commit
 
-    def follow(self, path: Path | None = None, n: int | None = None) -> Generator[dict, None, None]:
+    def history(self, path: Path | None = None, n: int | None = None) -> Generator[dict, None, None]:
         """Yields commit dicts representing the history of ``path``.
 
-        History according to ``git log --follow`` if a path is given
-        or ``git log`` otherwise.
+        History according to git log (git log --follow if a path is given).
 
         Parameters
         ----------
-        path: Path, optional
+        path:
           What file to follow. If `None`, get the history of HEAD instead.
-        n: int, optional
+        n:
           Limit history going back ``n`` commits.
           ``None`` for no limit (default).
         """
-        # TODO: Do we want to distinguish methods for `git log` (path optional) vs `git log --follow` (requires a path)?
+        # TODO: Something like this may allow us to efficiently deal with multiline strings in git-log's output,
+        #       rather than splitting lines and iterating over them when assembling output that belongs to a single
+        #       commit, parsing the git data and then the operations record (also removes leading spaces for the commit
+        #       message):
+        #       --pretty='format:commit: %H%nAuthor: %an (%ae)%nCommitter: %cn (%ce)%nCommitDate: %cI%nMessage:%n%B'
         limit = [f'-n{n}'] if n is not None else []
         pathspec = ['--follow', '--', str(path)] if path else []
         cmd = ['log', '--date=iso-strict', '--pretty=fuller'] + limit + pathspec
diff --git a/onyo/lib/inventory.py b/onyo/lib/inventory.py
index 8d680b75..37e7dd5f 100644
--- a/onyo/lib/inventory.py
+++ b/onyo/lib/inventory.py
@@ -726,6 +726,5 @@ def raise_empty_keys(self, asset: dict | UserDict) -> None:
             # Hence, `None` as a key would show up here as 'None'.
             raise ValueError("Keys are not allowed to be empty or None-values.")
 
-    def get_history(self, path: Path | None = None, n: int | None = None):
-        # TODO: Not sure we need this one yet. We get the data from pseudo-keys.
+    def get_history(self, path: Path | None = None, n: int | None = None) -> Generator[UserDict, None, None]:
         yield from self.repo.get_history(path, n)
diff --git a/onyo/lib/items.py b/onyo/lib/items.py
index e364d002..a97debcd 100644
--- a/onyo/lib/items.py
+++ b/onyo/lib/items.py
@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+from copy import deepcopy
 from pathlib import Path
 from typing import TYPE_CHECKING
 
@@ -51,7 +52,10 @@ def __init__(self,
         self.update(onyo.lib.pseudokeys.PSEUDO_KEYS)
         self._path = None
 
-        if isinstance(item, Path):
+        if isinstance(item, Item):
+            self._path = item._path
+            self.data = deepcopy(item.data)
+        elif isinstance(item, Path):
             assert item.is_absolute()  # currently no support for relative. This is how all existing code should work ATM.
             self.update_from_path(item)
         elif item is not None:
@@ -95,15 +99,51 @@ def update_from_path(self, path: Path):
             loader = self.repo.get_asset_content if self.repo else get_asset_content
             self.update(loader(path))
 
-    # TODO: git metadata:
-    # def fill_created(self, what: str | None):
-    #     # self.repo.git.xxx   (self.get('onyo.path.file'))
-    #     # fill in all created keys
-    #     # switch `what` -> return whatever part was requesting this
-    #     pass#raise NotImplementedError
-    #
-    # def fill_modified(self, what: str | None):
-    #     pass#raise NotImplementedError
+    def fill_created(self, what: str | None):
+        """Initializer for the 'onyo.was.modified' pseudo-keys.
+
+        Fills in the entire sub-dict and returns the value specified by ``what``.
+
+        Note/TODO:
+        ----------
+        This is currently based on ``git log --follow <path>``. Looking back, the first appearance
+        of a 'new_assets'/'new_directories' operation should be it, assuming the history
+        was created by onyo commands.
+        However, if the history was created using the python interface, that assumption wouldn't hold
+        and we'd have to trace back moves and renames in order to know what path or file name we are looking
+        to match against these operations.
+        """
+        if self.repo and self['onyo.path.absolute']:
+            for commit in self.repo.get_history(self['onyo.path.file']):  # pyre-ignore[16]
+                if 'operations' in commit:
+                    if (self['onyo.is.asset'] and commit['operations']['new_assets']) or \
+                            (self['onyo.is.directory'] and commit['operations']['new_directories']):
+                        self['onyo.was.created'] = commit.data
+                        return commit[what] if what else None
+            return None
+
+    def fill_modified(self, what: str | None):
+        """Initializer for the 'onyo.was.modified' pseudo-keys.
+
+        Fills in the entire sub-dict and returns the value specified by ``what``.
+
+        Note/TODO:
+        ----------
+        See ``fill_created``.
+        """
+        if self.repo and self['onyo.path.absolute']:
+            for commit in self.repo.get_history(self['onyo.path.file']):  # pyre-ignore[16]
+                if 'operations' in commit:
+                    if (self['onyo.is.asset'] and
+                        (commit['operations']['modify_assets'] or
+                         commit['operations']['new_assets'])) or \
+                       (self['onyo.is.directory'] and
+                        (commit['operations']['new_directories'] or
+                         commit['operations']['move_directories'] or
+                         commit['operations']['rename_directories'])):
+                        self['onyo.was.modified'] = commit.data
+                        return commit[what] if what else None
+        return None
 
     def get_path_absolute(self):
         """Initializer for the 'onyo.path.absolute' pseudo-key."""
@@ -125,7 +165,14 @@ def get_path_parent(self):
 
     def get_path_file(self):
         """Initializer for the 'onyo.path.file' pseudo-key."""
-        return self._path.relative_to(self.repo.git.root) if self.repo and self['onyo.is.asset'] else None
+        if self.repo and self['onyo.path.absolute']:
+            if self['onyo.is.asset'] and not self['onyo.is.directory']:
+                return self['onyo.path.relative']
+            if self['onyo.is.asset'] and self['onyo.is.directory']:
+                return self['onyo.path.relative'] / onyo.lib.onyo.OnyoRepo.ASSET_DIR_FILE_NAME
+            if self['onyo.is.directory'] and not self['onyo.is.asset']:
+                return self['onyo.path.relative'] / onyo.lib.onyo.OnyoRepo.ANCHOR_FILE_NAME
+        return None
 
     def is_asset(self) -> bool | None:
         """Initializer for the 'onyo.is.asset' pseudo-key."""
@@ -147,7 +194,10 @@ def is_template(self) -> bool | None:
 
     def is_empty(self) -> bool | None:
         """Initializer for the 'onyo.is.empty' pseudo-key."""
-        return None  # TODO: Unclear what exactly this needs to consider. Probably child items? Or just assets?
+        if self['onyo.is.directory'] and self.repo and self._path:
+            # TODO: This likely can be faster when redoing/enhancing caching of repo paths.
+            return not any(p.parent == self._path for p in self.repo.get_asset_paths())
+        return None
 
 # TODO/Notes for next PR(s):
 # - Bug/Missing feature: pseudo-keys that are supposed to be settable by commands, are not yet
diff --git a/onyo/lib/onyo.py b/onyo/lib/onyo.py
index 901f2f45..5e7e243e 100644
--- a/onyo/lib/onyo.py
+++ b/onyo/lib/onyo.py
@@ -17,7 +17,11 @@
 from .utils import get_asset_content, write_asset_file
 
 if TYPE_CHECKING:
-    from typing import Iterable, List
+    from typing import (
+        Generator,
+        Iterable,
+        List,
+    )
     from collections import UserDict
 
 log: logging.Logger = logging.getLogger('onyo.onyo')
@@ -739,14 +743,15 @@ def commit(self, paths: Iterable[Path] | Path, message: str):
         self.git.commit(paths=paths, message=message)
         self.clear_cache()
 
-    def get_history(self, path: Path | None = None, n: int | None = None):
+    def get_history(self, path: Path | None = None, n: int | None = None) -> Generator[UserDict, None, None]:
         # TODO: This isn't quite right yet. operations records are defined in Inventory.
         #       But Inventory shouldn't talk to GitRepo directly. So, either pass the record
         #       from Inventory to OnyoRepo and turn it into a commit-message part only,
         #       or have sort of a proxy in OnyoRepo.
         #       -> May be: get_history(Item) in Inventory and get_history(path) in OnyoRepo.
-        from onyo.lib.decoder import parse_operations_record
-        for commit in self.git.follow(path, n):
+        from onyo.lib.parser import parse_operations_record
+        from onyo.lib.utils import DotNotationWrapper
+        for commit in self.git.history(path, n):
             record = []
             start = False
             for line in commit['message']:
@@ -757,4 +762,4 @@ def get_history(self, path: Path | None = None, n: int | None = None):
             if record:
 
                 commit['operations'] = parse_operations_record(record)
-            yield commit
+            yield DotNotationWrapper(commit)
diff --git a/onyo/lib/decoder.py b/onyo/lib/parser.py
similarity index 97%
rename from onyo/lib/decoder.py
rename to onyo/lib/parser.py
index 361859a8..662e0b4e 100644
--- a/onyo/lib/decoder.py
+++ b/onyo/lib/parser.py
@@ -3,7 +3,7 @@
 from onyo.lib.inventory import OPERATIONS_MAPPING
 
 
-def parse_operations_record(record: list[str]):
+def parse_operations_record(record: list[str]) -> dict:
 
     if not record[0].strip() == "--- Inventory Operations ---":
         raise RuntimeError("Invalid operations record.")
diff --git a/onyo/lib/pseudokeys.py b/onyo/lib/pseudokeys.py
index 8f3d2603..d1d8185b 100644
--- a/onyo/lib/pseudokeys.py
+++ b/onyo/lib/pseudokeys.py
@@ -24,6 +24,12 @@ class PseudoKey:
     This Callable is expected to have a single parameter of type `Item`.
     """
 
+    def __eq__(self, other):
+        if not isinstance(other, PseudoKey):
+            return False
+
+        # TODO: This isn't clean yet, since it relies on `implementation` being a `partial`:
+        return self.description == other.description and self.implementation.func == other.implementation.func
 
 def delegate(self: Item, attribute: str, *args, **kwargs):
     # This is to avoid circular imports ATM.
@@ -56,6 +62,48 @@ def delegate(self: Item, attribute: str, *args, **kwargs):
     'onyo.is.empty': PseudoKey(description="Is the directory empty. <unset> if the item is not a directory.",
                                implementation=partial(delegate, attribute='is_empty')
                                ),
+    'onyo.was.modified.hexsha': PseudoKey(description="SHA of the last commit that modified the item.",
+                                          implementation=partial(delegate, attribute='fill_modified', what='hexsha')
+                                          ),
+    'onyo.was.modified.time': PseudoKey(description="Time of the last commit that modified the item.",
+                                        implementation=partial(delegate, attribute='fill_modified', what='time')),
+    'onyo.was.modified.author.name': PseudoKey(
+        description="Name of the author of the last commit that modified the item.",
+        implementation=partial(delegate, attribute='fill_modified', what='author.time')
+    ),
+    'onyo.was.modified.author.email': PseudoKey(
+        description="Email of the author of the last commit that modified the item.",
+        implementation=partial(delegate, attribute='fill_modified', what='author.email')
+    ),
+    'onyo.was.modified.committer.name': PseudoKey(
+        description="Name of the committer of the last commit that modified the item.",
+        implementation=partial(delegate, attribute='fill_modified', what='committer.name')
+    ),
+    'onyo.was.modified.committer.email': PseudoKey(
+        description="Email of the committer of the last commit that modified the item.",
+        implementation=partial(delegate, attribute='fill_modified', what='committer.email')
+    ),
+    'onyo.was.created.hexsha': PseudoKey(description="SHA of the last commit that created the item.",
+                                         implementation=partial(delegate, attribute='fill_created', what='hexsha')
+                                         ),
+    'onyo.was.created.time': PseudoKey(description="Time of the last commit that created the item.",
+                                       implementation=partial(delegate, attribute='fill_created', what='time')),
+    'onyo.was.created.author.name': PseudoKey(
+        description="Name of the author of the last commit that created the item.",
+        implementation=partial(delegate, attribute='fill_created', what='author.time')
+    ),
+    'onyo.was.created.author.email': PseudoKey(
+        description="Email of the author of the last commit that created the item.",
+        implementation=partial(delegate, attribute='fill_created', what='author.email')
+    ),
+    'onyo.was.created.committer.name': PseudoKey(
+        description="Name of the committer of the last commit that created the item.",
+        implementation=partial(delegate, attribute='fill_created', what='committer.name')
+    ),
+    'onyo.was.created.committer.email': PseudoKey(
+        description="Email of the committer of the last commit that created the item.",
+        implementation=partial(delegate, attribute='fill_created', what='committer.email')
+    ),
 }
 r"""Pseudo-Keys are key names that are addressable but not written to disk in asset YAML.
 
@@ -66,33 +114,6 @@ def delegate(self: Item, attribute: str, *args, **kwargs):
 RESERVED_KEYS
 """
 
-# 'onyo.git.created.time': PseudoKey(description="Datetime the inventory item was created.",
-#                                    implementation=partial(delegate,
-#                                                           attribute='fill_created',
-#                                                           what='time')
-#                                    # or onyo.git.created.time for an "return self.get(what)" in implementation?
-#                                    ),
-# 'onyo.git.created.commit': PseudoKey(description="Commit SHA of the commit the object was created in",
-#                                      implementation=partial(delegate,
-#                                                             attribute='fill_created',
-#                                                             what='SHA')
-#                                      ),
-
-# 'onyo.git.created.committer.name': None,
-                   # 'onyo.git.created.committer.email': None,
-                   # 'onyo.git.created.author.name': None,
-                   # 'onyo.git.created.author.email': None,
-                   # 'onyo.git.modified.time': None,
-                   # 'onyo.git.modified.commit': None,
-                   # 'onyo.git.modified.committer.name': None,
-                   # 'onyo.git.modified.committer.email': None,
-                   # 'onyo.git.modified.author.name': None,
-                   # 'onyo.git.modified.author.email': None,
-                   #
-                   #          },
-                   #  }
-                   # }
-
 # Hardcode aliases for now:
 # Introduction of proper aliases requires config cache first.
 PSEUDOKEY_ALIASES: Dict[str, str] = {
diff --git a/onyo/lib/tests/test_commands_new.py b/onyo/lib/tests/test_commands_new.py
index 0c6eead0..041736f6 100644
--- a/onyo/lib/tests/test_commands_new.py
+++ b/onyo/lib/tests/test_commands_new.py
@@ -265,8 +265,12 @@ def test_onyo_new_clones(inventory: Inventory) -> None:
     assert inventory.repo.is_asset_path(new_asset_path1)
     new_asset = inventory.get_asset(new_asset_path1)
     # equals existing asset except for path-pseudo-keys and serial:
+    # Actually: history differs as well. onyo.is. doesn't, though
+    for k, v in existing_asset.items():
+        if k != "serial" and not k.startswith('onyo.path.') and not k.startswith('onyo.was.'):
+            assert v == new_asset[k], f"{k}: {v} != {new_asset[k]}"
     assert all(v == new_asset[k] for k, v in existing_asset.items()
-               if k != "serial" and not k.startswith('onyo.path'))
+               if k != "serial" and not k.startswith('onyo.path') and not k.startswith('onyo.was.'))
     assert new_asset['serial'] == 'ANOTHER'
 
     # second new asset
@@ -275,7 +279,7 @@ def test_onyo_new_clones(inventory: Inventory) -> None:
     new_asset = inventory.get_asset(new_asset_path2)
     # equals existing asset except for path-pseudo-keys and serial:
     assert all(v == new_asset[k] for k, v in existing_asset.items()
-               if k != "serial" and not k.startswith('onyo.path'))
+               if k != "serial" and not k.startswith('onyo.path') and not k.startswith('onyo.was.'))
     assert new_asset['serial'] == 'whatever'
     assert inventory.repo.git.is_clean_worktree()
 
diff --git a/onyo/lib/tests/test_inventory_operations.py b/onyo/lib/tests/test_inventory_operations.py
index 29880050..41a1a9e9 100644
--- a/onyo/lib/tests/test_inventory_operations.py
+++ b/onyo/lib/tests/test_inventory_operations.py
@@ -80,9 +80,15 @@ def test_add_asset(repo: OnyoRepo) -> None:
     assert repo.is_asset_path(asset_file)
     asset_from_disc = inventory.get_asset(asset_file)
     assert asset_file == asset_from_disc.get('onyo.path.absolute')
+    # All pseudo-keys are set, except `onyo.is.empty` which is only set for directories:
+    assert all(asset_from_disc[k] is not None for k in PSEUDO_KEYS.keys() if k != 'onyo.is.empty')
     for k, v in asset.items():
         if k not in RESERVED_KEYS + list(PSEUDO_KEYS.keys()):
             assert asset_from_disc[k] == v
+    assert asset_from_disc['onyo.path.file'] == asset_from_disc['onyo.path.relative'] == asset_file.relative_to(inventory.root)
+    assert asset_from_disc['onyo.is.asset'] is True
+    assert asset_from_disc['onyo.is.directory'] is False
+    assert asset_from_disc['onyo.is.empty'] is None
 
     # check operations record:
     commit = [c for c in inventory.get_history(asset_file, n=1)][0]
@@ -94,6 +100,8 @@ def test_add_asset(repo: OnyoRepo) -> None:
             assert newdir2.relative_to(inventory.root) in v
         else:
             assert v == []
+    # history pseudo-keys:
+    assert asset_from_disc['onyo.was.created.hexsha'] == asset_from_disc['onyo.was.modified.hexsha'] == inventory.repo.git.get_hexsha()
 
     # required keys must not be empty
     asset.update(dict(model=dict(name="")))
@@ -194,27 +202,31 @@ def test_move_asset(repo: OnyoRepo) -> None:
     assert isinstance(inventory.operations[0].operands, tuple)
     assert asset_file in inventory.operations[0].operands
     assert newdir1 in inventory.operations[0].operands
-
+    moved_asset_path = newdir1 / asset_file.name
     # nothing done on disc yet:
     assert asset_file.is_file()
-    assert not (newdir1 / asset_file.name).exists()
+    assert not moved_asset_path.exists()
 
     # TODO: test diff
 
     # now commit:
     inventory.commit("Move an asset")
     assert not asset_file.exists()
-    assert (newdir1 / asset_file.name).is_file()
+    assert moved_asset_path.is_file()
 
     # check operations record:
-    commit = [c for c in inventory.get_history(newdir1 / asset_file.name, n=1)][0]
+    commit = [c for c in inventory.get_history(moved_asset_path, n=1)][0]
     for k, v in commit['operations'].items():
         if k == 'move_assets':
             assert v == [(
-                asset_file.relative_to(inventory.root), (newdir1 / asset_file.name).relative_to(inventory.root)
+                asset_file.relative_to(inventory.root), moved_asset_path.relative_to(inventory.root)
             )]
         else:
             assert v == []
+    asset_from_disc = inventory.get_asset(moved_asset_path)
+
+    # history pseudo-keys: The move is not an asset modification:
+    assert asset_from_disc['onyo.was.created.hexsha'] == asset_from_disc['onyo.was.modified.hexsha'] == inventory.repo.git.get_hexsha('HEAD~1')
 
 
 def test_rename_asset(repo: OnyoRepo) -> None:
@@ -317,7 +329,10 @@ def test_modify_asset(repo: OnyoRepo) -> None:
 
     assert asset_on_disc['onyo.path.absolute'] == new_asset_file
     assert asset_on_disc['onyo.path.parent'] == new_asset_file.parent.relative_to(inventory.root)
+    assert asset_on_disc['onyo.path.file'] == asset_on_disc['onyo.path.relative'] == new_asset_file.relative_to(inventory.root)
     assert asset_on_disc['onyo.is.directory'] is False
+    assert asset_on_disc['onyo.is.asset'] is True
+    assert asset_on_disc['onyo.is.empty'] is None
 
     # check operations record:
     commit = [c for c in inventory.get_history(new_asset_file, n=1)][0]
@@ -329,6 +344,10 @@ def test_modify_asset(repo: OnyoRepo) -> None:
         else:
             assert v == []
 
+    # history pseudo-keys:
+    assert asset_on_disc['onyo.was.modified.hexsha'] == inventory.repo.git.get_hexsha()
+    assert asset_on_disc['onyo.was.created.hexsha'] == inventory.repo.git.get_hexsha('HEAD~1')
+
 
 def test_add_directory(repo: OnyoRepo) -> None:
     inventory = Inventory(repo)
@@ -363,6 +382,14 @@ def test_add_directory(repo: OnyoRepo) -> None:
         else:
             assert v == []
 
+    # pseudo-keys
+    newdir_item = Item(new_dir, repo=inventory.repo)
+    assert newdir_item['onyo.is.directory'] is True
+    assert newdir_item['onyo.is.asset'] is False
+    assert newdir_item['onyo.is.empty'] is True
+    assert newdir_item['onyo.path.file'] == new_dir.relative_to(inventory.root) / repo.ANCHOR_FILE_NAME
+    assert newdir_item['onyo.was.created.hexsha'] == newdir_item['onyo.was.modified.hexsha'] == inventory.repo.git.get_hexsha()
+
 
 def test_remove_directory(repo: OnyoRepo) -> None:
     inventory = Inventory(repo)
@@ -452,25 +479,34 @@ def test_move_directory(repo: OnyoRepo) -> None:
 
     # nothing happened on disc yet:
     assert newdir2.is_dir()
-    assert not (emptydir / newdir2.name).exists()
+    moved_dir = emptydir / newdir2.name
+    assert not moved_dir.exists()
 
     # TODO diff
 
     # now commit:
     inventory.commit("Move a directory")
     assert not newdir2.exists()
-    assert repo.is_inventory_dir(emptydir / newdir2.name)
+    assert repo.is_inventory_dir(moved_dir)
 
     # check operations record:
     commit = [c for c in inventory.get_history(n=1)][0]
     for k, v in commit['operations'].items():
         if k == 'move_directories':
             assert v == [(
-                newdir2.relative_to(inventory.root), (emptydir / newdir2.name).relative_to(inventory.root)
+                newdir2.relative_to(inventory.root), moved_dir.relative_to(inventory.root)
             )]
         else:
             assert v == []
 
+    # pseudo-keys
+    newdir_item = Item(moved_dir, repo=inventory.repo)
+    assert newdir_item['onyo.is.directory'] is True
+    assert newdir_item['onyo.is.asset'] is False
+    assert newdir_item['onyo.is.empty'] is False  # There's an asset within
+    assert newdir_item['onyo.was.modified.hexsha'] == inventory.repo.git.get_hexsha()
+    assert newdir_item['onyo.was.created.hexsha'] == inventory.repo.git.get_hexsha('HEAD~1')
+
 
 def test_rename_directory(repo: OnyoRepo) -> None:
     inventory = Inventory(repo)
@@ -524,6 +560,14 @@ def test_rename_directory(repo: OnyoRepo) -> None:
         else:
             assert v == []
 
+    # pseudo-keys
+    newdir_item = Item(new_name, repo=inventory.repo)
+    assert newdir_item['onyo.is.directory'] is True
+    assert newdir_item['onyo.is.asset'] is False
+    assert newdir_item['onyo.is.empty'] is False  # There's an asset within
+    assert newdir_item['onyo.was.modified.hexsha'] == inventory.repo.git.get_hexsha()
+    assert newdir_item['onyo.was.created.hexsha'] == inventory.repo.git.get_hexsha('HEAD~1')
+
 
 def test_add_asset_dir(repo: OnyoRepo) -> None:
     inventory = Inventory(repo)
@@ -572,6 +616,15 @@ def test_add_asset_dir(repo: OnyoRepo) -> None:
         else:
             assert v == []
 
+    # pseudo-keys
+    asset_from_disk = inventory.get_asset(asset_dir_path)
+    assert asset_from_disk['onyo.path.relative'] == asset_dir_path.relative_to(inventory.root)
+    assert asset_from_disk['onyo.path.file'] == asset_from_disk['onyo.path.relative'] / OnyoRepo.ASSET_DIR_FILE_NAME
+    assert asset_from_disk['onyo.is.asset'] is True
+    assert asset_from_disk['onyo.is.directory'] is True
+    assert asset_from_disk['onyo.is.empty'] is True
+    assert asset_from_disk['onyo.was.modified.hexsha'] == asset_from_disk['onyo.was.created.hexsha'] == inventory.repo.git.get_hexsha()
+
     # add asset aspect to existing directory, which does not yet comply with asset naming scheme
     dir_path = inventory.root / "newdir"
     inventory.add_directory(dir_path)
@@ -628,6 +681,17 @@ def test_add_asset_dir(repo: OnyoRepo) -> None:
         else:
             assert v == []
 
+    # pseudo-keys
+    asset_from_disk = inventory.get_asset(expected_path)
+    assert asset_from_disk['onyo.path.relative'] == expected_path.relative_to(inventory.root)
+    assert asset_from_disk['onyo.path.file'] == asset_from_disk['onyo.path.relative'] / OnyoRepo.ASSET_DIR_FILE_NAME
+    assert asset_from_disk['onyo.is.asset'] is True
+    assert asset_from_disk['onyo.is.directory'] is True
+    assert asset_from_disk['onyo.is.empty'] is True
+    # Note: 'onyo.was.created.*' considers the last commit with 'new_assets'/'new_directories' records:
+    #       Not clear yet, whether that is what we want.
+    assert asset_from_disk['onyo.was.modified.hexsha'] == asset_from_disk['onyo.was.created.hexsha'] == inventory.repo.git.get_hexsha()
+
 
 def test_add_dir_asset(repo: OnyoRepo) -> None:
     inventory = Inventory(repo)
@@ -667,6 +731,17 @@ def test_add_dir_asset(repo: OnyoRepo) -> None:
         else:
             assert v == []
 
+    # pseudo-keys
+    asset_from_disk = inventory.get_asset(asset_path)
+    assert asset_from_disk['onyo.path.relative'] == asset_path.relative_to(inventory.root)
+    assert asset_from_disk['onyo.path.file'] == asset_from_disk['onyo.path.relative'] / OnyoRepo.ASSET_DIR_FILE_NAME
+    assert asset_from_disk['onyo.is.asset'] is True
+    assert asset_from_disk['onyo.is.directory'] is True
+    assert asset_from_disk['onyo.is.empty'] is True
+    # Note: 'onyo.was.created.*' considers the last commit with 'new_assets'/'new_directories' records:
+    #       Not clear yet, whether that is what we want.
+    assert asset_from_disk['onyo.was.modified.hexsha'] == asset_from_disk['onyo.was.created.hexsha'] == inventory.repo.git.get_hexsha()
+
 
 def test_remove_asset_dir_directory(repo: OnyoRepo) -> None:
     inventory = Inventory(repo)
@@ -717,6 +792,16 @@ def test_remove_asset_dir_directory(repo: OnyoRepo) -> None:
         else:
             assert v == []
 
+    # pseudo-keys
+    asset_from_disk = inventory.get_asset(asset_dir_path)
+    assert asset_from_disk['onyo.path.relative'] == asset_dir_path.relative_to(inventory.root)
+    assert asset_from_disk['onyo.path.file'] == asset_from_disk['onyo.path.relative']
+    assert asset_from_disk['onyo.is.asset'] is True
+    assert asset_from_disk['onyo.is.directory'] is False
+    assert asset_from_disk['onyo.is.empty'] is None
+    # Note: Removal of directory aspect doesn't count as asset modification:
+    assert asset_from_disk['onyo.was.modified.hexsha'] == asset_from_disk['onyo.was.created.hexsha'] == inventory.repo.git.get_hexsha('HEAD~1')
+
 
 def test_remove_asset_dir_asset(repo: OnyoRepo) -> None:
     inventory = Inventory(repo)
@@ -767,6 +852,16 @@ def test_remove_asset_dir_asset(repo: OnyoRepo) -> None:
         else:
             assert v == []
 
+    # pseudo-keys
+    asset_from_disk = inventory.get_asset(asset_dir_path)
+    assert asset_from_disk['onyo.path.relative'] == asset_dir_path.relative_to(inventory.root)
+    assert asset_from_disk['onyo.path.file'] == asset_from_disk['onyo.path.relative'] / OnyoRepo.ANCHOR_FILE_NAME
+    assert asset_from_disk['onyo.is.asset'] is False
+    assert asset_from_disk['onyo.is.directory'] is True
+    assert asset_from_disk['onyo.is.empty'] is False  # asset_within still exists
+    # Note: Removal of asset aspect doesn't count as directory modification:
+    assert asset_from_disk['onyo.was.modified.hexsha'] == asset_from_disk['onyo.was.created.hexsha'] == inventory.repo.git.get_hexsha('HEAD~1')
+
 
 def test_move_asset_dir(repo: OnyoRepo) -> None:
     # An asset dir could be moved by either move_dir or move_asset. Since it's both, there's no difference when we treat
@@ -842,6 +937,16 @@ def test_move_asset_dir(repo: OnyoRepo) -> None:
         else:
             assert v == []
 
+    # pseudo-keys
+    asset_from_disk = inventory.get_asset(asset_dir_path)
+    assert asset_from_disk['onyo.path.relative'] == asset_dir_path.relative_to(inventory.root)
+    assert asset_from_disk['onyo.path.file'] == asset_from_disk['onyo.path.relative'] / OnyoRepo.ASSET_DIR_FILE_NAME
+    assert asset_from_disk['onyo.is.asset'] is True
+    assert asset_from_disk['onyo.is.directory'] is True
+    assert asset_from_disk['onyo.is.empty'] is True
+    assert asset_from_disk['onyo.was.modified.hexsha'] == inventory.repo.git.get_hexsha()
+    assert asset_from_disk['onyo.was.created.hexsha'] == inventory.repo.git.get_hexsha('HEAD~2')
+
 
 def test_rename_asset_dir(repo: OnyoRepo) -> None:
     # While an asset dir is both - an asset and a dir - it can't be renamed by a rename_dir operations, because it
@@ -900,6 +1005,16 @@ def test_rename_asset_dir(repo: OnyoRepo) -> None:
         else:
             assert v == []
 
+    # pseudo-keys
+    asset_from_disk = inventory.get_asset(new_asset_dir_path)
+    assert asset_from_disk['onyo.path.relative'] == new_asset_dir_path.relative_to(inventory.root)
+    assert asset_from_disk['onyo.path.file'] == asset_from_disk['onyo.path.relative'] / OnyoRepo.ASSET_DIR_FILE_NAME
+    assert asset_from_disk['onyo.is.asset'] is True
+    assert asset_from_disk['onyo.is.directory'] is True
+    assert asset_from_disk['onyo.is.empty'] is True
+    assert asset_from_disk['onyo.was.modified.hexsha'] == inventory.repo.git.get_hexsha()
+    assert asset_from_disk['onyo.was.created.hexsha'] == inventory.repo.git.get_hexsha('HEAD~2')
+
 
 def test_modify_asset_dir(repo: OnyoRepo) -> None:
     # This should make no difference to modifying any other asset
@@ -907,11 +1022,11 @@ def test_modify_asset_dir(repo: OnyoRepo) -> None:
     inventory = Inventory(repo)
     newdir1 = repo.git.root / "somewhere"
     newdir2 = newdir1 / "new"
-    asset_path = newdir2 / "TYPE_MAKER_MODEL.SERIAL"
+    asset_path = newdir2 / "TYPE_MAKE_MODEL.SERIAL"
     asset = Item(
         {"some_key": "some_value",
          "type": "TYPE",
-         "make": "MAKER",
+         "make": "MAKE",
          "model": dict(name="MODEL"),
          "serial": "SERIAL",
          "other": 1,
@@ -936,7 +1051,7 @@ def test_modify_asset_dir(repo: OnyoRepo) -> None:
     inventory.modify_asset(asset, new_asset)
     # modify operation:
     assert num_operations(inventory, 'modify_assets') == 1
-    new_asset_path = newdir2 / "TYPE_MAKER_CORRECTED-MODEL.SERIAL"
+    new_asset_path = newdir2 / "TYPE_MAKE_CORRECTED-MODEL.SERIAL"
     assert (asset, new_asset) == inventory.operations[0].operands
     # implicit rename operation:
     assert num_operations(inventory, 'rename_assets') == 1
@@ -945,7 +1060,11 @@ def test_modify_asset_dir(repo: OnyoRepo) -> None:
     # nothing done on disc yet:
     assert inventory.repo.is_asset_dir(asset_path)
     assert not new_asset_path.exists()
-    assert asset == inventory.get_asset(asset_path)
+    asset_on_disc = inventory.get_asset(asset_path)
+    for k1, v1 in asset_on_disc.items():
+        assert asset[k1] == v1
+    for k2, v2 in asset.items():
+        assert asset_on_disc[k2] == v2
     assert inventory.repo.git.is_clean_worktree()
 
     # now commit:
@@ -973,3 +1092,13 @@ def test_modify_asset_dir(repo: OnyoRepo) -> None:
             assert v == [(asset_path.relative_to(inventory.root), new_asset_path.relative_to(inventory.root))]
         else:
             assert v == []
+
+    # pseudo-keys
+    asset_from_disk = inventory.get_asset(new_asset_path)
+    assert asset_from_disk['onyo.path.relative'] == new_asset_path.relative_to(inventory.root)
+    assert asset_from_disk['onyo.path.file'] == asset_from_disk['onyo.path.relative'] / OnyoRepo.ASSET_DIR_FILE_NAME
+    assert asset_from_disk['onyo.is.asset'] is True
+    assert asset_from_disk['onyo.is.directory'] is True
+    assert asset_from_disk['onyo.is.empty'] is True
+    assert asset_from_disk['onyo.was.modified.hexsha'] == inventory.repo.git.get_hexsha()
+    assert asset_from_disk['onyo.was.created.hexsha'] == inventory.repo.git.get_hexsha('HEAD~1')
diff --git a/onyo/lib/tests/test_item.py b/onyo/lib/tests/test_item.py
index 0195e787..57de51da 100644
--- a/onyo/lib/tests/test_item.py
+++ b/onyo/lib/tests/test_item.py
@@ -2,6 +2,7 @@
 
 import pytest
 
+from onyo.lib.onyo import OnyoRepo
 from ..items import Item
 from ..pseudokeys import PSEUDO_KEYS, PSEUDOKEY_ALIASES
 
@@ -51,10 +52,12 @@ def test_item_init(onyorepo) -> None:
             assert item.get('onyo.path.parent') is None
         # Check actual paths:
         if idx == 4:
+            # item is a dir in a repo
             assert item.get('onyo.path.absolute') == onyorepo.test_annotation['dirs'][0]
             assert item.get('onyo.path.relative') == onyorepo.test_annotation['dirs'][0].relative_to(onyorepo.git.root)
             assert item.get('onyo.path.parent') == onyorepo.test_annotation['dirs'][0].parent.relative_to(onyorepo.git.root)
-            assert item.get('onyo.path.file') is None
+            assert item.get('onyo.path.file') == onyorepo.test_annotation['dirs'][0].relative_to(onyorepo.git.root) / OnyoRepo.ANCHOR_FILE_NAME
+            assert item.get('onyo.is.empty') is not None
         elif idx == 5:
             assert item.get('onyo.path.absolute') == onyorepo.test_annotation['assets'][0]['onyo.path.absolute']
             assert item.get('onyo.path.relative') == onyorepo.test_annotation['assets'][0]['onyo.path.absolute'].relative_to(onyorepo.git.root)

From 15ebf32158a174eb682afcbec388d26f5cc8d4cf Mon Sep 17 00:00:00 2001
From: Benjamin Poldrack <benjaminpoldrack@gmail.com>
Date: Tue, 17 Dec 2024 11:44:55 +0100
Subject: [PATCH 4/4] BF: Don't end up with unset pseudo-keys after edit

When reading an asset's content from the temporary file for editing
(``onyo new --edit``/``onyo edit``), do not remove the `Item`'s
pseudo-keys. This happened, because we overwrite the ``.data`` attribute
of the ``Item`` directly. That's necessary for ruamel's comment
roundtrip, but we need to put the pseudo-key definitions back in
afterwards.
Failing to do so, resulted in unset pseudo-keys, which in case of
``onyo.path.relative`` then generated commit messages with ``None`` as
the asset path.

Closes #739
---
 onyo/lib/commands.py                | 8 ++++++--
 onyo/lib/tests/test_commands_new.py | 6 ++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/onyo/lib/commands.py b/onyo/lib/commands.py
index 68bb41e2..8d86fb97 100644
--- a/onyo/lib/commands.py
+++ b/onyo/lib/commands.py
@@ -296,7 +296,7 @@ def _edit_asset(inventory: Inventory,
     # Store original pseudo-keys of `asset`, in order to re-assign
     # them when loading edited file from disc. This is relevant, when
     # `operation` uses them (`Inventory.add_asset/modify_asset`).
-    reserved_keys = {k: v for k, v in asset.items() if k in list(PSEUDO_KEYS.keys()) + ['template']}
+    reserved_keys = {k: v for k, v in asset.items() if k in list(PSEUDO_KEYS.keys()) + ['template'] and v is not None}
     disallowed_keys = RESERVED_KEYS + list(PSEUDO_KEYS.keys())
     disallowed_keys.remove('onyo.is.directory')
     disallowed_keys.remove('onyo.path.parent')
@@ -332,7 +332,11 @@ def _edit_asset(inventory: Inventory,
             if any(k in disallowed_keys for k in tmp_asset.keys()):
                 raise ValueError(f"Can't set any of the keys ({', '.join(disallowed_keys)}).")
             asset = Item()
-            asset.data = tmp_asset.data  # keep exact objects for ruamel-based roundtrip
+            # keep exact objects for ruamel-based roundtrip:
+            asset.data = tmp_asset.data
+            # ^ This kills pseudo-keys, though. Re-add those, that aren't specified:
+            asset.update({k: v for k, v in PSEUDO_KEYS.items() if k not in tmp_asset})
+
             # When reading from file, we don't get reserved keys back, since they are not
             # part of the file content. We do need the object from reading the file to be
             # the basis, though, to get comment roundtrip from ruamel.
diff --git a/onyo/lib/tests/test_commands_new.py b/onyo/lib/tests/test_commands_new.py
index 041736f6..5c106bc9 100644
--- a/onyo/lib/tests/test_commands_new.py
+++ b/onyo/lib/tests/test_commands_new.py
@@ -191,7 +191,7 @@ def test_onyo_new_creates_directories(inventory: Inventory) -> None:
 @pytest.mark.ui({'yes': True})
 def test_onyo_new_edit(inventory: Inventory, monkeypatch) -> None:
     directory = inventory.root / "edited"
-    monkeypatch.setenv('EDITOR', "printf 'key: value' >>")
+    monkeypatch.setenv('EDITOR', "printf 'key: value  #w/ comment' >>")
 
     specs = [{'template': 'empty',
               'model': {'name': 'MODEL'},
@@ -207,7 +207,8 @@ def test_onyo_new_edit(inventory: Inventory, monkeypatch) -> None:
     assert expected_path in inventory.repo.git.files
     asset_content = inventory.get_asset(expected_path)
     assert asset_content['key'] == 'value'
-
+    assert 'key: value  #w/ comment' in expected_path.read_text()
+    assert 'None' not in list(inventory.get_history(expected_path, n=1))[0]['message']
     # file already exists:
     edit_str = "model:\n  name: MODEL\nmake: MAKER\ntype: TYPE\n"
     monkeypatch.setenv('EDITOR', f"printf '{edit_str}' >>")
@@ -240,6 +241,7 @@ def test_onyo_new_edit(inventory: Inventory, monkeypatch) -> None:
     expected_content = '---\n' + edit_str
     expected_path = directory / "TYPE_MAKER_MODEL.8675309"
     assert expected_content == expected_path.read_text()
+    assert 'None' not in list(inventory.get_history(expected_path, n=1))[0]['message']
 
 
 @pytest.mark.ui({'yes': True})