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

implement patch command #1299

Merged
merged 1 commit into from
Jun 3, 2015
Merged

implement patch command #1299

merged 1 commit into from
Jun 3, 2015

Conversation

whyrusleeping
Copy link
Member

This PR implements a basic DAG object patching utility, and another command to create new objects.

@whyrusleeping whyrusleeping added the status/in-progress In progress label May 28, 2015

var PatchCmd = &cmds.Command{
Helptext: cmds.HelpText{
Tagline: "Mutate a given merkledag object and return the modified node",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd avoid “mutate” for immutable IPFS objects ;). Maybe “Create a new Merkle DAG object based on an existing object”? Or “Create a new Merkle DAG object that's similar to an existing object”? Or something… ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

good point

}

switch req.Arguments()[1] {
case "add-link":
Copy link
Contributor

Choose a reason for hiding this comment

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

As I explain here, I'd also like an add-or-replace-link action (can you think of a more compact name?) to avoid either (a) adding two links with the same name or (b) requiring separate calls with rm-link and add-link. Not that (b) is so bad, but I expect this to be a common activity, so adding some sugar to handle it seems to be worth the cost of a separate action.

@whyrusleeping
Copy link
Member Author

this is RFM (unless other CR is wanted)


EMPTY_DIR=QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn
BAR=$(echo "bar" | ipfs add -q)
ipfs patch $EMPTY_DIR add-link foo $BAR
Copy link
Contributor

Choose a reason for hiding this comment

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

This still has inconsistent tab-vs.-space indenting. I think you should fix that, even if you don't use my object new suggestion for EMPTY_DIR ;).
#1299 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

+1

  • fix inconsistent tab-vs.-space indenting

@jbenet
Copy link
Member

jbenet commented Jun 2, 2015

@whyrusleeping CR above

  • use example with hash + new unixfs-dir implement patch command #1299 (comment)
  • fix inconsistent tab-vs.-space indenting
  • comment
  • patch data ?
  • ctx cancel fix
  • decop / parse args into vars
  • resolving paths

@jbenet
Copy link
Member

jbenet commented Jun 2, 2015

@wking @whyrusleeping i wonder if this command should "bubble" too?

@wking
Copy link
Contributor

wking commented Jun 2, 2015

On Mon, Jun 01, 2015 at 08:12:13PM -0700, Juan Batiz-Benet wrote:

@wking @whyrusleeping i wonder if this command should "bubble" too?

Bubbling is complicated, so I'd rather restrict the ‘ipfs object
patch’ implementation to creating a single new object. Once you try
to apply this idea to altering a unixfs tree, you're probably going to
want layout-adjustments (e.g. occasional rebalancing for fanout trees)
along with simple Merkle-update bubbling.

@jbenet
Copy link
Member

jbenet commented Jun 3, 2015

@whyrusleeping oh uh:

it this a problem with the import cycle thing we merged? oh maybe just need to rebase?

WIP: object creator command

better docs

move patch command into object namespace

dont ignore cancel funcs

addressing comment from CR

add two new subcommands to object patch and clean up main Run func

cancel contexts in early returns

switch to util.Key
jbenet added a commit that referenced this pull request Jun 3, 2015
@jbenet jbenet merged commit 6cff7e8 into master Jun 3, 2015
@jbenet jbenet removed the codereview label Jun 3, 2015
@jbenet jbenet deleted the feat/patch branch June 3, 2015 22:56
@whyrusleeping whyrusleeping mentioned this pull request Jun 9, 2015
50 tasks
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.

4 participants