add --name new flag when ipfs adding from stdin

[kjzz]

Closes #4986.

License: MIT Signed-off-by: Kejie Zhang 601172892@qq.com

[schomatis]

Hey @kjzz, take a look at https://github.com/ipfs/go-ipfs/issues/4986#issuecomment-417656218. We're mixing the name of the directory wrapper with the name of a file coming from stdin, which are two different issues.

[kjzz]

@schomatis thank you very much for your advice.I have modify my pr.

[kjzz]

@schomatis @keks In godoc,the file has a function named fullpath not fullname.So i use fullpath to judge the name, And In my test,if i use command such as

$cat ./test1.txt | ./ipfs add -w --name "myfile.txt"
added Qmf3xdREx34pewgYZjyvHQsrZxuk1TLUFdzoF95dsXfv3k myfile.txt
added QmSLquBbWRXs3VjTCJBXYv4BHn7NFNZxh5iU3zGNp5aMcf

the file path is also is empty string，I have log the full path in ipfs the log is:

09:20:50.138 INFO   coreunix: The file path is : add.go:475

not os.Stdin(/dev/stdin). So please help me review my pr.

[schomatis]

the file has a function named fullpath not fullname

You're right, we copied it incorrectly, it's FullPath().

I have log the full path in ipfs the log is:

09:20:50.138 INFO coreunix: The file path is : add.go:475 not os.Stdin(/dev/stdin).

Could you double check that please? If I add the line

fmt.Printf("Fullpath: %s\n", file.FullPath())

I'm seeing

Fullpath: /dev/stdin

(not an empty string).

Feel free to include the log line (temporarily) in the PR, to check that you're printing the correct attribute.

[kjzz]

@schomatis I have take a picture for you.And my computer is mac os.Maybe the computer system will cause difference?

[kjzz]

@schomatis And I have pushed my log line in pr

[kjzz]

@schomatis please help check it,Thx.

[schomatis]

And my computer is mac os.Maybe the computer system will cause difference?

Yes, that must be it, because with your patch I'm still seeing the /dev/stdin.

@keks Thoughts?

[kjzz]

@schomatis @keks so can i use

