add --name new flag when ipfs adding from stdin #5399

kjzz · 2018-08-23T11:29:15Z

Closes #4986.

License: MIT
Signed-off-by: Kejie Zhang [email protected]

schomatis · 2018-08-23T12:36:25Z

core/coreunix/add.go

@@ -470,6 +472,9 @@ func (adder *Adder) addFile(file files.File) error {
 		return err
 	}

+	if !strings.EqualFold(adder.WpName, "") && adder.Wrap {


This is the place a normal file is added, not the directory wrapper.

sorry for delay.you can look at this picture.

when i use command 'cat' to get file,you can use --name to name the file.Just like next picture.

i have rebase repo and please help me review this pr @schomatis

Let's drop the adder.Wrap check.

License: MIT Signed-off-by: Kejie Zhang <[email protected]>

schomatis · 2018-08-31T13:01:39Z

Hey @kjzz, take a look at #4986 (comment). We're mixing the name of the directory wrapper with the name of a file coming from stdin, which are two different issues.

schomatis

The main issue in this review iteration is decoupling the --name flag from the -w flag, although they are related from the user perspective, in the code these are two different issues, and removing the wrap references could make the PR easier to understand.

schomatis · 2018-09-02T18:40:50Z

core/commands/add.go

@@ -37,6 +37,7 @@ const (
 	progressOptionName    = "progress"
 	trickleOptionName     = "trickle"
 	wrapOptionName        = "wrap-with-directory"
+	wrapPathName          = "name"


So, as discussed in the issue, this is not exactly related to the -w (wrap) flag, is a more generic issue of assigning names to a unnamed file content (such as the one received from stdin). Let's change the name of this option to reflect that, e.g., wrapPathName -> pathName.

schomatis · 2018-09-02T18:42:16Z

core/commands/add.go

@@ -116,6 +117,7 @@ You can now check what blocks have been created by:
 		cmdkit.BoolOption(trickleOptionName, "t", "Use trickle-dag format for dag generation."),
 		cmdkit.BoolOption(onlyHashOptionName, "n", "Only chunk and hash - do not write to disk."),
 		cmdkit.BoolOption(wrapOptionName, "w", "Wrap files with a directory object."),
+		cmdkit.StringOption(wrapPathName, "Assign path name when use wrap-with-directory option"),


Similar to the above comment, let's drop the wrap reference and say something like: "Assign a name to an unnamed file" (feel free to reword this).

schomatis · 2018-09-02T18:44:06Z

core/coreunix/add.go

@@ -83,6 +84,7 @@ type Adder struct {
 	RawLeaves  bool
 	Silent     bool
 	Wrap       bool
+	WpName     string


WpName -> Name

schomatis · 2018-09-02T18:44:10Z

core/coreunix/add.go

@@ -470,6 +472,9 @@ func (adder *Adder) addFile(file files.File) error {
 		return err
 	}

+	if !strings.EqualFold(adder.WpName, "") && adder.Wrap {


Let's drop the adder.Wrap check.

schomatis · 2018-09-02T18:48:15Z

core/coreunix/add.go

@@ -470,6 +472,9 @@ func (adder *Adder) addFile(file files.File) error {
 		return err
 	}

+	if !strings.EqualFold(adder.WpName, "") && adder.Wrap {


Do we need an EqualFold call to compare to an empty string? I don't think the case is a problem here.

schomatis · 2018-09-02T18:56:41Z

core/coreunix/add.go

@@ -470,6 +472,9 @@ func (adder *Adder) addFile(file files.File) error {
 		return err
 	}

+	if !strings.EqualFold(adder.WpName, "") && adder.Wrap {
+		return adder.addNode(dagnode, adder.WpName)


I'd rather avoid duplicating the function call, we can define a new variable here, e.g., addFileName := file.FileName(), and if that name is "" we set it to adder.Name. e.g.,

addFileName := file.FileName() if addFileName == "" && adder.Name != "" { addFileName = adder.Name adder.Name = "" // Unset the `Name` option to avoid applying the same name to many unnamed files. // TODO: This could be turned into a FIFO queue to handle the scenario with more than one unnamed file. } adder.addNode(dagnode, addFileName)

We do two checks:

Apply the --name option only to unnamed files (e.g., coming from stdin).

Apply the --name option only once (since I fear that the HTTP commands library may be able to send many unnamed files), otherwise we would have many files with the same name in one directory.

License: MIT Signed-off-by: Kejie Zhang <[email protected]>

kjzz · 2018-09-03T03:11:45Z

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

schomatis · 2018-09-03T14:01:13Z

core/coreunix/add.go

@@ -470,8 +471,13 @@ func (adder *Adder) addFile(file files.File) error {
 		return err
 	}

+	addFileName := file.FileName()
+	if addFileName == "" && adder.Name != "" {


@keks WDYT about checking for an empty filename? I think this covers the stdin case in CLI, I'm not sure what effect this may have in the HTTP case, that's why we're resetting the adder.Name option to rename only one file (in the case HTTP may send many unnamed files).

It looks like there are several occasions where we use an empty string as file name. I just looked into this and it looks like when using stdin, te FullName() should be os.Stdin.Name(), i.e. /dev/stdin. IMHO that would be a better check, and should also possible on the http server side.

hello @keks ,i want to fix this bug.And i do not understand theFullname().can u explain more for me? my pr is aim to solve issue #5399 . I have explain to @schomatis why I do for this.You can see at this . Thx.

@kjzz Sure! The file variable is a "files".File, an interface described here: https://godoc.org/github.com/ipfs/go-ipfs-cmdkit/files#File. It has the method FileName(), which is currently used in the check. But it also has the method FullName(), which should return /dev/stdin if the file is stdin. If you check file.FullName() == "/dev/stdin" or file.FullName() == os.Stdin.Name(), that should be more reliable, because there are other reasons why file.FileName() may be the empty string. I only grepped through the code in go-ipfs-cmds and saw a lot of files with empty string as name. I'm not sure this will be a problem in practice, but I can't rule out that it won't. I think checking file.FullName() should be much more reliable.

Thanks @keks! That sounds much more robust, @kjzz, so the check would be

if file.FullName() == os.Stdin.Name() && adder.Name != "" {

keks

Let's find a better way to check for stdin, looks good otherwise.

keks · 2018-09-04T16:16:57Z

core/coreunix/add.go

@@ -470,8 +471,13 @@ func (adder *Adder) addFile(file files.File) error {
 		return err
 	}

+	addFileName := file.FileName()
+	if addFileName == "" && adder.Name != "" {


It looks like there are several occasions where we use an empty string as file name. I just looked into this and it looks like when using stdin, te FullName() should be os.Stdin.Name(), i.e. /dev/stdin. IMHO that would be a better check, and should also possible on the http server side.

schomatis · 2018-09-05T00:05:35Z

core/commands/add.go

@@ -116,6 +117,7 @@ You can now check what blocks have been created by:
 		cmdkit.BoolOption(trickleOptionName, "t", "Use trickle-dag format for dag generation."),
 		cmdkit.BoolOption(onlyHashOptionName, "n", "Only chunk and hash - do not write to disk."),
 		cmdkit.BoolOption(wrapOptionName, "w", "Wrap files with a directory object."),
+		cmdkit.StringOption(pathName, "Assign path name when use wrap-with-directory option"),


Let's change this to reflect that the option (with the new check) works only for stdin, e.g., "Assign a name if the file source is stdin" (or something like that, feel free to reword it).

kjzz · 2018-09-05T01:38:46Z

@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 · 2018-09-05T01:59:13Z

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 · 2018-09-05T02:03:40Z

@schomatis

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

kjzz · 2018-09-05T02:05:34Z

@schomatis And I have pushed my log line in pr

kjzz · 2018-09-05T02:15:15Z

@schomatis please help check it,Thx.

schomatis · 2018-09-05T02:18:16Z

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 · 2018-09-05T02:26:48Z

@schomatis @keks so can i use

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

to make different system compatible.

kjzz · 2018-09-05T06:50:10Z

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

License: MIT Signed-off-by: Kejie Zhang <[email protected]>

schomatis

@kjzz Great! Thanks for taking the time and effort to get this properly fixed.

Stebalien · 2018-09-10T16:13:46Z

core/coreunix/add.go

@@ -470,8 +471,14 @@ func (adder *Adder) addFile(file files.File) error {
 		return err
 	}

+	addFileName := file.FileName()
+	addFileInfo := file.(files.FileInfo)


@keks are you sure this won't panic in some cases? We should probably use a checked type assertion (or comment on why this must be true).

kjzz · 2018-09-11T02:17:45Z

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

Stebalien · 2018-09-11T21:16:39Z

core/commands/add.go

@@ -116,6 +117,7 @@ You can now check what blocks have been created by:
 		cmdkit.BoolOption(trickleOptionName, "t", "Use trickle-dag format for dag generation."),
 		cmdkit.BoolOption(onlyHashOptionName, "n", "Only chunk and hash - do not write to disk."),
 		cmdkit.BoolOption(wrapOptionName, "w", "Wrap files with a directory object."),
+		cmdkit.StringOption(pathName, "Assign a name if the file source is stdin."),


Given that this'll only work for STDIN, I'd consider calling this --stdin-name. I believe the original issue was suggesting a more general --name flag (even for regular files) but I actually prefer how this patch currently works.

That seams a little two verbose to me. Also, in the future want to allow this to override an existing name and then we will be stuck with both a --name and --stdin-name option.

For now I recommend we keep it as is and instead return an error (or maybe just a warning) when the option won't have any effect.

Also, in the future want to allow this to override an existing name and then we will be stuck with both a --name and --stdin-name option.

Actually, my thought was that this should never be a general-purpose flag. I believe that was the original suggestion but, as was discovered when writing this patch, that's ambiguous with multiple inputs. We also don't want to change the semantics at some point in the future (IMO).

We could also have a "rename" feature but we'll have to carefully consider how we want to do it.

Note, by "multiple inputs" I mean:

> ipfs add -w --name "foo" file1 file2

We can only ever have one stdin so setting the name for stdin is unambiguous. We can easily add multiple files at the same time.

Stebalien · 2018-09-11T21:35:00Z

@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.

License: MIT Signed-off-by: Kejie Zhang <[email protected]>

kjzz · 2018-09-11T22:01:56Z

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 · 2018-09-12T06:27:56Z

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

keks · 2018-09-12T11:23:53Z

@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 · 2018-09-12T16:33:39Z

@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 · 2018-09-12T18:49:18Z

@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 · 2018-09-12T18:53:40Z

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 · 2018-09-12T19:17:01Z

@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 · 2018-09-13T02:32:55Z

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 · 2018-09-13T21:44:36Z

(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 · 2018-09-14T01:22:48Z

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

djdv · 2018-09-19T20:12:17Z

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 · 2018-09-19T20:25:02Z

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 · 2018-09-28T21:48:34Z

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.

kjzz requested a review from Kubuxu as a code owner August 23, 2018 11:29

kjzz force-pushed the zkj/feat branch from 6898c4a to 80bb565 Compare August 23, 2018 11:40

kjzz mentioned this pull request Aug 23, 2018

[FR] ipfs add -w --name when adding from stdin #4986

Closed

schomatis reviewed Aug 23, 2018

View reviewed changes

kjzz force-pushed the zkj/feat branch from 80bb565 to d39e85f Compare August 31, 2018 08:03

add --name new flag when ipfs adding from stdin

514891d

License: MIT Signed-off-by: Kejie Zhang <[email protected]>

kjzz force-pushed the zkj/feat branch from e94fdea to 514891d Compare August 31, 2018 12:03

schomatis reviewed Sep 2, 2018

View reviewed changes

drop wrap check and modify parameter name

19e3c12

License: MIT Signed-off-by: Kejie Zhang <[email protected]>

kjzz force-pushed the zkj/feat branch from 02b6092 to 19e3c12 Compare September 3, 2018 03:02

schomatis requested a review from keks September 3, 2018 13:58

schomatis reviewed Sep 3, 2018

View reviewed changes

keks suggested changes Sep 4, 2018

View reviewed changes

schomatis reviewed Sep 5, 2018

View reviewed changes

kjzz force-pushed the zkj/feat branch 2 times, most recently from 5433eea to ea9087c Compare September 5, 2018 01:31

kjzz force-pushed the zkj/feat branch from 2c5a337 to f32340b Compare September 5, 2018 02:14

kjzz force-pushed the zkj/feat branch from f32340b to 0b4817d Compare September 5, 2018 06:48

kjzz force-pushed the zkj/feat branch from 0b4817d to 026b77c Compare September 5, 2018 06:51

use file fullpath to judge file name

b61dae3

License: MIT Signed-off-by: Kejie Zhang <[email protected]>

schomatis approved these changes Sep 8, 2018

View reviewed changes

schomatis added RFM and removed status/in-progress In progress labels Sep 8, 2018

Stebalien requested changes Sep 10, 2018

View reviewed changes

kjzz force-pushed the zkj/feat branch from 2e353f9 to 8ddb437 Compare September 11, 2018 02:19

Stebalien self-requested a review September 11, 2018 19:36

Stebalien reviewed Sep 11, 2018

View reviewed changes

add type assertion checked

7de83f2

License: MIT Signed-off-by: Kejie Zhang <[email protected]>

kjzz force-pushed the zkj/feat branch from 8ddb437 to fba5fa1 Compare September 11, 2018 21:57

modify command name and update the test

b7ea4bf

License: MIT Signed-off-by: Kejie Zhang <[email protected]>

kjzz force-pushed the zkj/feat branch from fba5fa1 to b7ea4bf Compare September 11, 2018 21:59

Stebalien mentioned this pull request Sep 12, 2018

Add a built-in --stdin-name flag for setting the filename for stdin ipfs/go-ipfs-cmds#119

Open

Stebalien approved these changes Sep 12, 2018

View reviewed changes

Stebalien requested a review from kevina September 12, 2018 18:49

Stebalien merged commit 7213328 into ipfs:master Sep 13, 2018

add --name new flag when ipfs adding from stdin #5399

add --name new flag when ipfs adding from stdin #5399

Conversation

kjzz commented Aug 23, 2018 • edited by schomatis Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schomatis commented Aug 31, 2018

schomatis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjzz commented Sep 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjzz Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjzz commented Sep 5, 2018 • edited Loading

schomatis commented Sep 5, 2018

kjzz commented Sep 5, 2018

kjzz commented Sep 5, 2018

kjzz commented Sep 5, 2018

schomatis commented Sep 5, 2018

kjzz commented Sep 5, 2018 • edited Loading

kjzz commented Sep 5, 2018

schomatis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjzz commented Sep 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Sep 11, 2018

kjzz commented Sep 11, 2018 • edited Loading

kjzz commented Sep 12, 2018

keks commented Sep 12, 2018

schomatis commented Sep 12, 2018

Stebalien commented Sep 12, 2018

Stebalien commented Sep 12, 2018 • edited Loading

kevina commented Sep 12, 2018

kjzz commented Sep 13, 2018

Stebalien commented Sep 13, 2018

kevina commented Sep 14, 2018

djdv commented Sep 19, 2018

keks commented Sep 19, 2018

Stebalien commented Sep 28, 2018

kjzz commented Aug 23, 2018 •

edited by schomatis

Loading

kjzz Sep 4, 2018 •

edited

Loading

kjzz commented Sep 5, 2018 •

edited

Loading

kjzz commented Sep 5, 2018 •

edited

Loading

kjzz commented Sep 11, 2018 •

edited

Loading

Stebalien commented Sep 12, 2018 •

edited

Loading