Assorted 'ipfs object patch' cleanups #1407

wking · 2015-06-21T05:40:33Z

Lots of detail and motivation in the commit messages themselves.
Major changes:

New replace-link action.
Both add-link and replace-link auto-create nodes for any missing
intermediate paths (more on this below).
rm-link now removes all links matching a name (not just the first).
Docs and tests for all the actions :).

I expect most of this to be fairly non-controversial (although I've
been wrong about that sort of thing before ;). The main UI thing we
need to work out is auto-creating intermediate nodes. Talking about
this on IRC, @whyrusleeping expressed concern about auto-creation as
part of the object-patch command (see 1 through 2). Options I
see:

a. Keep the auto-creation as it's currently implemented in this PR
(i.e. just do it). Folks who typo paths may end up accidentally
creating a few DAG entries, but that's not a big deal (we don't
even pin the stuff made by object patch as far as I can tell).
Folks who want a different intermediate node can either patch the
auto-created node, or build their indentded intermediate node first
and then run the bubbling patch through that node (or any of its
ancestors).

b. Add a flag enabling auto-creation (like mkdir's -p /
--parents). This lets folks like me (who don't want them)
opt-out of typo'ed path checks, but keeps the checks in place for
users who haven't thought about what they want (and who may
therefore expect errors in the missing-intermediate case). This
approach almost ties with (a) for me, since -p isn't that hard to
type (and I could always alias it), with the main drawback being an
extra knob that I don't really think we need. This option still
doesn't give the control @whyrusleeping apparently desires over the
auto-created objects themselves.