if (file.FullPath() == "/dev/stdin"||file.FullPath() == "") && adder.Name != "" {

to make different system compatible.

[kjzz]

And i have done some compatible processing.It is OK? @schomatis @keks

[keks]

Ah, FullPath, not FullName, sorry. I'm puzzled. We're using empty string FullPath() in multiple places as well so again I don't think it's a good check. Actually I believe it should return /dev/stdin on Mac and even Windows, I'm looking into that now.

[kjzz]

@keks But please look at my picture , it is return empty string in mac.

[keks]

Okay I just let this run on linux over HTTP and I also get an empty string. So this is not a Mac-only thing.

Unfortunately the story around which files have which names and paths is not very consistent, so it looks like we first need to make sure we can reliably detect the stdin file. Doing that now.

[kjzz]

@keks But i think that No matter the file add by HTTP or Stdin.It can still work.Can you give me a counterexample about how the pr will be wrong? Beceuse i just want to solve problem about the wrap file.You can take a look at this part. Thx a lot.

[schomatis]

Hey @kjzz, it's not that it won't work in this particular case, but we need to be sure this doesn't affect the API in different scenarios. I'm labeling this as blocked until @keks can figure out what's the best way to detect the stdin case. Please don't be discouraged by this setback, you did some nice work here, in the meanwhile you can focus on other issues to work on.

[kjzz]

Or do you have any idea about check stdin file? @keks

[keks]

I did more research on our possibilies here.

What seems to work with both local and http calls is to check that file.(files.FileInfo).AbsPath() == "/dev/stdin". There is a commit with this change on this branch of my repo: https://github.com/keks/go-ipfs/tree/zkj/feat. For the purpose of demonstration I left a fmt.Println in the code that prints the AbsPath. I don't want to merge that, though. Can you verify this works for both of you @schomatis and @kjzz?

[kjzz]

@keks Thx a lot. The file.(files.FileInfo).AbsPath() works for me.And i have update the code.So can you help me review my pr again? This pr to fix the issue #5399 .Thank you so much.

[keks]

@kjzz Also, it would be great if you could add sharness tests for this option. They are shell-script style tests. The test for ipfs add -w is here: https://github.com/ipfs/go-ipfs/blob/master/test/sharness/t0043-add-w.sh.

I propose you make a new file t0047-add-name.sh in which you test the --name option both with and without the -w flag. If you need any help, please ask! Sorry I forgot to say this earlier.

[schomatis]

This is working for me also.

[kjzz]

hi @schomatis @keks , i have add test in pr.Please help me review it .Thx a lot

[schomatis]

Hey @kjzz, thanks for taking the time to prepare those tests, given that the --name option is a minor one I don't think we need a new test file for it, also there's no need to test the -w option explicitly since we have decoupled the --name option from that one (we can, though, use the -w option to test ipfs cating a file through its name and not its hash), you can compact your tests into something like this

  test_expect_success "ipfs add --name" '
    HASH="QmdFyxZXsFiP4csgfM5uPu99AvFiKH62CSPDw5TP92nr7w" &&
    echo "IPFS" | ipfs add --name file.txt > actual &&
    echo "added $HASH file.txt" > expected &&
    test_cmp expected actual
  '

  test_expect_success "ipfs cat with name" '
    HASH=$(echo "IPFS" | ipfs add -w --name file.txt -Q) &&
    ipfs cat /ipfs/$HASH/file.txt > expected &&
    echo "IPFS" > actual &&
    test_cmp expected actual
  '

(feel free to modify/extend this test example in whatever way you see fit). Then you can add them at the end of test_add_cat_file().

[kjzz]

@schomatis hey ,i have update my test case.could you help me review it.Thx a lot

[keks]

Okay, the patch looks good. For some reason the sharness tests failed, but this looks unrelated. Anyway, I've restarted them to see whether that was a glitch or a reproducible problem. Let's wait until they finish.

[kjzz]

@keks thx a lot .i have fmt code again.if test have any problem,please ping me

[kjzz]

hey @keks ,there are something wrong with ci ?

[keks]

@kjzz I think I found the issue. In the sharness test you are setting the HASH and HASH1 environment variables. These are used in the subsequent tests in a call to ipfs cat $HASH, which now returns the wrong data. I prepared a commit on my branch: https://github.com/keks/go-ipfs/tree/zkj/feat

[kjzz]

hello @keks , thanks for your help , the pr has passed the required test .please help me review it.Thx a lot again.

[kjzz]

Hey @schomatis , i have updated test in pr.Please help me review it.Thanks a lot!

[kjzz]

hey @schomatis @keks , can you help me review my pr？I have finish my test file.Thx a lot.

[keks]

Thank you, these look good. Great work!

[keks]

@schomatis I don't have merge access. Are you down with making sure this gets merged?

[kjzz]

Hi @schomatis , if you have time to review my code . can you help me restart my pr test.There are something wrong with ci.

[kjzz]

Hi @Stebalien , i have add a checked type assertion in pr.Maybe it will be more robust.

[Stebalien]

@keks @schomatis are we sure this is the best way to fix this? This feels like something that should be fixed in go-ipfs-cmds itself (i.e., allow a --stdin-name flag and use this to pick the name for files added via stdin).

Note: As long as we can get agree on the name of the flag, we can merge this as-is. I'm just really not a fan of stringly typed stuff and, as-is, this fix is a quite hacky.

[kjzz]

hey @Stebalien , i have update the stdin-name,and i know what you worried about.And i think that this way is a useful way to fix the problem but maybe is the not best one.

[kjzz]

@Stebalien If you have any advice,you can ping me.Thx a lot for helping me review pr.

[keks]

@Stebalien Hm, yes that should also work. Now that you mention it, that would be cleaner. Maybe let's first get cmds#112 done, though :)

But yeah, --stdin-name is also good.

[schomatis]

@Stebalien Agreed that this is far from optimal, but I think the patch is cleanly encapsulated inside the Adder and it doesn't decrease code quality. We can use this as a first step towards the correct solution inside go-ipfs-cmds, also @kjzz has invested a significant amount of time and effort following our (sub-optimal :) advice so it would be nice to get this merged.

[Stebalien]

@schomatis, @keks you're right. This is a strict improvement, even if there's a better way to do this, we might as well merge this and improve it later.

[Stebalien]

Want to make sure @kevina and I are on the same page and then we can merge this. UX changes like this are always tricky...

[kevina]

@Stebalien if the goal is for this to only every work from naming an otherwise unnamed file from stdin I think I am okay with --stdin-name a more functional --name could also be useful, but if we are abandoning the idea I think I am okay with this.

I will get back to this later today or tomorrow and give my final approval then.

[kjzz]

Hey @kevina , the pr is aimed at solving name an otherwise unnamed file from stdin,and i know it is not the best solution.And i will continue to perfect it in the next step.In addition,if @Stebalien have any new issue or advice about this,please ping me and i am very glad to work on it.Thx a lot.

[Stebalien]

(this has 3 signoffs so I think it's ready to go; we can always change our minds later as long as we do it fast enough)

[kevina]

Yeah I don't have a major problem with it --stdin-name.

[djdv]

I'm late on this, and not sure how important it is. But doesn't this unnecessarily break struct alignment? https://github.com/kjzz/go-ipfs/blob/b7ea4bfc2a2c3b864d688c108245a3429029e185/core/coreunix/add.go#L79-L88

[keks]

I guess it's not too bad because most bools are consecutive and form a 7-byte block of 1-byte aligned values. I guess we could swap Chunker and NoCopy to save eight bytes, or reorder the whole thing to have the bools at the bottom, but I'm not sure it's worth it.

[Stebalien]

Yeah, we could fix this but this isn't exactly a bottleneck. Feel free to submit a patch if you want but it's probably not worth the time.