Fix path parser in add command

[djdv]

Parse paths with filepath instead of just path https://github.com/ipfs/go-ipfs/issues/1922

[jbenet]

this LGTM -- cc @rht this should be changed in dev0.4.0 too.

[jbenet]

can someone else with windows confirm this works? (we dont yet have windows CI)

[jbenet]

we need a windows CI -- https://github.com/ipfs/infrastructure/issues/110

[rht]

@djdv what about the case for ipfs add X:\path\to\file1 X:\path\to\file2 ...? This case is not in test yet. If you can change https://github.com/ipfs/go-ipfs/blob/master/commands/cli/parse.go#L387 to filepath, that would be great.

Also, optionally, if you can possibly refactor filepath.Split -> filepath.Base, if you grep the code with '_, name :=.*split'.

How many test in windows fail before/after the PR?

[djdv]

ipfs add X:\path\to\file1 X:\path\to\file2 Adds both/n files and then creates a directory hash for them. The files contain the full path as their name though which does not seem to be the intended behavior. I.e. "/ipfs/

/X:\path\to\file1" instead of "/ipfs//file1". It seems like only files underneath a directory are being formed correctly with the change. I'll have to look into this and compare with the behavior on other platforms, I'm not sure if that directory hash is intended to exist on multiple file adds without the -w argument, need clarification on that.

That's the only instance of .Split in add.go so I've changed it to be .Base.

There appears to be one less failed test with this change however I don't think I'm getting the count right, I'm running go test -tags nofuse "./..." | tee test.log in go-ipfs and then doing a grep -o FAIL test.log | wc -l With that I'm getting a count of 35 (before) and 34(after).

[rht]

@djdv

It seems like only files underneath a directory are being formed correctly with the change.

You're describing the #1878 case, which should be fixed by path -> filepath https://github.com/ipfs/go-ipfs/blob/master/commands/cli/parse.go#L387. Notice that Base only returns one argument, and should fix the ci test as well.

That's the only instance of .Split in add.go so I've changed it to be .Base.

Right, though I was referring to the entire codebase. There are two more in https://github.com/ipfs/go-ipfs/blob/master/core/corehttp/gateway_handler.go#L164 and https://github.com/ipfs/go-ipfs/blob/master/core/coreunix/add.go#L139. But only modify the corehttp/gateway_handler.go one, because coreunix/add.go has been refactored in dev0.4.0. I will finish this part of your PR in dev0.4.0 branch.

I'm running go test -tags nofuse "./..." | tee test.log in go-ipfs and then doing a grep -o FAIL test.log | wc -l With that I'm getting a count of 35 (before) and 34(after).

There is no windows option in travis yet, so we need a real and breathing windows machine for now. This test.log for before/after will be super useful if you post it on an ipfsbin. Also if you don't mind to include the output to the sharness test as well? Just run TEST_VERBOSE=1 make in test/sharness dir.

[rht]

It seems like only files underneath a directory are being formed correctly with the change.

Looks like this statement referred to the test you did before the Split -> Base refactor? Which means the filepath doesn't fix the relative path issue?

[rht]

If this is fixed, will close all #1676, #1730, #1878, #1922 at once.

[djdv]

@rht I wouldn't yet, there seems to still be issues with some of the filenames, particularly ones not under a directory. Gathering test logs and writing up examples now.

[djdv]

@rht

Notice that Base only returns one argument, and should fix the ci test as well.

Woops, I'm being a bit hasty.

Here are the logs: Origin (f36ada87f6564b1a3f48d3de7258a8b881efe379) http://ipfsbin.xyz/#QmRRk8djHj7sftpqnJn8i9McMJ2wzXJFhqVj86KhjT1Qxa Fork (e84926229d185d650ecaca79c412896fb0df0bb8) http://ipfsbin.xyz/#QmRCdMWtR8HFcVxh5EVLGH7eGa6EgPz2zt1RVeWvbZbigE Error count went down to 33. If any other tests are needed let me know.

Also if you don't mind to include the output to the sharness test as well?