c. Add a flag (-t / --template?) that accepts all the strings
understood by ipfs object new. Then we use the same logic
internally when auto-creating intermediates. This gives some
control over the auto-created objects, which may satisfy
@whyrusleeping. On the other hand, the only template there is
currently unixfs-new, and for creating intermediates that are
Unix-FS directories I think we should really be looking at an ipfs file patch … command that auto-creates directories by default. It
would also handle things like transparent sharding, chunking, and
shard/chunk-transparent Path resolution (e.g. an a/b/c path would
work even if the Merkle representation was:

    {a-shard-root}→{a-shard}→{b-shard-root}→{b-shard}→{c-chunk-root}→[{c-chunks}…]

d. Create some new command that handles the auto-creation in a
separate step. But then folks like me that can (with this PR) run:

    $ ipfs object patch $ROOT replace-link a/b/c $CONTENT

would need to run something like:

    $ if ipfs object ls $ROOT/a/b >/dev/null 2>&1
    > then  # parent already exists
    >   ipfs object patch $ROOT replace-link a/b/c $CONTENT
    > else
    >   ipfs object mkdir -p $ROOT/a/b &&
    >   ipfs object patch $ROOT add-link a/b/c $CONTENT
    > fi

and that seems like useless hoop-jumping.

Pull the independent multi-layer test added in d585e20 (allow patch add-link to add at a path, 2015-06-19, #1404) out into a separate block, so it's easier to read the single-layer tests that share the OUTPUT variable. License: MIT Signed-off-by: W. Trevor King <wking@tremily.us>

Because: ipfs object patch $ROOT add-link a/b/c $FILE is a lot easier than: EMPTY=$(ipfs object new unixfs-dir) && A=$(ipfs object patch $EMPTY add-link b $EMPTY) && R=$(ipfs object patch $ROOT add-link a $A) && ipfs object patch $R add-link a/b/c $FILE and the long form isn't even checking to see if the original $ROOT has descendents a or a/b. Note that these are just Merkle nodes, not Unix-FS directories, because 'ipfs object ...' is a Merkle-level tool. I'd like to make this flexible enough that we could have a Unix-FS-level 'ipfs file patch ...' with similar semantics except that it operates on Unix-FS-level nodes (e.g. it creates intermediate *directories*, turns directories into files if you use 'set-data', manages the '*-data' commands without clobbering the type information unixfs stores in Data, etc.). But I don't want that Unix-FS abstraction stuff sneaking into the Merkle-level command we're working on here. Now that insertNodeAtPath takes a *dag.Node to insert instead of a key.Key, and it can both add and remove (if toinsert is nil) links, there wasn't much need for the addLink helper. This commit just inlines (and extends to removal) that functionality in insertNodeAtPath. The '$(<file)' constructs differ from this file's previous '$(cat file)' usage, but it's always good to remove useless cats ;). I adjusted the existing '$(cat file)' call while I was at it. License: MIT Signed-off-by: W. Trevor King <wking@tremily.us>

And now that we have several multi-layer patch tests, disambiguate the name of the add-without-autocreation test. License: MIT Signed-off-by: W. Trevor King <wking@tremily.us>

Also fix a copy/paste error in an appendDataCaller error messages so it references append-data instead of set-data. License: MIT Signed-off-by: W. Trevor King <wking@tremily.us>

The "raw bytes as a command-line argument" UI makes it difficult to do some things. For example, I wasn't able to figure out a way to use $ ipfs object patch $EMPTY set-data "$(<set_data_expected)" to add data from a file that had a trailing newline (which is why I ended up using printf to write a file without a trailing newline). License: MIT Signed-off-by: W. Trevor King <wking@tremily.us>

Setting the append-to-existing test up to avoid duplicating the expected content was too much trouble, so I'm just hard-coding the partial strings in the append-data calls. License: MIT Signed-off-by: W. Trevor King <wking@tremily.us>

The functionality contained in hear (managing links and data, bubbling changes) is useful stuff that folks will likely want to call directly from other Go code (e.g. I'm about to combine rm-link and add-link to produce a replace-link action). It's easier to do that when the handlers stick to more primitive IPFS objects (e.g. DAGServices and Merkle nodes) than it would be if they used higher level stuff (e.g. Request objects, arrays of argument strigns). License: MIT Signed-off-by: W. Trevor King <wking@tremily.us>

The functionality contained in hear (managing links and data, bubbling changes) is useful stuff that folks will likely want to call directly from other Go code (e.g. I'm about to combine rm-link and add-link to produce a replace-link action). It's easier to do that when the handlers stick to more primitive IPFS objects (e.g. dag.Nodes) than it would be if they used higher level stuff (e.g. key.Keys). Now that it would have been operating at a more fundamental level, I removed the rmLinkCaller helper entirely and call insertNodeAtPath directly to handle that case. This commit also shifts some common code outside of the switch statement to make the switch cases easier to read and avoid duplicating the same procedure within each case. License: MIT Signed-off-by: W. Trevor King <wking@tremily.us>

I rarely want to add duplicate links with the same name. This new action ensures that the only link at the specified path is the one you just added, regardless of whether or not there was already a link at that path. License: MIT Signed-off-by: W. Trevor King <wking@tremily.us>

Instead of stopping after the first one. This catches the implementation up to the rm-link explanation in the ShortDescription. License: MIT Signed-off-by: W. Trevor King <wking@tremily.us>

jbenet · 2015-06-23T05:09:42Z

@whyrusleeping more thoughts on this? im not entirely sure what the implications of creating intermediate nodes is-- ware you worried about types?

whyrusleeping · 2015-06-23T15:45:52Z

I'm worried about a few different things that @wking mentions above, first, i dont want to accidentally mistype a long path under a node, and have it create a bunch of un-asked-for nodes (although, the only cost there is having to run a GC). The other worry i had was what should those intermediate nodes be? blank nodes? or unixfs dirs? You can pass that as an option, but then the command invocations start to get really messy, and it brings the UX down a bit in my opinion.

jbenet · 2015-06-25T21:20:59Z

first, i dont want to accidentally mistype a long path under a node, and have it create a bunch of un-asked-for nodes (although, the only cost there is having to run a GC).

i think accidentally creating additional objects is likely ok.

The other worry i had was what should those intermediate nodes be? blank nodes? or unixfs dirs? You can pass that as an option, but then the command invocations start to get really messy, and it brings the UX down a bit in my opinion.

yeah this is a harder question. not sure, could pass a template as an option and default to a blank node? not sure.

wking · 2015-06-25T23:12:20Z

On Thu, Jun 25, 2015 at 02:20:59PM -0700, Juan Batiz-Benet wrote:

The other worry i had was what should those intermediate nodes be?
blank nodes? or unixfs dirs? You can pass that as an option, but
then the command invocations start to get really messy, and it
brings the UX down a bit in my opinion.

yeah this is a harder question. not sure, could pass a template as
an option and default to a blank node? not sure.

I still think (see the tail end of (c) in my initial comment) that the
best choice here is to create blank nodes in ‘ipfs object patch …’
(like this PR does now), and then add additional commands for other
situations (e.g. ‘ipfs file patch …’ that hides sharding/fanout in
path handling and auto-creates Unix-FS directories).

jbenet · 2015-06-27T10:04:11Z

New replace-link action.

what are the semantics of replace when many links with the same name exist? remove all and add just one?

rm-link now removes all links matching a name (not just the first).

maybe should be named rm-links. another option is to use a flag rm-link --all. this brings up something else, links without names need to be able to be removed, maybe with their int offset.

(in general i would like to be able to address links by their number, but this is currently not kosher with names without introducing a special prefix)

Docs and tests for all the actions :).

👍 👍

auto-creation

i gave it some thought and i think i would want auto-creation, but definitely optionally, and being able to specify a template. So i think we want both -p and -t (with -t defaulting to a blank node (no data))

jbenet · 2015-08-02T12:58:13Z

is this still needed?

whyrusleeping · 2015-10-18T07:36:37Z

closing, old PR cleanup time.

wking added 10 commits June 20, 2015 11:05

core/commands/object: Add bubbling to rm-link

56953b2

And now that we have several multi-layer patch tests, disambiguate the name of the add-without-autocreation test. License: MIT Signed-off-by: W. Trevor King <wking@tremily.us>

core/commands/object: List all actions and their arguments for 'patch'

8198804

Also fix a copy/paste error in an appendDataCaller error messages so it references append-data instead of set-data. License: MIT Signed-off-by: W. Trevor King <wking@tremily.us>

t0051: Add patch append-data tests

ebc9a39

Setting the append-to-existing test up to avoid duplicating the expected content was too much trouble, so I'm just hard-coding the partial strings in the append-data calls. License: MIT Signed-off-by: W. Trevor King <wking@tremily.us>

core/commands/object: Adjust patch rm-links to remove all matching links

ef1e8ad

Instead of stopping after the first one. This catches the implementation up to the rm-link explanation in the ShortDescription. License: MIT Signed-off-by: W. Trevor King <wking@tremily.us>

jbenet added the status/in-progress In progress label Jun 21, 2015

whyrusleeping mentioned this pull request Jun 23, 2015

Sprint M ipfs/team-mgmt#17

Closed

34 tasks

whyrusleeping mentioned this pull request Jul 1, 2015

Sprint N ipfs/team-mgmt#20

Closed

44 tasks

whyrusleeping changed the title ~~Assorted 'ipfs object patch' cleanups~~ Assorted 'ipfs object patch' cleanupstime Jul 2, 2015

whyrusleeping changed the title ~~Assorted 'ipfs object patch' cleanupstime~~ Assorted 'ipfs object patch' cleanups Jul 2, 2015

whyrusleeping closed this Oct 18, 2015

jbenet removed the status/in-progress In progress label Oct 18, 2015

Kubuxu deleted the tk/bubble branch February 27, 2017 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Assorted 'ipfs object patch' cleanups #1407

Assorted 'ipfs object patch' cleanups #1407

wking commented Jun 21, 2015

jbenet commented Jun 23, 2015

whyrusleeping commented Jun 23, 2015

jbenet commented Jun 25, 2015

wking commented Jun 25, 2015

jbenet commented Jun 27, 2015

jbenet commented Aug 2, 2015

whyrusleeping commented Oct 18, 2015

Assorted 'ipfs object patch' cleanups #1407

Assorted 'ipfs object patch' cleanups #1407

Conversation

wking commented Jun 21, 2015

jbenet commented Jun 23, 2015

whyrusleeping commented Jun 23, 2015

jbenet commented Jun 25, 2015

wking commented Jun 25, 2015

jbenet commented Jun 27, 2015

jbenet commented Aug 2, 2015

whyrusleeping commented Oct 18, 2015