fix(cli): ipfs add with multiple files of same name

[schomatis]

This PR incorporates the go-ipfs-cmds fix that makes the ipfs add command not ignore repeated entries. This fix conflicts with another bug in the ipfs add -w variant of the command which can't correctly process files with the same name (I think this was uncovered here and we don't have a specific issue for it). This is shown by the shanress test failing (see more details below). We need to first fix the ipfs add -w issue before landing this so we can be confident this is the correct fix.

---

Detail of the ipfs add -w bug and the test that fails (uncovering it)

The ipfs add -w (repeats) test now fails. This is expected because:

• we repeatedly add the m/4r93 file
• ignored before, these extra/repeated files are now sent to the core UnixFS API Add()
• this API uses a fake (disposable) MFS root directory as a way to interact with the underlying DAG service (repo)
• with the "wrapped" option -w we group all the entries being added in a single directory
• the MFS directory, not intended for this ephemeral abuse, correctly refuses to add two entries with the same name (with the resulting error in sharness of a failed command: Error: directory already has entry by that name)

We are abusing the MFS API, see related and broader issue for more details on this general topic: Decouple MFS from UnixFS.

Also note that this issue might not happen with any kind of repeated entry in the ipfs add -w command as empty directories seem to be ignored when constructing the path and only files within those repeated directories will trigger the error, like ipfs add -w -r dir/file dir/file. There is some undocumented logic around path manipulation related to this:

https://github.com/ipfs/go-ipfs/blob/52a747763f6c4e85b33ca051cda9cc4b75c815f9/core/coreunix/add.go#L411-L424

---

Fixes #8264.

cc @guseggert

[schomatis]

Do we need to create a release here after tests pass?

[aschmahmann]

Once this PR is approved we should merge + release go-ipfs-cmds and bubble into this PR before merging it into master

[guseggert]

What's your estimate on how long it would take to fix #8541 ? If we fix that, then all the weird edge cases here would go away, right?

[schomatis]

@guseggert

What's your estimate on how long it would take to fix #8541 ?

This largely depends on review bandwidth and (optionally) being able to finally consolidate UnixFS/MFS back here (as we would likely be breaking a bunch of APIs).

If we fix that, then all the weird edge cases here would go away, right?

The general scope of that issue is too broad and vague at the moment, but achieving the specific use case of removing MFS from ipfs add is not hard and I think doing that would, if not solve every edge case here, largely simplify this issue.

[aschmahmann]

We are abusing the MFS API, see related and broader issue for more details on this general topic: #8541.

It doesn't seem like this is really about MFS as much as it is about some unintuitive behavior around what -w does with repeat parameters that leads to the behavior being undefined in the case of multiple files with the same name.

❯ ipfs add .\foo .\bar
added QmaZAaCrugAkinkaSGBP2uD8pchyKXY2rmPf8WQ2CzE2pN bar
added QmQ7qCwMvm3mb5PRaAJd1ytaAE4qULHzoJCunikZfT1oAp foo

This seems to indicate that ipfs add <file1> <file2> is just a shortcut for calling ipfs add file1 followed by ipfs add file2.

❯ ipfs add -w .\foo .\bar
added QmaZAaCrugAkinkaSGBP2uD8pchyKXY2rmPf8WQ2CzE2pN bar
added QmQ7qCwMvm3mb5PRaAJd1ytaAE4qULHzoJCunikZfT1oAp foo
added QmSQUBkpW1ApwzXK8Z23P4uL8jJkss31rd49bWG2SR6c2g

This seems to indicate that ipfs add -w <file1> <file2> adds file1 and file2 into a shared directory keyed by their file names. While this is an interesting and perhaps useful behavior, it's not the same as the repeated adds being a shortcut for ipfs add -w <file1> followed by ipfs add -w file2>.

This behavior seems to have been introduced intentionally ages ago #1536 and while likely not that common (due to bugs like this one with multiple files with the same name not being repeated flagged + upvoted by users) it's definitely in use (e.g. a cursory look on grep.app shows https://github.com/ipfs/public-gateway-checker/blob/16c99857e4f4cb67ff5a8bc617ec9fa44bdc8a6d/publish-to-ipfs.sh#L3)

In this case what happens when you do ipfs add -w .\test1\foo .\test2\foo is undefined and there could be any number of potential options:

1. Only one file gets added and that one is wrapped in a directory
2. Both files are added and only one gets wrapped in a directory
3. Both files are added and are put in the same directory but one of them has an autogenerated name (e.g. foo(1))
4. There's an error

It seems to me like the only sensible thing to do is to error if ipfs add -w contains entries with the same name. I'm not sure what the intent behind this test https://github.com/ipfs/go-ipfs/blob/2a871ef0184da3b021209881dfa48e3cfdaa9d26/test/sharness/t0043-add-w.sh#L131-L142 was, but if we want we can maintain compatibility here by only erroring if the entry paths are different (e.g. allowing ipfs add -w foo foo to be deduplicated to remove the last foo). We should also document the interesting behavior of ipfs add -w <file1> <file2> vs other ipfs add <file1> <file2> since at the moment the docs in ipfs add --help leave it ambiguous.

[schomatis]

@aschmahmann Thanks for the detailed explanation. Could you expand a bit more on this suggestion, please?

we can maintain compatibility here by only erroring if the entry paths are different

Maybe an example of when would we error?

[aschmahmann]

Maybe an example of when would we error?

The undefined behavior of when we should error is ipfs add -w .\test1\foo .\test2\foo (i.e. any time we would have two entries being added with the same name, which would necessitate an override or an error we should error)

The sharness test you linked https://github.com/ipfs/go-ipfs/blob/2a871ef0184da3b021209881dfa48e3cfdaa9d26/test/sharness/t0043-add-w.sh#L131-L142 is pretty weird in that it does intentionally do the equivalent of ipfs add -w .\test1\foo .\test1\foo and not expect an error.

At a high level if we want to continue to support that case (instead of just erroring from two files called foo), we can do so by either:

1. Checking if the import paths are the same .\test1\foo .\test1\foo and just not parsing the data a second time (does enable the backwards compatibility with the linked test)
2. Checking if the data in .\test1\foo and .\test2\foo are the same (and therefore will result in the same CID) and then ignoring the second entry (a more generous way of enabling backwards compatibility in more situations where it's safe)

Aside from the fact that technically this would be an unforced breaking change, if 1 or 2 are a pain to implement we'd probably be fine to modify the sharness test and just not support entries with the same name whether they have the same path or content. We'll be generating errors so in the unlikely event this breaks someone's workflow it'll be detectable and we could deal with it later. I'd rather not have breaking changes if we can avoid it, but we can evaluate depending on how painful you think it'll be to implement.

[schomatis]

Got it, thanks. I think we can implement this by extending the -cmds lib to retain the path and decide what to do in ipfs add here.

[schomatis]

According to Discord discussion we're going with:

3. Error if there would be a conflicting path name, but don't if the DAGs from the conflicting names would be identical - shouldn't require any API changes to go-ipfs-cmds or go-ipfs-files and breaks the least backwards compatibility. Might be a bit more work to deal with in the coreunix.Adder and we'll likely end up putting some blocks in the repo that will only get removed with GC, but that's fine.

to minimize messing around with go-ipfs-cmds or go-ipfs-files.

[schomatis]

@aschmahmann

I feel we have been going back and forth on the issue of which error to display to the user in go-ipfs which is not a blocker for my work (nor do I have any strong opinion on it).

My blocker is the following: AFAICT any of the proposed errors need go-ipfs to be aware of the import path, which is not available at the moment. I have asked for a simple way to get it in Discord and didn't get an answer. I have gotten pushback to modifying upstream dependencies but don't see any other way, so I need a decision on which dependency to modify. I'll repeat my original Discord question here:

I'm thinking we can satisfy #8493 (comment) with either:

• extend Node interface to also provide the path and filter/error in ipfs add - https://github.com/ipfs/go-ipfs-files/blob/5044b18695cb23ae69632c3e2511b4624a87b0d5/file.go#L16-L24
• do some preprocessing in go-ipfs-cmds to filter/error files there before reaching ipfs add

[guseggert]

Let's roll with doing the preprocessing in go-ipfs-cmds. Does that unblock you?

[schomatis]

Yes, thanks.

[schomatis]

@guseggert Ready for review.

[schomatis]

@guseggert Updated dependency, this should be ready now.