Due to the way Windows cmd handles "*.go" I can't run those make files "natively" but I can inside of mingw bash, however I don't know how to pass the "nofuse" build constraint to the go tool unless I edit the makefiles.

Looks like this statement referred to the test you did before the Split -> Base refactor? Which means the filepath doesn't fix the relative path issue?

That seems to be the case, 4c300d9459e7ad2c9f5ef5d1fd182cbbf7b66cd7 - e84926229d185d650ecaca79c412896fb0df0bb8 only fixed filenames for files under a directory. Explore the hashes below and compare with the tree in #1922 to see what I mean.

ipfs revision, command, output as ipfs hash

f36ada87f6564b1a3f48d3de7258a8b881efe379 : ipfs add -r "windows directory" QmYtNSri8H4wstyaEwHe9gWRM1cSADCNNJBNcw3mtQuB2T ipfs add -r "J:\demo\ipfs\windows directory" QmfPYceFpW2JNJcN8NpyncM7vT8tkasnd7pG5A6xQs7RUd

e84926229d185d650ecaca79c412896fb0df0bb8 : ipfs add -r "windows directory" QmVu8MgHzuF7asFYx8VxsXXt1QxPS3GYu3QBx17pWFg9Mr ipfs add -r "J:\demo\ipfs\windows directory" QmVu8MgHzuF7asFYx8VxsXXt1QxPS3GYu3QBx17pWFg9Mr

Notice the level 3 files appearing in the root of the structure and the filenames of those files, the ones inside of the actual directory seem to be fixed though compared to before. I'm not sure what's causing that to happen.

-Edit- More examples: These are the same on both revisions J:\demo\ipfs>ipfs add "Listing 1.PNG" "Listing 1B.PNG" "Listing 2.PNG" Listings.png QmS4djeiuH1HDRFfnu4Lo3ebvd8382LSL4DUr9qyJgy8Gs J:\>ipfs add "J:\demo\ipfs\Listing 1.PNG" "J:\demo\ipfs\Listing 1B.PNG" "J:\demo\ipfs\Listing 2.PNG" J:\demo\ipfs\Listings.png QmbJkHK4WGGDUcQpJ9hTP76sFwdHd6TEEtX2i6GLvANMkr

-Edit 2015-11-07- The previous hash for revision e84926229d185d650ecaca79c412896fb0df0bb8 (QmYnEVf8mS3d1yQFMXrgopvaimyQm53tV6x9YEeJCoJXh7) is NOT correct. The hash matches the other command, the above has been edited to reflect this.

[rht]

I checked the log, the only path error is \tassets_test.go:40: asset ..\\vendor\\dir-index-html-v1.0.0\\dir-index.html: could not read vcs file: open ..\\vendor\\dir-index-html-v1.0.0\\dir-index.html: The system cannot find the path specified. It appears that ioutil.ReadFile doesn't parse '..' in windows. There are merkledag key errs as well. The rest (blockservice, bitswap, repo lock, routing ...) was reported in #959.

To summarize the filename cases:

1. ipfs add -r pth/to/file1 ... when the files are within $PWD and the arg doesn't contain abs path. symptom: duplicates are created fixed by using filepath (this PR for now)
2. ipfs add -r /abs/pth/to/file1 ... when the files are specified with absolute path symptom: filenames contain absolute path
3. ipfs add -r ../file symptom: filename still contains '../' prefix (#1878)

@djdv https://github.com/ipfs/go-ipfs/blob/master/commands/files/serialfile.go#L72 is where each file's filename is created during the recursive add. I wonder if applying fp.Base to the second arg of fp.Join in fileName := fp.Join(f.name, stat.Name()) could fix the problem in case 2 and 3.

Edit: s/either or both of the/the second/

[djdv]

@rht Unfortunately that change doesn't seem to have fixed the issue, I'll have to step through the process at some point to see what's going on where. My knowledge of the project and its structures is low though so continued assistance is appreciated!

In addition I'm getting an inconsistent error count on my tests, it seems I am getting between 33-38 failures on the same revision(both in the origin and this fork) , I'll have to log several runs and find the discrepancies, unfortunately the tests take some time on my machine.

[rht]

@djdv I went through the ipfs add process and couldn't find where else the absolute path could have remained.

I found a small bug, that https://github.com/ipfs/go-ipfs/blob/master/commands/cli/parse.go#L360 needs to be wrapped with filepath.Clean, i.e. fpath := filepath.Clean(inputs[0]). If you can add this. See http://plan9.bell-labs.com/sys/doc/lexnames.html. Also, filepath.Clean parses ipfs add -r "" as ipfs add -r .. It likely doesn't fix the '../' case.

If only there are further cases to characterize the err. Whether it is caused by the cli parse, or the recursive add.

Yes, the test failures in #959 were reported to vary as well.

[djdv]

Latest commit is just a squashed version of all previous commits with the addition of cleaning the input along with a change related to gateway_handler.go. A previous commit used filepath instead of regular path in that file, this was reverted because it caused the gateway to return incorrect ipfs paths.

serialfile.go is unaltered for now since the suggested change seems to have no effect, further testing required.

Worth noting is the edit to a comment of mine above, edited 2015-11-07 the previous (as well as this) commit resulted in the same hash for both the relative and absolute command which is a step in the right direction.

As it stands right now for the directory structure in https://github.com/ipfs/go-ipfs/issues/1922 we get the following hash on Windows: QmVu8MgHzuF7asFYx8VxsXXt1QxPS3GYu3QBx17pWFg9Mr The intended hash and structure is (taken from ipfs running on FreeBSD) : QmPbRRZjtJgfX25tJzYUPceQXxUX9tWb5eRqFSMChLUKKy

Issue is still unresolved, this is just a status update mainly to show the target result/hash for a recursive add.

@rht

If only there are further cases to characterize the err. Whether it is caused by the cli parse, or the recursive add.

I intend to run the binary through GDB sometime soon to see what values are at various times as well as see the code paths that are traversed. I worry I may have issues with this approach on Windows but we'll see, if there is a better approach than this I'd like to know.

[djdv]

No luck with GDB with Go files on Windows (segfaults on everything).

It looks like there may be a problem with the creation of the file object itself and the names being passed in, if not here it is somewhere else further down the chain. Issue is still unresolved and this commit breaks recursive adding on other platforms, it's looking closer to target here so more testing required. Current hash on Windows: QmRuhkiQ6DNwqLKmv4freHdvGet1Xg5aaHhNRgJLUEJdJv

The accidental extra line will be squashed in the next commit.

Worth noting is that the same call is made in the unix adder as well. https://github.com/ipfs/go-ipfs/blob/master/core/coreunix/add.go#L51

-Edit- serialfile.go should be unchanged actually, only the arguments in calls to NewSerialFile should be cleaned, not cleaned inside the method itself. That commit accidentally calls Base on the same path.

[rht]

@djdv in this case you may just fmt.Printf the info you need, e.g. in https://github.com/ipfs/go-ipfs/pull/1933/files#diff-bd0ca9f1e2d5c7ba128af01ec4c1131cL441, fmt.Printf("addDir: %s", filepath.Base(file.FileName())) and see where things went wrong.

Yes, there seems to be duplication of usage of filepath.Base in the PR.

From the latest hash, it appears that the fullpath has now been trimmed into name?

The remaining problem is duplication of files being added? This is as if https://github.com/ipfs/go-ipfs/blob/master/commands/files/serialfile.go#L34 lists the entire content of a directory recursively.

[djdv]

This commit is a squashed version of the previous commits with any experiments removed. This should solve #1922 as well as all 3 cases mentioned by @rht above on Windows. Prior to this commit ipfs add always acted as if -w was invoked, that is not to say that the actual wrap flag was set to true but that ipfs had been adding the directory contents then wrapping everything into its own node as well (regardless of if -w was specified or not).

While this commit fixes that issue as well, I am unsure as to why this behavior was happening to begin with or why this solves it. Outputting the paths and filenames at every stage during add on the same directory on 2 different platforms resulted in the same output excluding an additional erroneous directory hash on Windows which held the correct contents as well as some malformed duplicates (for example "

/level 2 A\level 3 file A.txt") .

Since I cannot step through the code on this platform currently, I made the assumption that the filenames need to use forward slashes to be parsed correctly while the filepaths needed to use the platform specific delimiters for reading etc. but I couldn't actually confirm that this was the reason.

Given the uncertainty I'll need someone who knows better than me to scrutinize the change as well as the ci. It builds and works on my machine as well as inside of a VM with the test directory and real data. After merging with the latest origin it builds and runs the same on FreeBSD as before the merge. The circleci failures seem to all be flagged with "# TODO known breakage". pinging @jbenet @rht

[jbenet]

@whyrusleeping want to CR this one?

[whyrusleeping]

LGTM

[djdv]

@whyrusleeping If there are additional changes to be made I'll make sure to use the full name for filepath rather than "fp" in the next commit, there doesn't seem to be any name conflict to warrant the shortname except maybe one variable "filePath" on 2 lines.

@rht I see, are file paths expected to be standardized and stored as forward slash paths, or could/should this be changed to use filepath for parsing rather than the strings package? strings is used again for the path on line 101 as well for removing a link.

[rht]

If the fp naming is to be purged, then there are 2 places where it is used: thirdparty/tar/extractor.go, commands/file/serialfile.go. (@whyrusleeping on code cleanup in general (out of scope of this PR), there are several inconsistencies in package import naming as well, e.g. mdag/merkledag/dag for github.com/ipfs/go-ipfs/merkledag)

I initially thought standardizing toward forward slash is preferable (initial rationale: windows path appears only in ipfs add, and so can be standardized locally), but:

1. '/' is strictly required only in the http handler part of the code.
2. I recalled this was the initial choice in golang until path/filepath was written (e.g. there was a redundant function in os.Stat's (basename) where filepath.Base could have been used).
3. strings.Split with hard-coded '/' is less 'semantic' than https://golang.org/pkg/path/filepath/#SplitList.
4. the lib needs to be modular, e.g. dagutils are expected to be used various situations and so shouldn't assume the path separator.

+1 for filepath.SplitList instead of strings.Split.*/.

[rht]

There are too many places in the code where hardcoded '/' is assumed, however.

I think a sufficient fix just for ipfs add is to standardize commands/cli/parse.go, leaving aside the full cross-platform path (e.g. in ipfs resolve, parts of the lib, etc) issue for now.

[djdv]

@whyrusleeping I can cleanup the fp names in the suggested files as a separate commit inside this PR if that's desired or leave it be if there's an ongoing cleanup initiative.

@rht In relation to the slash issue, translating the path from back slashes to forward slashes in the call to NewSerialFile everywhere except in serialfile.go results in the "implied and incorrect -w" issue mentioned before where it's inserting duplicates in the root (the root that was not asked for), I'm still actually unsure as to why that particular issue is happening, without a way to step through it's a little hard to diagnose so I could be missing something obvious. I tried taking the input in parse.go and converting it to forward slashes before it hits anything else after it gets Clean()'d. It may have something to do with names returned by an os.FileInfo somewhere else but I'm not actually sure.

The attached image may better explain the -w thing I'm talking about.

[rht]

Understood the -w case.

Just to make sure:

1. I tried taking the input in parse.go and converting it to forward slashes before it hits anything else after it gets Clean()'d.

2. Inserting path = filepath.ToSlash(path) before https://github.com/ipfs/go-ipfs/blob/master/core/commands/add.go#L365

Both fix the fullpath, '../', file duplication bugs, but not the -w bug?

[djdv]

Partially. When those fixes are applied we generate and get the correct hash but we also get an additional unrequested hash, the files and names inside the target hash are fine but not in the unwanted hash. In addition when -w is passed we get the same outcome.

To put that another way when those 2 things are applied we get QmPbRRZjtJgfX25tJzYUPceQXxUX9tWb5eRqFSMChLUKKy which is the target as well as QmVu8MgHzuF7asFYx8VxsXXt1QxPS3GYu3QBx17pWFg9Mr which is unwanted AND contain malformed filenames such as "/level 2 A\level 3 file A.txt". When -w is supplied the target hashes include the previous target plus Qme1Pd6PjA6Gd7278LrXwiDL9NUmNNSQ7KHnAebttkHL5d which is the intended -w target.

The current PR commit will get the intended result in all cases (full, relative, ../relative, -w or not), it keeps the native path all the way up to here where it gets translated: https://github.com/ipfs/go-ipfs/pull/1933/files#diff-030475adc730fdd79d7d8c459a8a7ccfR72 The ToSlash is the thing that resolves the -w bug particularly.

Sorry for the confusion, it's hard to explain when I don't know the root cause of it myself. There may be another method that solves this in other places that may or maynot be preferable. My proposal seems like a hacky fix at the end of the code path rather than a proper solution in the middle.

[rht]

Confirming, so at least '/'-standardizing the commands/cli/parse.go does fix all the naming issues. Worth checking is the !wrap condition to get the RootNode in https://github.com/ipfs/go-ipfs/blob/master/core/commands/add.go#L342.

It would better to investigate the underlying cause so that the code can be further cleaned/clarified, but since the PR already works, LGTM.

[djdv]

Agreed, it seems like the best course of action would be to implement this now so long as it appears fine to everyone and does not cause conflicts on other platforms/tests. This would be good for Windows users in the short term, in the future someone else or I can reassess this and look for the underlying issue if it doesn't get resolved naturally through refactoring other parts of the project.

As for code cleanup I'll leave that alone since it's not introduced here and would be better as a separate broader request on its own.

@jbenet Ready to be merged if no objections.

[jbenet]

I'm not sure about adding this if we're not sure the pathing is working exactly right. There may be other problems lurking that our tests arent catching.

@gatesvp thoughts?

---

re standardizing, it should be / (forward slash) everywhere except when accepting / reading paths on windows. meaning that the canonical path separator for IPFS is / globally and should be translated only when strictly necessary for compatibility. Should still be displayed as / etc.

[gatesvp]

In general, it looks like windows is rather accepting of both front and back slashes.

So if you output the stuff with front slashes, someone should still be able to pipe that through to the next commend anyways. That stated, the spaces will screw things up, regardless of slashing, so I don't think this "new" output is any better or worse.

From an output perspective, I'm totally fine with this.

From an input perspective, my only real caveat here is UNC paths. These are paths for "shared folders" typically denoted with a leading \\. So you would see something like \\file_server\logs\web_server01 Even more examples here. (https://en.wikipedia.org/wiki/Path_(computing)#Representations_of_paths_by_operating_system_and_shell)

It looks like filePath has a specific Windows implementation for handling UNC. So as long as we're correctly leveraging their library here, we should be good (https://golang.org/src/path/filepath/path_windows.go)

[rht]

It appears that for shared folders, '\' will be converted to '//' (https://golang.org/src/path/filepath/path.go#L161). @djdv if you can check this case.

the canonical path separator for IPFS is / globally and should be translated only when strictly necessary for compatibility

(goland's stance used to be this) To explain, this PR isn't entirely arbitrary (as in how Kepler/Balmer discovered things). It works by intercepting all the filenames that are added into the dagnode (because all files have to first pass through the File interface) and converting path separators to /'s.

[djdv]

@jbenet

re standardizing, it should be / (forward slash) everywhere except when accepting / reading paths on windows. meaning that the canonical path separator for IPFS is / globally and should be translated only when strictly necessary for compatibility. Should still be displayed as / etc.

Understood. Undoing the filepaths that were added here while keeping the ToSlash where it is will result in all cases failing due to the implied -w and malformed names in those directories, except for when adding a relative path while in the directory it resides in. If we can track down why that is happening then this may be resolved since the correct hashes are being produced prior to the final hash (see above screenshot). Example commands and their final hash output: Expected hash for all is QmPbRRZjtJgfX25tJzYUPceQXxUX9tWb5eRqFSMChLUKKy

J:\demo\ipfs>ipfs add -r "J:\demo\ipfs\windows directory" QmUfA8HL5h62Giv3uruywzuAxZk1UMufWGihkgskKHGpXX

J:\demo\ipfs>ipfs add -r "windows directory" QmPbRRZjtJgfX25tJzYUPceQXxUX9tWb5eRqFSMChLUKKy ^Correct

J:\demo\ipfs\windows directory>ipfs add -r "..\windows directory" QmeEW3XxDQb4Lss2Vk3H4hdvdhPWi3EZzy38SMGBM1eyDt

J:\demo>ipfs add -r "ipfs\windows directory" QmPH5aShaFkEuQzqZEJQNmz9qwxsRax55LLAatKY4ZUN4A

Tracking down why this duplication is happening without a means to step through the running process is proving to be difficult for me. @rht's information has been critical in getting this far.

@gatesvp One issue I see with UNC paths on the input side is their use of \\ which will result in a single \, so no matter what a UNC path will have to be entered like this `ipfs add -r "\\\\HOST\\sharename" or this ipfs add -r "//HOST/sharename" instead of the native \\HOST\sharename, the latter of the two is problematic since it will be converted to /HOST/sharename when cleaned by path in parse.go using filepath.Clean() gives the intended string but then we'd have to convert it back to forward slashes, so something like filepath.ToSlash(filepath.Clean(input)). I need to experiment with these and how they interact with Go, path, filepath, reading, etc.. This seems like something that may need a test case as well, something like: create a structure, add that structure as a share, add the share to ipfs via its UNC path, compare output and then unwind all that.

@rht UNC paths seem to convert to this when the double slash is accounted for: filepath.ToSlash("\\\\HOST\\windows directory") "//HOST/windows directory" Which seems to work inside of Windows for reading, doing a dir on that output will return the contents.

[djdv]

Latest commit standardizes input paths to forward slashes instead of using platform native paths, this fixes all above issues on systems that support using a forward slash as a delimiter, this is the opposite of the previous commit where we were translating the filenames at the latest possible moment instead of the earliest.

UNC paths were not considered in this commit, I will be going over those next.

[gatesvp]

Personal opinion here on UNC paths, but I would consider these a "nice to have" rather than a total requirement. I mean, the premise of UNC is to provide a consistent way to address content/objects on the network, which is also kind of the point of IPFS :)

Just printing a "UNC is not supported" for now may be sufficient.

[djdv]

@gatesvp That makes sense. If people need to add a share now, Windows has ways of resolving these paths into drive letters itself using net use *driveletter*: \\HOST\sharename which would work fine with ipfs add -r *driveletter*:\.

Does the latest commit seem adequate for detecting these? We use filepath to clean the input which means on Windows all / characters will be translated to \, so the check against \\ should always work and not conflict with // input on other platforms if supplied. After the check we convert all delimiters to / and proceed as normal. --Edit-- Perhaps I should change the "\\\\" to a literal \\

Just for reference, the initial issues with UNC is when it makes the call to the API, somewhere along the line the leading slashes get turned into a single slash. Somewhere in the command context maybe, I wasn't sure yet.

J:\demo\ipfs>ipfs add -r "\\\\HOST\\windows directory"
IPFS_PATH C:\Users\Dominic\.ipfs
no context set in request
Calling pre-command hooks...
Total size of file being added: 0
Executing command via API
Error: Post http://127.0.0.1:5001/api/v0/add?encoding=json&progress=true&r=true&stream-channels=true
: open /HOST/windows directory/level 1 A: The system cannot find the path specified.

[jbenet]

without much knowledge of windows, this roughly LGTM. however this breaks tests:

• https://travis-ci.org/ipfs/go-ipfs/builds/90912229

need to fix those. and we should setup appveyor with windows testing

[rht]

IMO adding the content of UNC paths should work. This is different from encoding UNC/symlink/namedpipe object itself into merkledag.

I think the previous version of the PR (where filepath.ToSlash is in serialfile.go instead) already accepts UNC path (since only the filenames were /-converted).

[djdv]

@rht You're right, the filepath.ToSlash in serialfile does work with UNC, using that over path leaves extra slashes intact while any calls to path itself will strip them out, I forgot about that inherent behavior. This was the cause of the error before where \\HOST\share and //HOST/share would be converted to an invalid /HOST/share on any path calls instead of the valid //HOST/share.

@jbenet Since add is now working with UNC paths I've removed the faulty check for them that was causing the issue before (forgot to check length before use, oops!). I need confirmation on the CI results again though, are all of the failures listed known to be problems or did I introduce these?

[jbenet]

Mac tests seem to be repo auto gc related. @rht and @whyrusleeping could you take a look? Is this a real problem? (Prob not from this PR but might be a real issue)

Anyone with appveyor (or whatever windows CI systems exist) XP want to help me manage it? I just don't have the bandwidth atm. On Mon, Nov 16, 2015 at 15:31 Dominic Della Valle notifications@github.com wrote:

@rht https://github.com/rht You're right, the filepath.ToSlash in serialfile does work with UNC, using that over path leaves extra slashes intact while any calls to path itself will strip them out, I forgot about that inherent behavior. This was the cause of the error before where \HOST\share and //HOST/share would be converted to an invalid /HOST/share on any path calls instead of the valid //HOST/share.

@jbenet https://github.com/jbenet Since add is now working with UNC paths I've removed the faulty check for them that was causing the issue before (forgot to check length before use, oops!). I need confirmation on the CI results again though, are all of the failures listed known to be problems or did I introduce these?

— Reply to this email directly or view it on GitHub https://github.com/ipfs/go-ipfs/pull/1933#issuecomment-157208638.

[rht]

Likely daemon startup hiccup? (and hence affects the fixed period gc). If the sleep time is increased to 50ms or 60ms it is less likely to happen.

If the test runs again there is odds the err will disappear.

[rht]

I ran the test 3-4 times through #1975. All tests passed except for travis-ci os x sharness test, which always hanged, not necessarily at the auto-gc. ci tests always passed.

[djdv]

@rht @jbenet Given issue #1960 and the fact that only OS X tests fail here, is it safe to assume this PR does not induce any failures? Should this wait until there are some Windows CI tests set up?

[jbenet]

windows CI would be very useful to test this against

[rht]

There will be lots of goroutine errs as has been pointed out. It wouldn't be sensible to make sure all those errs get fixed within a PR.

[rht]

The os x hiccup is gone now. Test of branch that contains this pr's commit passes https://travis-ci.org/ipfs/go-ipfs/builds/92252256

[djdv]

@rht

The os x hiccup is gone now. Test of branch that contains this pr's commit passes

Good to hear, thanks for running those! With the CI now passing is there anything left to do or test for in the scope of this PR? Anything related to Windows outside of this should probably go into its own issue and then be referenced here: #959, I'd expect that issue to grow a bit when a Windows CI is added but I don't know if it's necessary to wait for that to happen first.

Paging @jbenet for direction.

[whyrusleeping]

one slight concern over ToSlash but otherwise looks good to me.

[djdv]

@whyrusleeping Both / and \ are forbidden for use in filenames on Windows although I think NTFS technically allows for them, but files with those in the name are treated as malformed under Windows so they are not to be used and are not guaranteed to work either. On Windows ToSlash will convert something like directory/file with slash in name 1\2.txt to directory/file with slash in name 1/2.txt but that only happens on platforms where the path separator is \, on systems that use anything else this should not happen.

Here's a screenshot of the different behavior on the playground and a Windows machine for the same input. Note: The filename used as an example here would be erroneous and forbidden on Windows either way, so input like this should never actually be expected on a Windows system.

[whyrusleeping]

@djdv okay, this PR is a strict improvement on what already exists, so i'm fine with it.

in related news, i'm still trying to figure out whose great idea led to this: https://ipfs.pics/QmbiK6ThQeqFXHeV8gP7NYmSVwyxw9rRXKbEU8Y27qLMrQ

[rht]

But it looks like a unicode mimic.

(somewhat related to the related news, iirc minecraft is in need of a proper package manager?)

[jbenet]

I reran OSX and it's still hanging. @rht do we need to rebase or something?