Deprecated filepath.HasPrefix

[ankitm123]

While trying to solve the issue #290, I took a look at the dep source code. I see multiple instances of filepath.HasPrefix. The function is deprecated, so should we not remove/replace it from the source code? Take a look at this link:
https://golang.org/pkg/path/filepath/#HasPrefix

Ankit

[sdboyer]

@adg could you speak to this one?

[adg]

@sdboyer I don't remember this being deprecated, but I can see why it is. The places where we use it were once a simple strings.HasPrefix (which is all filepath.HasPrefix does).

We should probably replace such occurrences with our own function that uses filepath.Abs to canonicalize the paths before comparing them.

[sdboyer]

It's just strings.HasPrefix() on unix flavors, but there's more to it on windows. But it's that windows impl that seems - I guess? - to be more incorrect.

Either way, yeah, we should write our own implementation.

[ankitm123]

@adg, @sdboyer, I can work on it this weekend, if it is ok.

[adg]

I'd be very happy if you did! Thank you.

…

On 9 March 2017 at 15:08, ankitm123 ***@***.***> wrote: @adg <https://github.com/adg>, @sdboyer <https://github.com/sdboyer>, I can work on it this weekend, if it is ok. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#296 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIDildDSds8wTdzxLUfzydN11cPVLJsfks5rj3s-gaJpZM4MVkXI> .

[zacps]

@ankitm123 Are you still working on this? I might take a look at it if you don't have time.

[ankitm123]

@zacps, I should have it done within 2 days.

[mattn]

any update on this?

[zacps]

I'll do it tomorrow unless @ankitm123 has a pull in.

[ankitm123]

@sdboyer, @zacps Sorry for the delay, I am in the process of joining a new job and relocating!
Please take a look at this piece of code, I will polish it a bit over the weekend, and if it fixes the issue, then I will submit a PR.
https://play.golang.org/p/EoAliG0rNG

I will use the os.pathseparator in the hasSuffix function, to make it cross platform.

[sdboyer]

Honestly, I'm not sure if that works or not, because I don't know what "not respecting path boundaries" means in those comments on the windows filepath.HasPrefix() implementation.

[zacps]

Should we expand special paths/environment variables/UNC paths? I think it makes sense for this:
~/path
to be the same as:
/home/USERNAME/path
and:
C:\Users\username\Documents
to be the same as:
\\HOSTNAME\Users\USERNAME\Documents
Do the current path/filepath tools handle this?

---

You also shouldn't use strings.compare() just use ==.

[ankitm123]

@sdboyer, I think what they mean by path boundaries is just path separator (/ in linux and \ in windows)
For instance:
filepath.HasPrefix("/home/user4/some-file","/home/user") returns true, where it should return false, because the real prefix that should get matched is /home/user4, and not /home/user. But I will investigate it a bit further.
The issues with HasPrefix are outlined here. I got this link from this discussion.

@zacps, Thanks for the suggestion. Using strings.Compare is not the ideal way to compare. I will change it to ==. Also, I think we can check is the filepath has ~ (say ~/file), and then use the os.environ function to replace it with /home/. I will include the changes, and we can see how it works. Thanks!
Update: Also, filepath.HasPrefix is just strings.HasPrefix, so it does not handle ~ and HOSTNAME

[sdboyer]

OHH, I see. Yeah, I've been dealing with respecting path element boundaries for some time now - it just came up in #271, but there's a handful of funcs in gps that deal with it, too. That part definitely makes sense.

Looking at the links, and recalling from the earlier issue that introduced this, here's some thoughts:

1. Respect path boundaries correctly, of course.
2. We definitely care about case sensitivity. As far as I can tell, the only way to check that is to actually test it on the fly. It seems like it might be possible to do this without creating a file - e.g., compare inodes (or the windows/fs equivalent) of the cwd's tail element with a case-variant version of it to see if they're the same.
3. While each path element could be a different mountpoint with different case sensitivity properties, just doing the tail element checking should be sufficient for our needs here.
4. Independent of the path portions of the check, Windows will - I think? - unconditionally need to do a case-insensitive comparison on the volume name (C:\ == c:\).

There's also this unexported func in stdlib that might provide some guidance.

[zacps]

@sdboyer Looks like filepath.VolumeName() handles UNC paths properly (as well as the case check). Why would we need to test case sensitivity on the fly? Can't we just use build flags for different behaviours based on OS?

• Windows: Insensitive
• Linux: Sensitive
• OSX: Insensitive
• Other BSDs: ??

[ankitm123]

@zacps,Doesn't filepath.VolumeName() only work for windows system? It returns empty strings for other OS, isn't it?

[zacps]

@ankitm123 Yeah, which is what we want right? We can compare the value of this to check the volume. On windows it'll work properly with UNC paths and drive names and on other systems, where those aren't relevant it'll always succeed ("" == "").

[sdboyer]

@zacps that was what I'd figured about case sensitivity until recently, as well. But it's a filesystem property, not an OS one. See rsc's comment on the CL @ankitm123 linked to.

[zacps]

@rsc Yeah ok it looks like it's possible to change it on windows, I don't think we can just write a file somewhere though, you wouldn't expect a path comparison function to write to the filesystem. We might not even have permission to.

Are any windows systems case sensitive by default? It looked like it was an obscure config option. Just assuming per OS is far from perfect but I think it's definitely better than writing to the filesystem.

[sdboyer]

I don't think we can just write a file somewhere though, you wouldn't expect a path comparison function to write to the filesystem. We might not even have permission to.

Yep. I think I abbreviated my first thought on this too much. Allow me to expand.

Given that:

1. We can get cwd (if os.Getwd() fails, dep must also fail)
2. I think all our uses for this func would inevitably either be cwd or some subpath of it
3. dep will also eventually fail if cwd isn't writeable, because that's where it has to write vendor

If we had to write a file somewhere, then the tail dir in cwd has to be writeable anyway, so that's the place to do it.

But, I also think we could do one better and avoid writing altogether (as I agree, it would be quite unexpected to do that in a func like this).

Say that cwd is /home/user/go/src/foo.

1. os.Stat("/home/user/go/src/foo"), get the inode (or windows fs equivalent). My quick testing suggests it's os.FileInfo.Sys().(*syscall.Stat_t).Ino on OSX/HFS and linux/zfs.
2. os.Stat("/home/user/go/src/Foo").
   • If it doesn't exist, it's definitely a case-sensitive filesystem.
   • If it does exist, then we compare the inodes. If they're the same, it's case insensitive. If they're not, it's case sensitive.
   • If we can't figure out the inode/whatever, then fall back to assuming that having a match on both indicates case insensitivity. That seems like it would be the overwhelming case.

Have I missed something?

[zacps]

Nope, I think that looks good to me. So checklist is:

• Compares volumes properly (filepath.VolumeName())
• Respects path boundaries
• Detects and applies correct case rules (Via inodes as above)

[sdboyer]

We're running into issues with this over in #379, as well. It seems like we have a TODO list now - I'm hoping someone has the time to tackle an implementation?

[sdboyer]

(maybe we'll just convert that issue to be the PR that fixes this)

[ankitm123]

I will take a look at it this weekend.

[sdboyer]

@ankitm123 OK - @Civil also indicated they might try over the weekend. Neither of you fully committed, but just wanted to give you both a heads-up.

[mem]

I would appreciate it if you guys can take a look at the PR and provide feedback. This was done very very quickly. At least the tests need fixing.

[ryboe]

dep is using the staticcheck static analyzer now. staticcheck is currently set to -ignore the deprecation warnings about filepath.HasPrefix.

# Ignore the deprecation warning about filepath.HasPrefix (SA1019). This flag
# can be removed when issue #296 is resolved.
- staticcheck -ignore='github.com/golang/dep/context.go:SA1019 github.com/golang/dep/cmd/dep/init.go:SA1019' $PKGS

If you're submitting a PR for this, would you be so kind as to delete that flag (and the comment above) from the .travis.yml file? 🙏

[sdboyer]

#395 is merged, so this should now be fixed. Closing this out 😄