add: [Windows] Drive root parse fix

[djdv]

Previously when adding the root of a drive on Windows (either directly through X:\ or via . while in X:), ipfs would prefix everything and not handle directories properly in a manner simillar to this issue:#1922

This does not happen with paths or files underneath the root, it's just when adding the root itself. 👻
This can be fixed by preserving the full path X:\ instead of using X: as a base when specifying the root of a drive only. As an aside it would not be valid to use / instead, since / is relative to the drive in your working directory on Windows, not an absolute path. Using / while adding Y:\ from X:\ would either give you N contents from X: or panic if X: contains less items in the root than Y: does. Preserving the full path doesn't have this issue.

I make sure not to act on paths starting with / and check for a backslash, to my knowledge a valid input path like that should only show up on Windows.
Using long path prefix form \\?\x: will still give the incorrect behavior, I don't think it's necessary to check for this but it could be caught in the same way, then converted from the long form to the short form X:\

Any issues with this and should long form be handled too?

[whyrusleeping]

Can some other windows user verify and test this? @slothbag ?

[djdv]

Changed comment, "drive letter" -> "drive path".

[Kubuxu]

@mateon1 could you give it a test.

[mateon1]

Sorry for the late response. I gave this PR a test. It doesn't seem to fix the problem, the ipfs add output is the same for both this PR and the parent commit.
Here are my results
master.txt is 68d8a29 and pr.txt is f2b271c

[djdv]

Thanks @mateon1
X: isn't handled by this yet, only X:\ or X:/. When using . in X:\ it should have worked as expected though, I'm not actually sure why that didn't catch since . should resolve to X:\ with os.Getwd(), the same test works on my actual system and I just tried it inside a vm with a fresh Windows install. Both give these results (note the long form still giving bad results since it's unchecked for now).

I also rebuilt with Go 1.7.3 and still get the same. I'm not sure where the discrepancy could be, any idea?

I'm also not sure how X: on its own should be handled, it could be translated to X:\ but I think it may be best to return an invalid path error, with a message explaining to add a \ to the input if you intend to add an entire root. The only use of X: by itself that I know of is to change the active drive in the command interpreter, so it shouldn't be a valid path target on its own. X:file.ext(no slash but has file or path) however is a valid target, it points to the file relative to the current working directory (not necessarily the root) of drive X: which will add correctly even recursively.

I'd need some guidance on what the behavior should be for both X: and the long form \\?\x:, convert them, return an error, or leave as is (bad behavior).

Sorry for the verbosity but I have to be pedantic with these DOS path semantics and behaviors.

[djdv]

Alright, so I wrote something here that should properly handle all cases and hopefully not break anything.
X: and \\?\X: will return an error asking you to append a \ to your input.
X:\ and \\?\X:\ should work as expected
X:. and X:\.\somepath should also work as expected, the relative paths being resolved into absolute paths and then handled like normal.
. while in a drive root should also work as expected.

If anyone has suggestions for improvement, I'd like to hear them. I'm also not sure if the comments are overkill, but these are weird Windows specific special cases.

Everything works as normal in my case but I'd like others to test this if they can. If everything looks good I'll squash these and rebase on master.

Pinging @slothbag, @mateon1, and any other Windows users that see this.

Obligatory shell shot.

[slothbag]

Unfortunately I don't have the time to test this right now, but looking at the screenshot that pretty much covers the scenarios I would run.. It looks pretty good to me.

[whyrusleeping]

Lets make sure to consider the effects these changes will have on linux and OSX systems. (among others)

[whyrusleeping]

Hrm, we need to special case this to only run on windows. In linux, X:. is a perfectly valid filename

[djdv]

@whyrusleeping
You think it would be best to check the OS at runtime for this? I was trying to figure out a way to avoid it by using Go build constraints so that special case checking was only built in on platforms that need it, but I'm not super familiar with them for this kind of thing.

[Kubuxu]

See https://github.com/ipfs/go-ipfs/blob/9094cc8f4297b9365a3d3260f839724b135094d0/cmd/ipfs/main.go#L607-L613

[djdv]

Modified the comments to have consistent quotations and changed some wording, also shortened the error message. Unless anyone has any issues with the logic or the wording, this should be good to go.

[djdv]

@whyrusleeping o/
Any chance I could have this looked at? I was hoping to have this merged before any filestore changes, I feel like adding entire drives+no-copy will go hand in hand.

[whyrusleeping]

One request, then you'll need to rebase this to fix the merge conflicts.

After that this LGTM

[whyrusleeping]

can we extract this out into a windowsParseFile sort of thing?

[djdv]

Like this?

[whyrusleeping]

@djdv Yeah, though now tests are failing strangely... It looks like an issue that we've already fixed.

cc @Kubuxu, could you take a look?

[whyrusleeping]

Going to assume the travis failure is an issue with caching old code.