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

hood: recursive |tomb #6056

Merged
merged 10 commits into from
Mar 1, 2023
Merged

hood: recursive |tomb #6056

merged 10 commits into from
Mar 1, 2023

Conversation

ryjm
Copy link
Contributor

@ryjm ryjm commented Nov 6, 2022

|tomb-lobe uses %lobe which ended up being much faster.

Latter uses %tomb %lobe instead of %worn.
@zalberico
Copy link
Collaborator

@ryjm can you clarify what this is actually for or write up and link it to an issue about what problem this is solving? As is it's hard for someone to evaluate and approve/merge.

@ryjm
Copy link
Contributor Author

ryjm commented Nov 9, 2022

@ryjm can you clarify what this is actually for or write up and link it to an issue about what problem this is solving? As is it's hard for someone to evaluate and approve/merge.

pairs with #6057 somewhat - sometimes you want to remove/tombstone every file in a directory.

both of these PRs are rather small changes and only touch generators, so they're mostly QoL stuff.

@zalberico
Copy link
Collaborator

zalberico commented Nov 9, 2022

@ryjm thanks - @belisarius222, @philipcmonk adding you to this for review for approval/merge (generally I'm trying to get things that can merge to merge so we don't have PRs open unnecessarily).

@ryjm in future PRs/issues can you default to having a bit more context in the details for the change? thanks!

@zalberico
Copy link
Collaborator

@belisarius222 ping on this one, trying to keep some of these that look ready moving along

@belisarius222
Copy link
Contributor

@ryjm Can you please explain the intended behavior of this generator? Then I'll review it.

@jalehman
Copy link
Member

jalehman commented Feb 3, 2023

@ryjm Please comment.

@ryjm
Copy link
Contributor Author

ryjm commented Feb 4, 2023

Currently you can only tombstone single files and not dirs - I made the original |tomb recursive, and added a new generator that's also recursive but much faster, using %lobe. PR'd both just to cover all bases.

Happy to update to coalesce into one generator, just was hoping to get some feedback on which strategy is preferable, why using %lobe is so much faster, etc.

Copy link
Contributor

@philipcmonk philipcmonk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes sense, just some formatting issues

%- zing
=- (turn - notes)
=- (turn lobes -)
|= =lobe
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation

pkg/arvo/gen/hood/tomb.hoon Outdated Show resolved Hide resolved
@ryjm
Copy link
Contributor Author

ryjm commented Feb 6, 2023

i don't have write access anymore, should i make a new PR? here's a patch if someone wants to just apply it instead.

From af17b3ad52b5aa11e4b86009b2c1aecbf9c7bf2b Mon Sep 17 00:00:00 2001
From: ryjm <jraydermiller@gmail.com>
Date: Mon, 6 Feb 2023 13:51:29 -0500
Subject: [PATCH] fix indentation

---
 pkg/arvo/gen/hood/tomb-lobe.hoon |  2 +-
 pkg/arvo/gen/hood/tomb.hoon      | 24 ++++++++++++------------
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/pkg/arvo/gen/hood/tomb-lobe.hoon b/pkg/arvo/gen/hood/tomb-lobe.hoon
index 6668ed4966..4987c283e2 100644
--- a/pkg/arvo/gen/hood/tomb-lobe.hoon
+++ b/pkg/arvo/gen/hood/tomb-lobe.hoon
@@ -8,7 +8,7 @@
 |=  [[now=@da eny=@uvJ bec=beak] [target=path ~] dry=_|]
 :-  %helm-pans
 =;  cards
-?.  dry  cards
+  ?.  dry  cards
   %-  (slog leaf+"card: {<cards>}" ~)  ~
 =|  lubs=(list note-arvo)
 |-  ^-  (list note-arvo)
diff --git a/pkg/arvo/gen/hood/tomb.hoon b/pkg/arvo/gen/hood/tomb.hoon
index 5db418fa9c..8f77e0377f 100644
--- a/pkg/arvo/gen/hood/tomb.hoon
+++ b/pkg/arvo/gen/hood/tomb.hoon
@@ -54,18 +54,18 @@
   --
 ::
 ++  lobes
-=|  lubs=(list lobe)
-|-  ^-  (list lobe)
-=+  .^(=arch %cy target)
-?~  fil.arch
-  =/  dirs  ~(tap by dir.arch)
-  %-  zing
-  %+  turn  dirs
-  |=  [kid=@ta ~]
-  =/  paf=path  /[kid]
-  =/  kud=path  `path`(weld target /[kid])
-  ^$(target kud)
- (snoc lubs u.fil.arch)
+  =|  lubs=(list lobe)
+  |-  ^-  (list lobe)
+  =+  .^(=arch %cy target)
+  ?~  fil.arch
+    =/  dirs  ~(tap by dir.arch)
+    %-  zing
+    %+  turn  dirs
+    |=  [kid=@ta ~]
+      =/  paf=path  /[kid]
+      =/  kud=path  `path`(weld target /[kid])
+      ^$(target kud)
+  (snoc lubs u.fil.arch)
 ::
 ++  notes
   |=  norms=(set [ship desk tako norm path])
-- 
2.37.3

mopfel-winrux and others added 2 commits February 8, 2023 10:03
indentation; also removed a superfluous |- just after a |^
@jalehman
Copy link
Member

jalehman commented Feb 9, 2023

@mcevoypeter Any idea why the mingw build is running on this branch?

@mcevoypeter
Copy link

mcevoypeter commented Feb 9, 2023

Any idea why the mingw build is running on this branch?

@jalehman which job are you referring to? The jobs I see under the Checks tab of this PR both show it only running on ubuntu-latest.

@philipcmonk
Copy link
Contributor

The branch was out of date, Jake just merged develop into it

wasn't properly descending into directories next to files with the same
name.

now offers to remove files that can't be tombstoned. can choose to
remove the file at the head of the current desk or from the files on
other desks sharing the same hash.
@ryjm
Copy link
Contributor Author

ryjm commented Feb 10, 2023

@philipcmonk @belisarius222

i added a fallback to |rm option and fixed the recursion (wasn't working on files/dirs sharing the same name). probably needs another review if you don't mind.

if you're trying to tombstone at the head of the desk, you probably
don't know what you're doing. so we abort.

we keep the option to `|rm` any matching hashes in other desks since
this is something the tombstoner might not know exists in advance and is
actively blocking them from completing the desired tombstone operation.
belisarius222
belisarius222 previously approved these changes Mar 1, 2023
@belisarius222
Copy link
Contributor

CI is borked. Merging.

@belisarius222 belisarius222 merged commit ccc3527 into develop Mar 1, 2023
@belisarius222 belisarius222 deleted the jm/tomb-recurse branch March 1, 2023 22:29
@jalehman jalehman restored the jm/tomb-recurse branch March 17, 2023 15:23
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

Successfully merging this pull request may close these issues.

None yet

7 participants