Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inventory methods should consistently account for pending operations #546

Open
bpoldrack opened this issue Mar 4, 2024 · 1 comment
Open

Comments

@bpoldrack
Copy link
Collaborator

This is most important for enabling the full potential of python API (and future commands/features by extension).

Inventory methods catch invalid cases based on assessing whether something is or isn't an asset, dir, etc.
They would need to systematically account for the impact of already pending operations, though.
This involves a change to executors that currently report paths to stage/commit. That reporting needs to be structured and more informative. For example, we'd need to now what paths may be new or removed by an operation, so that subsequent operations can tell, whether an input will is expected to be valid at the point of execution.

One flavor of that is addressing #426 (move and rename dir in one go): First operation moves src to dst1, second one is renaming dst1 to dst2. Both think that dst1 is a path that needs committing, but at the point of committing that path doesn't exist and was never known to git.

bpoldrack added a commit that referenced this issue Mar 5, 2024
This is a hack, enabling `onyo mv` to move and rename in one go (but
record two operations) w/o running into the problem of both operations
"thinking" that the intermediate path needs committing when in fact that
path does not exist at this point and was never known to git.

Q&D solution; Should be redone by properly addressing issue #546
bpoldrack added a commit that referenced this issue Mar 5, 2024
This path allows to rename a directory that comes into existence only by
already pending operations.
This is a bit hacky and should be done more systematically across
operations (see issue #546).
TobiasKadelka pushed a commit that referenced this issue Mar 5, 2024
- Fixes `onyo rm` to account for already pending removals. This should
  be done more consistently (see #546).
- Fixes `onyo rm` to fail when trying to remove the inventory's root
  dir.

Note, that changes in `Inventory.remove_directory` are mostly about
recursive removal of directories. For example, changing a condition to
recursively call itself on `path.is_dir` rather than considering
`is_inventory_dir` is done, because the latter, more restrictive
condition would be checked again during recursion anyway.

Refactoring of the "dummy operation" for generic file removal is done,
so it can be easily accounted for by `Inventory._get_pending_removals`,
which would be harder to do if it remained that unnamed ad-hoc
operation.

Lastly, note that the unskipped test's referral to #309 was wrong to
begin with - #519 is what it was about.

(Closes #519)
(Closes #520)
@bpoldrack
Copy link
Collaborator Author

Note: Based on "hacky" solutions so far, it seems like Inventory should have equivalents to OnyoRepo.is_inventory_dir and the likes, that would additionally check the pending operations (whereas the repo methods only check what's already in the repo).
Inventory methods would then simply have their conditions based on these instead and otherwise the logic remains the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant