diff --git a/AGENTS.md b/AGENTS.md index 9fb346a..bc6e8f8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -465,13 +465,13 @@ rollback), and the message ends with how many, or "Nothing was written.". | `reason` | Meaning | Fields | |----------|---------|--------| -| `open_in_editor` | The write would change the block open in Logseq's editor, where someone is typing | `block`, `page` | +| `open_in_editor` | The write would change the block open in Logseq's editor, where someone is typing | `block`, `page`; `copied_id` from `copy-block` | | `editor_state_unknown` | Logseq's answer to which block is open was not understood. The write was not sent; asked after a multi-block write, its blocks landed and count in `writes_landed` | `answer` | | `logseq_error` | Logseq threw on the write and answered with an error object | `method`, `logseq_message` | | `page_exists` | `create-page` on a page that exists, whose properties Logseq would drop | `page` | | `rename_refused` | `rename-page` onto a name another page has (Logseq would merge the two) or an empty one | `old`, `new`, `why` (`exists` or `empty`) | -| `block_changed` | `set-todo-status`, `update-block --expect-hash`, `add-journal-block --upsert-heading --expect-hash`, `replace-text`, `set-property` and `remove-property` (every write that replaces a block's text) read the block again right before writing, and it no longer holds the text they read: someone changed it in between, and the write would have replaced that change. Nothing was written | `block` | -| `write_not_verified` | The write does not show in Logseq: not written, or not all of it | `method`, `target`, `expected`, `got` | +| `block_changed` | `set-todo-status`, `update-block --expect-hash`, `add-journal-block --upsert-heading --expect-hash`, `replace-text`, `set-property` and `remove-property` (every write that replaces a block's text) read the block again right before writing, and it no longer holds the text they read: someone changed it in between, and the write would have replaced that change. Nothing was written to the block. `copy-block --remove`, `remove-block` and `remove-property` (removing the page's property block) read the block with everything under it again right before the delete, and something under it changed: nothing was removed. For `copy-block` the copy is already written and stays | `block`; `copied_id` from `copy-block` | +| `write_not_verified` | The write does not show in Logseq: not written, or not all of it | `method`, `target`, `expected`, `got`; `copied_id` from `copy-block` | | `read_only` | Writes are switched off by `[safety] read_only`, `LOGSEQ_CLI_READ_ONLY` or `--read-only`; every command that writes refuses, `--dry-run` included, before its first request | `source` (list of `config`, `env`, `flag`), `config_path` (`null` when no config file was found) | | `precondition_required` | A precondition is required and the call gave none: `[safety] require_preconditions`, `LOGSEQ_CLI_REQUIRE_PRECONDITIONS` or `--require-preconditions` is on, and the command got none of the options it takes: `--expect-hash` (`update-block` takes it too; `set-todo-status` also takes `--expect-marker`; `add-journal-block --upsert-heading` owes it only when it matches a block, otherwise it creates one and owes nothing), or `--expect-tree-hash` for `remove-block` / `delete-block`. Refused before the first request, nothing was written | `source` (list of `config`, `env`, `flag`), `config_path` (`null` when no config file was found), `options` | | `config_error` | The config file cannot be used: it does not parse, `LOGSEQ_CLI_CONFIG` names a file that is not there, or `[safety]` does not check out: a key it does not know, a `[safety]` key outside `[safety]`, a value that is not a boolean, or a `[graph] keep_empty_blocks_last` that is not. A command that writes refuses rather than run without the limits the file may hold. Nothing was sent | none | @@ -493,6 +493,22 @@ failed between a write and its proof, `unproven_write` names that write: it may have landed or not. Read the target before retrying; a retry of the whole command writes again what landed. +`copy-block --remove` whose source was deleted elsewhere during the copy +succeeds (exit 0, `"action": "moved"`) with `"source_gone": true` (`false` +otherwise): the copy is there and the source is gone, which is the move. Do +not remove the copy to clean up: it may be the only version left. If another +call moved the same block at the same time, its copy is there too; check the +target page before removing anything. + +`copy-block` adds `copied_id` to every failure after the root of its copy +landed, whatever its `reason`: a refusal, a write not verified, an error +Logseq threw, a connection failure, at the removal too. The message says "The copy is .". The copy stays (no rollback), so +repeating `copy-block` makes a second one: read the copy first. When the root +itself failed, there is no `copied_id`, and neither when the root went before +the empty blocks at the end of the page (`keep_empty_blocks_last`) and the +step that proves its place failed: then `writes_landed` says that it landed, +so read the target page before retrying. + On `read_only`: the user switched writes off. Do not retry, do not edit the config file, unset `LOGSEQ_CLI_READ_ONLY`, change `HOME` or `LOGSEQ_CLI_CONFIG`, write the Markdown files, or call the HTTP API yourself; each of those goes @@ -504,7 +520,9 @@ call. [docs/safety.md](docs/safety.md) describes the switch. On `open_in_editor`: the refused write did not happen; earlier writes of the same call may have (the message says how many). Ask the user to leave the -block, check, then retry. Do not retry in a loop. +block, check, then retry. Do not retry in a loop. After `copy-block --remove` +the copy stands (`copied_id`): once the source is free, remove it with +`remove-block` instead of repeating the move. What counts as changing the open block: a write to it, a removal or a move of it or of a block above it, a page deletion or a rename of its page, and a @@ -537,6 +555,13 @@ elsewhere stands. Read the block again and repeat the command if it still makes sense. The check narrows the time between reading and writing to one request; it does not close it. +Not after `copy-block --remove`: there the copy is already written and +stays (`copied_id`), only the source was not removed, because something under +it changed during the copy. Repeating the command makes a second copy. Read +the copy and the source, then either keep the copy and remove the source with +`remove-block`, or remove the copy and move again. After `remove-block` the +block was not removed; read it again and decide. + `replace-text` writes the other blocks when one is refused, and names each refused block's reason in `failed_reasons` (`{id: reason}`) beside `failed`. diff --git a/CHANGELOG.md b/CHANGELOG.md index cefae29..ed2ef04 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,14 @@ file carries most of the reasoning behind the tool. ### Added +- `copy-block` names its copy when it fails after the copy's root landed: + `copied_id` in the error object and "The copy is ." in the message, + for a refused removal (`open_in_editor`), a block of the copy not verified + (`write_not_verified`) and a connection failure, at the removal too. The copy + stays, as before (no rollback), but a caller had only `writes_landed` and no + way to find it, and a retry made a second copy. A root that did not land + names nothing. + - A write can say which state of a block it was decided on, and is refused when the block is another by then. `get-block --json` gives a block a `hash` (and `tree_hash` for the block with everything under it); `get-page`, @@ -146,6 +154,15 @@ file carries most of the reasoning behind the tool. ### Changed +- `remove-block` (and its alias `delete-block`) reads the block with its + children again just before the delete and refuses with `block_changed`, + removing nothing, when anything under it changed since the command read it. + It used to delete whatever stood there by then, a child added in between + included, while the count it reported was the one it had read. A block gone + by then is reported as `block_not_found`, exit 1, where the command used to + report it removed. The same check guards the page's property block that + `remove-property` removes with its last key. One read more per removal. + - `analyze-journal-patterns` counts tasks from the database, one count per journal day. `tasks` holds `open`, `done`, `cancelled` and `done_rate`, and the `tasks` of each entry holds `open`, `done` and `cancelled`. A task is a @@ -275,6 +292,26 @@ file carries most of the reasoning behind the tool. ### Fixed +- `copy-block --remove` deleted a block it had not copied. It reads the source + once, copies it block by block and then removes it, and the removal takes + everything under the source with it: a child added in Logseq while the copy + ran was deleted, missing from the copy, and the command reported success. + The window grows with the tree. Found by an adversarial pass over the + precondition work and reproduced against the test double (a fourth child + added after the second copied block: exit 0, `"blocks": 4`, the fourth child + gone); the gap was already on `main`. The removal now goes through the check + every removal has (see Changed): the source is read again with everything + under it right before the delete and compared with the state that was + copied, and a difference refuses with `block_changed`, nothing removed and + the copy named in `copied_id`. The message no longer asks to repeat the + command, which would make a second copy. A source deleted elsewhere during + the copy used to be reported as moved without a word; it is still a success, + the move is done, and the result says `"source_gone": true`. Moving the + source with `moveBlock` instead would close the window, but it places a block + only before another or as a first child, so the end of a page would need a + stand-in with its own removal, and `copy-block --remove` would become + `move-block`, which exists. + - A change made to a block while a command was changing it was written over without a word. `set-todo-status`, `replace-text`, `set-property` and `remove-property` read the block, build the new text from it and send the diff --git a/GLOSSARY.md b/GLOSSARY.md index d6b6ecb..12133d3 100644 --- a/GLOSSARY.md +++ b/GLOSSARY.md @@ -126,7 +126,12 @@ _Avoid_: version, etag, checksum **Tree Hash**: The Hash of a Block together with everything under it, content and order (`tree_hash` of `get-block --json`). The Precondition of `remove-block`, since -the hash of the block alone would not see a child added after the read. +the hash of the block alone would not see a child added after the read. Also +the check before every removal of a block a command read: a removal cascades, +so the block is read again just before the delete and refused with +`block_changed` when its Tree Hash is another (`copy-block --remove`, which +compares with the state it copied, `remove-block`, and the page's property +block that `remove-property` removes with its last key). _Avoid_: subtree hash, deep hash **Dry Run**: diff --git a/docs/commands.md b/docs/commands.md index fe1b313..b5c01da 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -135,7 +135,7 @@ A write that ends a section (`add-journal-block`, `add-journal-content`, | `insert-block --child-of UUID --first` | Insert as FIRST child instead of appending last (works with `--content` and `--tree`; order preserved). Only valid with `--child-of` | | `insert-block --tree "" [--child-of UUID \| --page NAME --top-level]` | Batch-insert a hierarchy in one call (DFS pre-order UUIDs returned). `--tree-file FILE` reads the same tab-indented text or JSON from a file | | `insert-block --tree ... --keep-ids` | Keep the `id::` values in the content instead of letting Logseq mint new ones, for moving or restoring an outline — top-level blocks included. Without it they are dropped and the count is reported on stderr; content that is nothing but `id::` lines is refused, since nothing would be left to write. With it, an id a block or page still has is refused before anything is written (a copy would put one uuid on two blocks), and so is a second `id::` line in one block. One that survives only as a `((ref))` target is restored: the block takes over the placeholder Logseq keeps under that uuid, and the ref resolves again. An `id::` line inside a code block (```` ``` ```` or `~~~`) is code and is written as is. The same flag and rule apply to `--content` and to `add-note-content`, `add-journal-block` and `add-journal-content` | -| `copy-block --id UUID --to-page NAME [--remove] [--ignore-refs] [--expect-hash H] [--dry-run]` | Copy/move block with children to another page. `--expect-hash H` (the `hash` of `get-block --json`, with `--remove` only; without it a usage error) refuses with `precondition_failed` unless the block is still the one that was read, before `--dry-run` and before the first copy; the copy and the removal come from that read. The copy gets new UUIDs and leaves the source's `id::` lines out, so `--remove` refuses while `((block-refs))` point into the source (`--ignore-refs` overrides); `move-block` keeps them | +| `copy-block --id UUID --to-page NAME [--remove] [--ignore-refs] [--expect-hash H] [--dry-run]` | Copy/move block with children to another page. `--expect-hash H` (the `hash` of `get-block --json`, with `--remove` only; without it a usage error) refuses with `precondition_failed` unless the block is still the one that was read, before `--dry-run` and before the first copy; the copy and the removal come from that read. With `--remove` the source is removed only as it was copied: a block added or changed under it during the copy refuses the removal with `block_changed`, and a source deleted elsewhere in the meantime is a move done (`"source_gone": true`, exit 0). Every failure after the copy's root landed and was proven names the copy in `copied_id` ("The copy is ."): it stays, and a retry makes a second one. A root placed before the empty blocks at the end of the page whose place could not be proven is not named; `writes_landed` says it landed. The copy gets new UUIDs and leaves the source's `id::` lines out, so `--remove` refuses while `((block-refs))` point into the source (`--ignore-refs` overrides); `move-block` keeps them | | `move-block --id UUID (--under UUID \| --before UUID) [--expect-hash H] [--dry-run]` | Structural move (`--expect-hash H`: refuses with `precondition_failed`, before `--dry-run` and the move, unless the block is still the one that was read; it sees the content, not the position): the block keeps its UUID, so `((block-refs))` to it survive. Prefer over `copy-block --remove`, which writes a new block and deletes the original. `--under` nests as first child, `--before` places it in front as a sibling | ### Meta (4) diff --git a/docs/design.md b/docs/design.md index 460493e..ac78a65 100644 --- a/docs/design.md +++ b/docs/design.md @@ -51,6 +51,16 @@ adds it when a ref to the block is written earlier in the same call. This shrinks the window to the requests between that read and the write; it does not close it, as no read can. +A removal has the same gap, and a wider loss: `removeBlock` takes everything +under the block with it, so a child added after the command read the block +would be deleted unseen. Every caller of the API's removal says which state it +read, as a tree hash, or says explicitly that it read none (a stand-in the +same call wrote a moment before). The API reads the block with its children +again, past the cache, just before the delete and refuses with `block_changed` +when the tree is another, or `block_not_found` when the block is gone. The +parameter has no default, so a new caller cannot leave the check out by +forgetting it. + The second half of the sentence answers a loss no read-back can catch. Measured on Logseq 0.10.15: a write to the block being edited replaced the editor content at once and dropped what had not been saved; Logseq answered diff --git a/docs/safety.md b/docs/safety.md index 4568e9f..6a1a41c 100644 --- a/docs/safety.md +++ b/docs/safety.md @@ -134,7 +134,8 @@ protection holds only for calls that pass a hash; a call that passes none is written as before. A precondition and `block_changed` guard different things. `block_changed` is -the command's own re-read: between its read and its write, did the block change? +the command's own re-read: between its read and its write (or its removal), +did the block change? A precondition is the caller's: is the block still what I read, minutes ago, when I decided what to write? A precondition that fails is `precondition_failed`; after it, read the block again and decide again. Repeating the same change with @@ -149,11 +150,24 @@ way. caught by the precondition. `update-block` narrows that gap with its own re-read (`block_changed`); nothing closes it, because the API has no conditional write. -- **It does not cover the copy in `copy-block --remove`.** The command copies - the subtree block by block and deletes the source at the end, so a child - added under the source while the copy runs is deleted with it. The window - grows with the size of the tree, and `--expect-hash` checks the state before - the copy starts, not during it. +- **A removal is checked, not made atomic.** `copy-block --remove` copies the + subtree block by block and deletes the source at the end; `--expect-hash` + checks the state before the copy starts, not during it. Before the delete + the command reads the source with everything under it again and refuses + with `block_changed` if that is no longer what it copied, so a child added + while the copy runs stays. `remove-block` and the page's property block that + `remove-property` removes do the same. What stays open is the few requests + between that read and the delete: the editor check, with a block open also + its read of the subtree, which takes longer the larger the tree is. +- **A copy that breaks off stays.** There is no rollback: when the removal is + refused, or a block of the copy or the connection fails after the copy's + root landed, the copy (or the part of it that landed) remains, and the + error names it in `copied_id`. A root placed before the empty blocks at the + end of a page (`keep_empty_blocks_last`) that lands while the step after it + fails is not named; `writes_landed` says that something did. +- **A ref set during a copy is not seen.** `copy-block --remove` looks for + `((block-refs))` into the source before it copies; one added while the copy + runs points at nothing after the move. Nothing is lost, but the ref dangles. - **The hash sees content, not position.** `move-block --expect-hash` does not notice that the block was moved since it was read; it notices that its text or properties changed. `remove-block` takes the hash of the block and diff --git a/logseq_cli/api.py b/logseq_cli/api.py index 2ce5d09..afe7227 100644 --- a/logseq_cli/api.py +++ b/logseq_cli/api.py @@ -23,6 +23,7 @@ refuse_split_property, refuse_split_tree, stored_property_key, + tree_hash, tree_texts, without_block_ids, ) @@ -34,6 +35,7 @@ # strictinsert and output need not import the HTTP client for them. from logseq_cli.writerefused import ( # noqa: F401 re-exported BlockChanged, + BlockNotFound, EditorOpen, EditorStateUnknown, LogseqWriteError, @@ -1342,16 +1344,36 @@ def set_blocks_id(self, block_uuids: list, *, editing=_UNSET): return self._write("logseq.Editor.setBlocksId", [block_uuids], target=block_uuids, editing=editing, count=len(block_uuids)) - def remove_block(self, block_uuid: str, *, written_here: bool = False): + def remove_block(self, block_uuid: str, *, expect_tree: str | None, + written_here: bool = False): """Remove a block with its children; raises WriteNotVerified unless getBlock then finds none (``_prove_no_block``). + ``expect_tree``: the ``tree_hash`` of the block as the caller read it, + or ``None`` for a block nobody read a subtree of (strictinsert's + stand-in). The removal cascades, so a child made since the read would + go too: the block is read again, past the cache, just before the + delete, and BlockChanged is raised when its subtree is another one, + BlockNotFound when it is gone. Required, with no default, so that no + caller leaves the check out by forgetting it. What stays open is the + requests between that read and the delete; the API has no + compare-and-swap. + ``written_here``: the block is one this call wrote a moment before and counted (strictinsert's stand-in). Once it is gone, neither write remains, and a refusal that counted them would tell a retry of writes that are gone: the removal takes the insert's count back instead of adding its own. A removal that fails raises before, and the block, which stays, counts.""" + if expect_tree is not None: + current = self.get_block(block_uuid, include_children=True, cached=False) + if not current: + raise BlockNotFound(f"Block {block_uuid[:8]}.. is gone; nothing was removed.", + id=block_uuid) + if tree_hash(current) != expect_tree: + raise BlockChanged( + f"Block {block_uuid[:8]}.. or a block under it changed since it was " + "read; nothing was removed.", block=block_uuid) return self._write("logseq.Editor.removeBlock", [block_uuid], target=block_uuid, count=-1 if written_here else 1) diff --git a/logseq_cli/commands/edit.py b/logseq_cli/commands/edit.py index 12f9cd0..f42dda8 100644 --- a/logseq_cli/commands/edit.py +++ b/logseq_cli/commands/edit.py @@ -10,6 +10,7 @@ refuse_split_block, refuse_split_heading, refuse_split_tree, + tree_hash, unwrap_block_id, without_block_ids, ) @@ -46,6 +47,7 @@ follow_page, follow_page_to_write, handle_connection_error, + on_failure, output, uuid_fields, would_go_before_fields, @@ -70,7 +72,7 @@ insert_tree_at_page_end, move_block_verified, ) -from logseq_cli.writerefused import WriteRefused, partial_state +from logseq_cli.writerefused import BlockNotFound, WriteRefused, partial_state @cli.command("update-block", cls=WriteCommand, owes=lambda params: Owed(("--expect-hash",)), epilog="""\b @@ -259,7 +261,7 @@ def remove_block_cmd(ctx, block_id, ignore_refs, expect_tree_hash, dry_run, as_j click.echo(f" incoming block refs that would dangle: {len(refs)}") return - api.remove_block(clean_id) + api.remove_block(clean_id, expect_tree=tree_hash(block)) if as_json: output({"id": clean_id, "removed": True, "content": content, @@ -931,6 +933,11 @@ def add_block_ref(ctx, source_id, journal_date, page, under_heading, dry_run, as a section goes before the empty blocks that end it, so they stay last, and nothing is overwritten. --expect-hash goes with --remove only: a plain copy changes no block. + --remove removes the source only as it was copied: something added or + changed under it during the copy refuses with block_changed and removes + nothing. A source deleted elsewhere meanwhile is a move done (source_gone). + A failure after the copy's root landed names it (copied_id, "The copy + is ."): the copy stays, and running the command again copies again. """) @click.option("--id", "block_id", required=True, help="Source block UUID") @click.option("--to-page", required=True, help="Target page name") @@ -1016,6 +1023,9 @@ def _copy_tree(block, parent_uuid=None): lambda: [api.append_block_in_page(to_page, content)["uuid"]], keep_last=keep_on, page_name=to_page) new_uuid = root[0] + # Proven from here on: whatever fails next leaves this copy in + # place, and a retry would make another. + on_failure(f"The copy is {new_uuid}.", copied_id=new_uuid) copied = 1 for child in block.get("children", []): copied += _copy_tree(child, new_uuid) @@ -1025,22 +1035,34 @@ def _copy_tree(block, parent_uuid=None): create_missing_page(api, target) count = _copy_tree(source) + source_gone = False if remove: # Only reached when every insert above was proven, so the source is # removed against a copy that is known to exist, never a claimed one. - api.remove_block(block_id) + # And only as it was copied: a child added under it since the read + # would go with it, and the copy does not have it (block_changed). + try: + api.remove_block(block_id, expect_tree=tree_hash(source)) + except BlockNotFound: + # Deleted elsewhere during the copy: copy there, source gone, which + # is the move asked for. A failure would invite cleaning up, and + # that would hit the only version left. + source_gone = True action = "Moved" if remove else "Copied" result_data = {"action": action.lower(), "blocks": count, **names, "source_id": block_id} result_data.update(before_empty_fields(ahead_of.get("uuid"))) if remove: result_data["refs_broken"] = len(refs) + result_data["source_gone"] = source_gone if as_json: output(result_data, True) else: click.echo(f"{action} {count} block(s) to '{to_page}'" f"{BEFORE_EMPTY_SUFFIX if ahead_of.get('uuid') else ''}.") + if source_gone: + click.echo(" The source was already gone.") if refs: click.echo(f" {len(refs)} block ref(s) into the source now point at nothing") diff --git a/logseq_cli/commands/properties.py b/logseq_cli/commands/properties.py index 42b28b8..6ff1f0a 100644 --- a/logseq_cli/commands/properties.py +++ b/logseq_cli/commands/properties.py @@ -2,7 +2,12 @@ import click -from logseq_cli.blocktext import refuse_split_property, unwrap_block_id, with_property_line +from logseq_cli.blocktext import ( + refuse_split_property, + tree_hash, + unwrap_block_id, + with_property_line, +) from logseq_cli.group import cli from logseq_cli.datalog import edn_keyword, edn_string from logseq_cli.blockprops import ( @@ -215,7 +220,7 @@ def _write_property_block(api, blocks, block, content): else: uuid, old = block["uuid"], block.get("content") or "" if not content and block is not None and not block.get("children") and len(blocks) > 1: - api.remove_block(uuid) + api.remove_block(uuid, expect_tree=tree_hash(block)) return if content == old and old: api.update_block(uuid, "", replacing=old) diff --git a/logseq_cli/output.py b/logseq_cli/output.py index 7d02ec5..6513248 100644 --- a/logseq_cli/output.py +++ b/logseq_cli/output.py @@ -21,6 +21,26 @@ from logseq_cli.writerefused import WriteRefused, partial_state +def on_failure(sentence: str, **fields) -> None: + """Say ``sentence`` and add ``fields`` to every failure of the rest of + the call, refused or cut off: what only the command knows about what its + landed writes left behind (copy-block: where the copy is). One call; a + later one replaces it. Both ways a failure is reported read it, so the + command needs no ``try`` of its own per kind of failure.""" + ctx = click.get_current_context(silent=True) + if ctx is not None and isinstance(ctx.obj, dict): + ctx.obj["on_failure"] = (sentence, fields) + + +def _with_on_failure(message: str, fields: dict) -> tuple: + """``message`` and ``fields`` with what the command left with + :func:`on_failure` added, the sentence last.""" + ctx = click.get_current_context(silent=True) + obj = ctx.obj if ctx is not None and isinstance(ctx.obj, dict) else {} + sentence, own = obj.get("on_failure") or ("", {}) + return (f"{message} {sentence}" if sentence else message), {**fields, **own} + + def _after_writes(message: str) -> tuple: """``message`` and the fields for a failure of the connection, which can come after writes of the same call landed, and between a write and its @@ -43,7 +63,7 @@ def _after_writes(message: str) -> tuple: message = f"{message} No earlier write of this call landed." if landed or unproven: fields["writes_landed"] = landed - return message, fields + return _with_on_failure(message, fields) def handle_connection_error(func): @@ -151,13 +171,9 @@ def wrapper(*args, **kwargs): # with its source open) must say so, and one raised outside the # central write says it too. Exit 1 for all of them. landed = click.get_current_context().obj["api"].writes_landed - fail( - f"{e} {partial_state(landed)}", - as_json=as_json, - reason=e.reason, - writes_landed=landed, - **e.fields, - ) + message, fields = _with_on_failure(f"{e} {partial_state(landed)}", + {**e.fields, "writes_landed": landed}) + fail(message, as_json=as_json, reason=e.reason, **fields) except IdLineError as e: # Refused before the write, like a line that splits the block: the # text would give the block another uuid (#56). diff --git a/logseq_cli/strictinsert.py b/logseq_cli/strictinsert.py index 5ee95f8..21a7c52 100644 --- a/logseq_cli/strictinsert.py +++ b/logseq_cli/strictinsert.py @@ -417,7 +417,7 @@ def insert_tree_keeping_ids(api, tree: list, where: str, target: str) -> list: raise if stand_in: # The batch landed: a stand-in left behind is an error of its own. - api.remove_block(stand_in, written_here=True) + api.remove_block(stand_in, expect_tree=None, written_here=True) expected = count_blocks(tree) after_tree = api.get_page_blocks_tree(page_name) or [] @@ -467,7 +467,7 @@ def _remove_stand_in(api, stand_in: str, failed: Exception) -> None: which would hide why the batch failed. Any WriteRefused: the removal can meet an error object, an open editor, or a removal that did not show.""" try: - api.remove_block(stand_in, written_here=True) + api.remove_block(stand_in, expect_tree=None, written_here=True) except WriteRefused as refused: note = (f"The empty stand-in block {stand_in[:8]}... written for the batch " f"stayed too: {refused}") diff --git a/logseq_cli/writerefused.py b/logseq_cli/writerefused.py index a62b2fd..35a0b81 100644 --- a/logseq_cli/writerefused.py +++ b/logseq_cli/writerefused.py @@ -39,14 +39,16 @@ class EditorOpen(WriteRefused): class BlockChanged(WriteRefused): - """The block no longer holds the text the caller read and meant to replace. + """The block no longer holds the text the caller read and meant to replace, + or, before a removal, it or a block under it changed since it was read. Field: block.""" reason = "block_changed" class BlockNotFound(WriteRefused): - """The block the caller meant is gone, found out before anything was - written. Field: id (or content and page, for a block picked by its text).""" + """The block the caller meant is gone, found out before the write it + refuses (a call may have written other blocks before, as copy-block does). + Field: id (or content and page, for a block picked by its text).""" reason = "block_not_found" diff --git a/skills/logseq-cli/SKILL.md b/skills/logseq-cli/SKILL.md index ec78425..e8dffa3 100644 --- a/skills/logseq-cli/SKILL.md +++ b/skills/logseq-cli/SKILL.md @@ -65,7 +65,7 @@ for example the token from Logseq's API settings, passed as `LOGSEQ_TOKEN`. how many. Ask the user to leave the block, then retry once (not in a loop). - `write_not_verified`: read the block before retrying. Someone writing there at the same time can make a write that landed look missing. - - `block_changed`, `precondition_failed`: the block changed after it was read; read it again and decide again. `precondition_required`: pass `--expect-hash` (from `get-block --json`) or `--expect-marker`; `remove-block` wants `--expect-tree-hash` (its `tree_hash`). `precondition_failed` is the caller's check (`block_changed` is the command's own re-read): never repeat the same change with a fresh hash. The hash sees content, not position; a write's output has no new hash, so read again before a second write to the block. Pass `--expect-hash` whenever you change a block you read earlier, switch or not. + - `block_changed`, `precondition_failed`: the block changed after it was read; read it again and decide again. `precondition_required`: pass `--expect-hash` (from `get-block --json`) or `--expect-marker`; `remove-block` wants `--expect-tree-hash` (its `tree_hash`). `precondition_failed` is the caller's check (`block_changed` is the command's own re-read): never repeat the same change with a fresh hash. The hash sees content, not position; a write's output has no new hash, so read again before a second write to the block. Pass `--expect-hash` whenever you change a block you read earlier, switch or not. After `copy-block` a failure may name `copied_id`: that copy stays, and repeating the command makes a second one. - `rename_refused`, `page_exists`: nothing was written. - `read_only`: writes are off on purpose. Do not retry or go around it; tell the user. diff --git a/tests/conftest.py b/tests/conftest.py index 4947877..25ff877 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -700,7 +700,7 @@ def append_block_in_page(self, name, content, options=None): page["blocks"].append(block) return self._out(block, page, None) - def remove_block(self, uuid, *, written_here=False): + def remove_block(self, uuid, *, expect_tree, written_here=False): # Single writes are not counted here (only batches are), so # ``written_here`` has nothing to take back. found = self.locate(uuid) diff --git a/tests/logseq_http_double.py b/tests/logseq_http_double.py index ec77d8b..d784236 100644 --- a/tests/logseq_http_double.py +++ b/tests/logseq_http_double.py @@ -393,6 +393,26 @@ def uuid_of(self, content: str) -> str: raise LookupError(f"{len(found)} blocks hold {content!r}") return found[0] + # --- changes made in Logseq meanwhile ------------------------------------- + def add_child(self, parent, content="added later"): + """Someone adds a child under ``parent`` in Logseq, last among its + children; returns its uuid.""" + _, siblings, i, _ = self._locate(parent) + node = self._build(content) + siblings[i]["children"].append(node) + self._index(node) + return node["uuid"] + + def set_content(self, uuid, content): + """Someone edits the block's text in Logseq.""" + _, siblings, i, _ = self._locate(uuid) + siblings[i]["content"] = content + + def delete_block(self, uuid): + """Someone deletes the block, with its children, in Logseq.""" + _, siblings, i, _ = self._locate(uuid) + del siblings[i] + def tree(self, name): """The page as first lines, nested: for assertions.""" def walk(nodes): diff --git a/tests/test_copy_block_remove_changed_source.py b/tests/test_copy_block_remove_changed_source.py new file mode 100644 index 0000000..c724694 --- /dev/null +++ b/tests/test_copy_block_remove_changed_source.py @@ -0,0 +1,134 @@ +"""copy-block names its copy whenever it fails after the copy's root landed. + +The copy is written block by block and the source removed last, with no +rollback: a failure after the root landed leaves a copy in the target that a +retry would copy again. The error object names it (``copied_id``) and the +message says "The copy is .", so a caller checks it instead of +repeating the command. +""" +import json + +import pytest +import requests + +from logseq_cli.cli import cli +from tests.conftest import split_runner +from tests.logseq_http_double import LogseqHttpDouble + +SOURCE = {"content": "source block", "children": ["first kid", "second kid", "third kid"]} + + +@pytest.fixture +def double(monkeypatch): + return LogseqHttpDouble.installed( + monkeypatch, {"Source Page": [SOURCE], "Target Page": ["target block"]}) + + +def _move(double): + uuid = double.uuid_of("source block") + return split_runner().invoke(cli, ["--token", "t", "copy-block", "--id", uuid, + "--to-page", "Target Page", "--remove", "--json"]) + + +def _copy_root(double): + """The uuid of the copy's root: the last top-level block of the target.""" + return double._require_page("Target Page")["blocks"][-1]["uuid"] + + +def test_a_child_copy_not_proven_names_the_copy(double): + double.set_mode("insertBlock", "noop", from_call=2) + r = _move(double) + assert r.exit_code == 1 + error = json.loads(r.stderr) + assert error["reason"] == "write_not_verified" + assert error["copied_id"] == _copy_root(double) + assert f"The copy is {_copy_root(double)}." in error["error"] + assert double.tree("Target Page")[-1] == ("source block", [("first kid", [])]) + assert double.sent("removeBlock") == [] + + +def test_a_timeout_at_the_removal_names_the_copy(double): + def timeout(args): + raise requests.Timeout("removeBlock timed out (test)") + double._handlers["logseq.Editor.removeBlock"] = timeout + r = _move(double) + assert r.exit_code == 1 + error = json.loads(r.stderr) + assert error["reason"] == "timeout" + assert error["copied_id"] == _copy_root(double) + assert f"The copy is {_copy_root(double)}." in error["error"] + + +def test_a_root_that_did_not_land_names_no_copy(double): + double.set_mode("appendBlockInPage", "noop") + r = _move(double) + assert r.exit_code == 1 + error = json.loads(r.stderr) + assert error["reason"] == "write_not_verified" + assert "copied_id" not in error + assert "The copy is" not in error["error"] + + +# --- The source is removed only as it was copied ---------------------------- + +def _after_child_insert(double, n, then): + """Run ``then`` once the copy's ``n``-th child insert has landed. The + root goes through appendBlockInPage, the children through insertBlock.""" + insert = double._handlers["logseq.Editor.insertBlock"] + calls = [] + + def hooked(args): + answer = insert(args) + calls.append(1) + if len(calls) == n: + then() + return answer + double._handlers["logseq.Editor.insertBlock"] = hooked + + +def test_a_child_added_during_the_copy_is_not_removed(double): + source = double.uuid_of("source block") + _after_child_insert(double, 2, lambda: double.add_child(source, "added during the copy")) + r = _move(double) + assert r.exit_code == 1 + error = json.loads(r.stderr) + assert error["reason"] == "block_changed" + assert error["copied_id"] == _copy_root(double) + assert "repeat" not in error["error"].lower() + assert f"The copy is {_copy_root(double)}." in error["error"] + assert double.sent("removeBlock") == [] + assert double.tree("Source Page") == [("source block", [ + ("first kid", []), ("second kid", []), ("third kid", []), + ("added during the copy", [])])] + assert double.tree("Target Page")[-1] == ("source block", [ + ("first kid", []), ("second kid", []), ("third kid", [])]) + + +def test_a_source_deleted_during_the_copy_is_a_move_done(double): + source = double.uuid_of("source block") + _after_child_insert(double, 1, lambda: double.delete_block(source)) + r = _move(double) + assert r.exit_code == 0, r.stderr + result = json.loads(r.stdout) + assert result["action"] == "moved" + assert result["source_gone"] is True + assert double.sent("removeBlock") == [] + assert double.tree("Target Page")[-1] == ("source block", [ + ("first kid", []), ("second kid", []), ("third kid", [])]) + + +def test_a_source_deleted_during_the_copy_says_so_in_text(double): + source = double.uuid_of("source block") + _after_child_insert(double, 1, lambda: double.delete_block(source)) + r = split_runner().invoke(cli, ["--token", "t", "copy-block", "--id", source, + "--to-page", "Target Page", "--remove"]) + assert r.exit_code == 0, r.stderr + assert "The source was already gone." in r.stdout + + +def test_an_unchanged_source_is_moved_as_before(double): + r = _move(double) + assert r.exit_code == 0, r.stderr + result = json.loads(r.stdout) + assert (result["action"], result["blocks"], result["source_gone"]) == ("moved", 4, False) + assert double.tree("Source Page") == [] diff --git a/tests/test_destructive_dry_run.py b/tests/test_destructive_dry_run.py index f0a66cc..c7a0af7 100644 --- a/tests/test_destructive_dry_run.py +++ b/tests/test_destructive_dry_run.py @@ -10,6 +10,7 @@ import pytest from click.testing import CliRunner +from logseq_cli.blocktext import tree_hash from logseq_cli.cli import cli from tests.conftest import answer_property_pulls, split_runner @@ -97,7 +98,8 @@ def test_without_dry_run_removes(self, api): api.get_block.return_value = {"uuid": "root", "content": "Parent"} result = CliRunner().invoke(cli, ["remove-block", "--id", "root"]) assert result.exit_code == 0 - api.remove_block.assert_called_once_with("root") + api.remove_block.assert_called_once_with( + "root", expect_tree=tree_hash({"uuid": "root", "content": "Parent"})) class TestCopyBlockDryRun: diff --git a/tests/test_editor_gate.py b/tests/test_editor_gate.py index ae5ec61..6649be1 100644 --- a/tests/test_editor_gate.py +++ b/tests/test_editor_gate.py @@ -57,12 +57,8 @@ def cached_api(double): def _open_new_child(double, parent, content="typed here"): """Someone presses Enter under ``parent`` and types in the new child; returns its uuid.""" - _, siblings, i, _ = double._locate(parent) - node = double._build(content) - siblings[i]["children"].append(node) - double._index(node) - double.editing = node["uuid"] - return node["uuid"] + double.editing = double.add_child(parent, content) + return double.editing def _uuid(double, content): @@ -177,12 +173,12 @@ def test_gate_refuses_set_blocks_id(double, api): def test_gate_refuses_remove_block_target(double, api): x = _uuid(double, "target block") - _assert_refused(double, lambda: api.remove_block(x), x) + _assert_refused(double, lambda: api.remove_block(x, expect_tree=None), x) def test_gate_refuses_remove_block_subtree(double, api): parent, kid = _uuid(double, "parent block"), _uuid(double, "kid block") - _assert_refused(double, lambda: api.remove_block(parent), kid) + _assert_refused(double, lambda: api.remove_block(parent, expect_tree=None), kid) # The subtree and the open block are read when the gate asks, not taken from @@ -195,7 +191,7 @@ def test_gate_remove_block_sees_a_child_made_after_the_first_read(double, cached kid = _open_new_child(double, parent) before = double.snapshot() with pytest.raises(EditorOpen) as refused: - cached_api.remove_block(parent) + cached_api.remove_block(parent, expect_tree=None) assert refused.value.fields["block"] == kid assert double.writes() == [] and double.snapshot() == before @@ -225,7 +221,8 @@ def test_gate_rename_page_sees_a_link_saved_after_the_first_read(double, cached_ def test_remove_block_command_sees_a_child_made_during_the_call(double): # Through the command, with its cache: the child appears while the call - # looks for refs into the block, after it read the block's subtree. + # looks for refs into the block, after it read the block's subtree. The + # read just before the delete sees it first, before the editor gate. parent = _uuid(double, "parent block") query = double._handlers["logseq.DB.datascriptQuery"] @@ -235,6 +232,23 @@ def child_appears(args): double._handlers["logseq.DB.datascriptQuery"] = child_appears r = split_runner().invoke(cli, ["--token", "t", "remove-block", "--id", parent, "--json"]) assert r.exit_code == 1 + assert _error_object(r)["reason"] == "block_changed" + assert double.sent("removeBlock") == [] + + +def test_remove_block_command_sees_a_child_typed_after_the_last_read(double): + # The child appears only when the gate asks, after the read before the + # delete: the editor gate is what refuses it. + parent = _uuid(double, "parent block") + check = double._check_editing + + def child_typed(): + if double.editing is None: + _open_new_child(double, parent) + return check() + double._check_editing = child_typed + r = split_runner().invoke(cli, ["--token", "t", "remove-block", "--id", parent, "--json"]) + assert r.exit_code == 1 assert _error_object(r)["reason"] == "open_in_editor" assert double.sent("removeBlock") == [] @@ -285,7 +299,7 @@ def test_gate_lets_unrelated_writes_through(double, api, write): _uuid(double, "body\nprio:: 1"), "prio"), "move_block_next_to_open": lambda: api.move_block(x, anchor, {"children": True}), "set_blocks_id": lambda: api.set_blocks_id([y]), - "remove_block": lambda: api.remove_block(x), + "remove_block": lambda: api.remove_block(x, expect_tree=None), "delete_page": lambda: api.delete_page("Props Page"), "rename_page": lambda: api.rename_page("Props Page", "Renamed Props"), } @@ -438,6 +452,9 @@ def test_copy_block_remove_open_source_reports_landed_copies(double): assert error["block"] == parent assert error["writes_landed"] == 2 assert "2 earlier write(s) in this call landed and remain" in error["error"] + copy = next(p for p in double.pages if p["name"] == "Other Page")["blocks"][-1]["uuid"] + assert error["copied_id"] == copy + assert f"The copy is {copy}." in error["error"] assert double.sent("removeBlock") == [] assert double.tree("Other Page")[-1] == ("parent block", [("kid block", [])]) assert ("parent block", [("kid block", [])]) in double.tree("Probe Page") diff --git a/tests/test_incoming_refs.py b/tests/test_incoming_refs.py index 9c191c6..6b8fb0a 100644 --- a/tests/test_incoming_refs.py +++ b/tests/test_incoming_refs.py @@ -16,6 +16,7 @@ from click.testing import CliRunner +from logseq_cli.blocktext import tree_hash from logseq_cli.cli import cli from logseq_cli.lookup import incoming_block_refs @@ -131,6 +132,11 @@ def test_a_page_entity_as_source_is_its_own_page(self): {"target": CHILD, "block": REF_OUT, "page": "Notes"}] +def _tree_as_read(api): + """The tree hash of the root as the command read it.""" + return tree_hash(api.get_block(ROOT, include_children=True)) + + class TestRemoveBlock: def test_refuses_with_incoming_refs(self): api = _api(_graph()) @@ -144,14 +150,14 @@ def test_ignore_refs_removes_and_says_what_broke(self): api = _api(_graph()) r = _run(api, "remove-block", "--id", ROOT, "--ignore-refs") assert r.exit_code == 0, r.output - api.remove_block.assert_called_once_with(ROOT) + api.remove_block.assert_called_once_with(ROOT, expect_tree=_tree_as_read(api)) assert "2 incoming block ref(s)" in r.output def test_no_refs_removes_as_before(self): api = _api(_graph(refs_from_outside=False, refs_from_inside=False)) r = _run(api, "remove-block", "--id", ROOT) assert r.exit_code == 0, r.output - api.remove_block.assert_called_once_with(ROOT) + api.remove_block.assert_called_once_with(ROOT, expect_tree=_tree_as_read(api)) def test_dry_run_predicts_the_refusal(self): api = _api(_graph()) @@ -233,5 +239,5 @@ def test_ignore_refs_moves(self): r = _run(api, "copy-block", "--id", ROOT, "--to-page", "Elsewhere", "--remove", "--ignore-refs", "--json") assert r.exit_code == 0, r.output - api.remove_block.assert_called_once_with(ROOT) + api.remove_block.assert_called_once_with(ROOT, expect_tree=_tree_as_read(api)) assert json.loads(r.output)["refs_broken"] == 3 diff --git a/tests/test_move_block.py b/tests/test_move_block.py index 6f1823b..bc0a8e4 100644 --- a/tests/test_move_block.py +++ b/tests/test_move_block.py @@ -20,6 +20,7 @@ import pytest from click.testing import CliRunner +from logseq_cli.blocktext import tree_hash from logseq_cli.cli import cli from tests.conftest import split_runner from tests.logseq_http_double import LogseqHttpDouble @@ -154,7 +155,8 @@ def test_successful_copy_still_removes(self): r = CliRunner().invoke(cli, [ "copy-block", "--id", SRC, "--to-page", "Target", "--remove"]) assert r.exit_code == 0, r.output - api.remove_block.assert_called_once_with(SRC) + api.remove_block.assert_called_once_with( + SRC, expect_tree=tree_hash({"uuid": SRC, "content": "Head", "children": []})) class _Graph: diff --git a/tests/test_remove_block_expect_tree.py b/tests/test_remove_block_expect_tree.py new file mode 100644 index 0000000..ccd1b3c --- /dev/null +++ b/tests/test_remove_block_expect_tree.py @@ -0,0 +1,149 @@ +"""A removal deletes only the subtree the caller read. + +``removeBlock`` cascades: whatever stands under the block when it runs goes +with it. A caller that read the block, counted and checked it, and then +removes it, passes the tree hash of that read; ``LogseqAPI.remove_block`` +reads the block again just before the delete and refuses when the subtree is +another one. The window that stays is the requests between that read and the +delete: the API has no compare-and-swap. +""" +import json + +import pytest + +from logseq_cli.api import LogseqAPI +from logseq_cli.blocktext import tree_hash +from logseq_cli.cli import cli +from logseq_cli.writerefused import BlockChanged, BlockNotFound +from tests.conftest import split_runner +from tests.logseq_http_double import LogseqHttpDouble + + +@pytest.fixture +def double(monkeypatch): + double = LogseqHttpDouble().install(monkeypatch) + double.add_page("Probe Page", [ + {"content": "parent block", "children": ["kid block"]}, + "other block", + ]) + return double + + +@pytest.fixture +def api(double): + return LogseqAPI(token="t") + + +def _read_tree(api, uuid): + return tree_hash(api.get_block(uuid, include_children=True, cached=False)) + + +# --- The API ---------------------------------------------------------------- + +def test_a_changed_subtree_is_not_removed(double, api): + parent = double.uuid_of("parent block") + read = _read_tree(api, parent) + double.add_child(parent) + with pytest.raises(BlockChanged) as refused: + api.remove_block(parent, expect_tree=read) + assert refused.value.fields["block"] == parent + assert "nothing was removed" in str(refused.value) + assert "repeat" not in str(refused.value).lower() + assert double.sent("removeBlock") == [] + + +def test_a_changed_root_text_is_not_removed(double, api): + parent = double.uuid_of("parent block") + read = _read_tree(api, parent) + double.set_content(parent, "parent edited") + with pytest.raises(BlockChanged): + api.remove_block(parent, expect_tree=read) + assert double.sent("removeBlock") == [] + + +def test_a_block_gone_before_the_delete_is_reported(double, api): + parent = double.uuid_of("parent block") + read = _read_tree(api, parent) + double.delete_block(parent) + with pytest.raises(BlockNotFound) as refused: + api.remove_block(parent, expect_tree=read) + assert refused.value.fields["id"] == parent + assert double.sent("removeBlock") == [] + + +def test_the_subtree_as_read_is_removed(double, api): + parent = double.uuid_of("parent block") + api.remove_block(parent, expect_tree=_read_tree(api, parent)) + assert len(double.sent("removeBlock")) == 1 + assert double.tree("Probe Page") == [("other block", [])] + + +def test_without_a_state_the_removal_is_as_before(double, api): + parent = double.uuid_of("parent block") + double.add_child(parent) + api.remove_block(parent, expect_tree=None) + assert double.tree("Probe Page") == [("other block", [])] + + +def test_every_caller_says_which_state_it_removes(double, api): + with pytest.raises(TypeError): + api.remove_block(double.uuid_of("parent block")) + + +# --- remove-block ----------------------------------------------------------- + +def test_remove_block_keeps_a_child_made_between_read_and_delete(double): + parent = double.uuid_of("parent block") + query = double._handlers["logseq.DB.datascriptQuery"] + added = [] + + def child_appears(args): + # The command looks for refs into the block after it read the subtree. + if not added: + added.append(double.add_child(parent)) + return query(args) + double._handlers["logseq.DB.datascriptQuery"] = child_appears + r = split_runner().invoke(cli, ["--token", "t", "remove-block", "--id", parent, "--json"]) + assert r.exit_code == 1, r.stderr + error = json.loads(r.stderr) + assert error["reason"] == "block_changed" + assert double.sent("removeBlock") == [] + assert double._locate(added[0]) is not None + + +def test_remove_block_reports_a_block_gone_before_the_delete(double): + parent = double.uuid_of("parent block") + query = double._handlers["logseq.DB.datascriptQuery"] + + def block_goes(args): + if double._locate(parent): + double.delete_block(parent) + return query(args) + double._handlers["logseq.DB.datascriptQuery"] = block_goes + r = split_runner().invoke(cli, ["--token", "t", "remove-block", "--id", parent, "--json"]) + assert r.exit_code == 1, r.stderr + assert json.loads(r.stderr)["reason"] == "block_not_found" + assert double.sent("removeBlock") == [] + + +# --- The page's property block ---------------------------------------------- + +def test_remove_property_keeps_a_property_block_that_got_a_child(monkeypatch): + double = LogseqHttpDouble().install(monkeypatch) + double.add_page("Props Page", ["kind:: a", "text"]) + block = double.uuid_of("kind:: a") + tree = double._handlers["logseq.Editor.getPageBlocksTree"] + added = [] + + def child_appears(args): + answer = tree(args) + if not added: + added.append(double.add_child(block)) + return answer + double._handlers["logseq.Editor.getPageBlocksTree"] = child_appears + r = split_runner().invoke(cli, ["--token", "t", "remove-property", "--name", "Props Page", + "--key", "kind", "--json"]) + assert r.exit_code == 1, r.stderr + assert json.loads(r.stderr)["reason"] == "block_changed" + assert double.sent("removeBlock") == [] + assert double._locate(block) is not None and double._locate(added[0]) is not None diff --git a/tests/test_write_proof_every_command.py b/tests/test_write_proof_every_command.py index 6c2243f..45ef5e8 100644 --- a/tests/test_write_proof_every_command.py +++ b/tests/test_write_proof_every_command.py @@ -476,7 +476,7 @@ def test_set_property_the_page_does_not_show_is_not_verified(monkeypatch): # --- removeBlock, deletePage, renamePage, createPage, setBlocksId ----------- @pytest.mark.parametrize("write,landed", [ - (lambda api, d: api.remove_block(d.uuid_of("alpha block")), 1), + (lambda api, d: api.remove_block(d.uuid_of("alpha block"), expect_tree=None), 1), (lambda api, d: api.delete_page("Other Page"), 1), (lambda api, d: api.rename_page("Other Page", "Renamed Page"), 1), (lambda api, d: api.create_page("Fresh Page"), 1),