[WIP]: internal/fs: add EquivalentPaths function #940

ibrasho · 2017-08-03T18:50:28Z

What does this do / why do we need it?

Add a new method to internal/fs to check for path equivalence.

What should your reviewer look out for in this PR?

Does this work as expected?

Which issue(s) does this PR fix?

fixes #772
fixes #805

ibrasho · 2017-08-04T11:48:17Z

@sdboyer I believe I found a bug in fs.HasFilepathPrefix...

I wasted a few hours debugging why the tests for EqualPaths fails on Mac on Travis and it seems that HasFilepathPrefix has a bug on case-insensitive filesystems on *nix.

This is where it happens:

	// d,p are initialized with "" on *nix and volume name on Windows
	d := vnPath
	p := vnPrefix

The next for loop always assume that the filesystem is case-sensitive since isCaseSensitiveFilesystem returns false when the passed dir doesn't exist.

On *nix, d and p are defaulting to "" instead of "/" and joining them with additional directory names results in a relative path. This happens because filepath.Join("", "dirName") returns "dirName".

	for i := range prefixes {
		// need to test each component of the path for
		// case-sensitiveness because on Unix we could have
		// something like ext4 filesystem mounted on FAT
		// mountpoint, mounted on ext4 filesystem, i.e. the
		// problematic filesystem is not the last one.
		if isCaseSensitiveFilesystem(filepath.Join(d, dirs[i])) {
			d = filepath.Join(d, dirs[i])
			p = filepath.Join(p, prefixes[i])
		} else {
			d = filepath.Join(d, strings.ToLower(dirs[i]))
			p = filepath.Join(p, strings.ToLower(prefixes[i]))
		}

     // ...
}

isCaseSensitiveFilesystem has not tests, and it's quite hard to test honestly... And the tests for HasFilepathPrefix don't cover this case.

I've pushed a commit with my proposed fix, which is to update both isCaseSensitiveFilesystem and HasFilepathPrefix.

sdboyer · 2017-08-05T04:35:45Z

context_test.go

@@ -341,49 +334,49 @@ func TestDetectProjectGOPATH(t *testing.T) {
 		{
 			name:         "project-with-no-AbsRoot",
 			root:         "",
-			resolvedRoot: filepath.Join(ctx.GOPATHs[0], "src", "real", "path"),
+			resolvedRoot: filepath.Join(ctx.GOPATHs[0], "src/real/path"),


why these changes? AFAIK this isn't safe for windows

@sdboyer you're right, it isn't.

I'm under the impression that filepath.Join will call filepath.Clean which calls filepath.FromSlash on the passed path. Am I missing something else?

Anyhow, this is a purposeless change and I can revert it. 😁

cool, ty; yes, it does call filepath.Clean(), but that func doesn't do separator transformations.

Just trying to clarify, shouldn't this line do that?

huh. guess i'm wrong on that!

sdboyer · 2017-08-06T18:22:38Z

internal/fs/fs.go

@@ -100,6 +102,53 @@ func HasFilepathPrefix(path, prefix string) bool {
 	return true
 }

+// EqualPaths compares the paths passed to check if the are equivalent.
+// It respects the case-sensitivity of the underlaying filesysyems.
+func EqualPaths(p1, p2 string) (bool, error) {


EquivalentPaths would probably be a better name here than EqualPaths, as it emphasizes that we are testing for logically equivalent paths rather than byte-identical ones (which is the whole point!)

sdboyer

just two little comment nits, then i think this is 👍 (feel free to merge once you fix those)

sdboyer · 2017-08-14T03:25:18Z

internal/fs/fs.go

@@ -98,6 +102,53 @@ func HasFilepathPrefix(path, prefix string) bool {
 	return true
 }

+// EquivalentPaths compares the paths passed to check if the are equivalent.


nit: s/if the are/if they are/

sdboyer · 2017-08-14T04:05:44Z

internal/fs/fs.go

@@ -98,6 +102,53 @@ func HasFilepathPrefix(path, prefix string) bool {
 	return true
 }

+// EquivalentPaths compares the paths passed to check if the are equivalent.
+// It respects the case-sensitivity of the underlaying filesysyems.


nit: s/underlaying/underlying/

sdboyer · 2017-08-17T05:50:51Z

well that's some catastrophic failure on linux :(

ibrasho · 2017-08-21T19:09:30Z

I figured out what's happening finally. 😓
A non-existing file error wasn't being handled properly.

Signed-off-by: Ibrahim AshShohail <[email protected]>

isCaseSensitiveFilesystem returns true when os.Stat fails on the dir passed. This caused HasFilepathPrefix to always treat *nix systems as case-sensitive, since it passed a relative path (missing the root) to isCaseSensitiveFilesystem. This commit updates isCaseSensitiveFilesystem to return an error in addtion to the boolean check. Also, HasFilepathPrefix now passes absolute paths to isCaseSensitiveFilesystem. Signed-off-by: Ibrahim AshShohail <[email protected]>

Signed-off-by: Ibrahim AshShohail <[email protected]>

ibrasho · 2017-09-02T07:01:22Z

@sdboyer Should we merge this? 😀

sdboyer

yes, i think this is probably good to go. (sorry, busy week). just one comment - up to you if you want to address

sdboyer · 2017-09-02T07:18:02Z

context_test.go

-		{th.Path("go"), filepath.Join(th.Path("go"), "src/github.com/username/package"), false},
-		{th.Path("gotwo"), filepath.Join(th.Path("gotwo"), "src/github.com/username/package"), false},
-		{"", filepath.Join(th.Path("."), "code/src/github.com/username/package"), true},
+		{th.Path("go"), th.Path(filepath.Join("go", "src", "github.com", "username", "package")), false},


i think that after looking through, you turned out to be totally right on this - it'll convert to windows line endings automatically. i'm fine with changing it back, though it's not really necessary either

Not worth changing, to be honest. 😛

sudo-suhas · 2017-09-04T05:56:58Z

@ibrasho Could you please tell me why you are not using os.SameFile? I ran the tests(TestEquivalentPaths) successfully with this modification on windows and linux:

func EquivalentPaths(p1, p2 string) (bool, error) {
    p1 = filepath.Clean(p1)
    p2 = filepath.Clean(p2)

    fi1, err := os.Stat(p1)
    if err != nil {
        return false, errors.Wrapf(err, "could not check for path equivalence")
    }
    fi2, err := os.Stat(p2)
    if err != nil {
        return false, errors.Wrapf(err, "could not check for path equivalence")
    }

    return os.SameFile(fi1, fi2), nil
}

sdboyer · 2017-09-05T02:53:34Z

Could you please tell me why you are not using os.SameFile?

i'd swear we have a good reason for this, because we certainly knew that function existed beforehand. i can't remember that reason now, though 😢

sudo-suhas · 2017-09-05T03:14:03Z

Hmm.. What do you suggest we do? I can make a PR with the changes above. We can see if everything works as expected.

sdboyer · 2017-09-05T03:23:05Z

We can see if everything works as expected.

no, we need to trace back over the lines of thinking that got us to here. hasty PRs in areas related to casing just lead to more headaches; "see if everything works as expected" is not something we can do, as there is no well-formed definition of "everything," let alone one codified into tests.

sudo-suhas · 2017-09-05T04:35:25Z

Okay. At the very least, we should identify and add a test which fails for os.Samefile but not the current implementation.

ibrasho · 2017-09-05T13:04:08Z

@sudo-suhas As Sam has said, we are trying to handle something that os.SameFile doesn't.

If I remember correctly, it has to do with nested filesystems that handle case-sensitivity differently. But I can't remember the exact case. 😕

* internal/fs: add EquivalentPaths * internal/fs: fix isCaseSensitiveFilesystem bug isCaseSensitiveFilesystem returns true when os.Stat fails on the dir passed. This caused HasFilepathPrefix to always treat *nix systems as case-sensitive, since it passed a relative path (missing the root) to isCaseSensitiveFilesystem. This commit updates isCaseSensitiveFilesystem to return an error in addtion to the boolean check. Also, HasFilepathPrefix now passes absolute paths to isCaseSensitiveFilesystem. Signed-off-by: Ibrahim AshShohail <[email protected]>

ibrasho requested a review from sdboyer August 3, 2017 18:50

googlebot added the cla: yes label Aug 3, 2017

ibrasho changed the title ~~internal/fs: add EqualPaths()~~ WIP: internal/fs: add EqualPaths() Aug 3, 2017

ibrasho changed the title ~~WIP: internal/fs: add EqualPaths()~~ [WIP]: internal/fs: add EqualPaths() Aug 3, 2017

ibrasho force-pushed the add-EqualPaths-to-fs branch 3 times, most recently from 22f4852 to fcfaa20 Compare August 3, 2017 20:51

ibrasho mentioned this pull request Aug 4, 2017

ctx.DetectProjectGOPATH: both C:\Work\src\github.com\F21\some_project are in the same GOPATH C:\Work\src\github.com\F21\some_project with Git Bash #772

Closed

ibrasho force-pushed the add-EqualPaths-to-fs branch from fcfaa20 to 0ec076d Compare August 4, 2017 07:24

sdboyer suggested changes Aug 5, 2017

View reviewed changes

sdboyer suggested changes Aug 6, 2017

View reviewed changes

ibrasho force-pushed the add-EqualPaths-to-fs branch from 4c1cbbb to b60c5a9 Compare August 9, 2017 04:14

ibrasho changed the title ~~[WIP]: internal/fs: add EqualPaths()~~ [WIP]: internal/fs: add EquivalentPaths function Aug 9, 2017

sdboyer suggested changes Aug 14, 2017

View reviewed changes

klauern mentioned this pull request Aug 19, 2017

Cannot use dep ensure on Windows #805

Closed

ibrasho requested a review from carolynvs as a code owner August 21, 2017 19:06

ibrasho force-pushed the add-EqualPaths-to-fs branch from 7d6b41a to a64d6b7 Compare August 21, 2017 19:11

ibrasho added 6 commits August 22, 2017 20:16

internal/fs: add EqualPaths()

546959f

Signed-off-by: Ibrahim AshShohail <[email protected]>

fix tests

d94a0f3

Signed-off-by: Ibrahim AshShohail <[email protected]>

internal/fs: rename EqualPaths to EquivalentPaths

a0e33dd

Signed-off-by: Ibrahim AshShohail <[email protected]>

fix typos

d93ac98

Signed-off-by: Ibrahim AshShohail <[email protected]>

internal/fs: fix bug in isCaseSensitiveFilesystem

67f83fc

Signed-off-by: Ibrahim AshShohail <[email protected]>

ibrasho force-pushed the add-EqualPaths-to-fs branch from a64d6b7 to 67f83fc Compare August 22, 2017 17:17

sdboyer approved these changes Sep 2, 2017

View reviewed changes

ibrasho merged commit 3b01418 into golang:master Sep 2, 2017

ibrasho mentioned this pull request Sep 2, 2017

check Gopkg filenames case on case insensitive systems #1114

Merged

ibrasho deleted the add-EqualPaths-to-fs branch November 29, 2017 06:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP]: internal/fs: add EquivalentPaths function #940

[WIP]: internal/fs: add EquivalentPaths function #940

ibrasho commented Aug 3, 2017 •

edited

Loading

ibrasho commented Aug 4, 2017 •

edited

Loading

sdboyer Aug 5, 2017

matjam Aug 5, 2017

ibrasho Aug 5, 2017 •

edited

Loading

sdboyer Aug 6, 2017

ibrasho Aug 8, 2017

sdboyer Aug 13, 2017

sdboyer Aug 6, 2017

sdboyer left a comment

sdboyer Aug 14, 2017

sdboyer Aug 14, 2017

sdboyer commented Aug 17, 2017

ibrasho commented Aug 21, 2017

ibrasho commented Sep 2, 2017

sdboyer left a comment

sdboyer Sep 2, 2017

ibrasho Sep 2, 2017

sudo-suhas commented Sep 4, 2017

sdboyer commented Sep 5, 2017

sudo-suhas commented Sep 5, 2017

sdboyer commented Sep 5, 2017

sudo-suhas commented Sep 5, 2017

ibrasho commented Sep 5, 2017

[WIP]: internal/fs: add EquivalentPaths function #940

[WIP]: internal/fs: add EquivalentPaths function #940

Conversation

ibrasho commented Aug 3, 2017 • edited Loading

What does this do / why do we need it?

What should your reviewer look out for in this PR?

Which issue(s) does this PR fix?

ibrasho commented Aug 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibrasho Aug 5, 2017 • edited 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

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdboyer commented Aug 17, 2017

ibrasho commented Aug 21, 2017

ibrasho commented Sep 2, 2017

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sudo-suhas commented Sep 4, 2017

sdboyer commented Sep 5, 2017

sudo-suhas commented Sep 5, 2017

sdboyer commented Sep 5, 2017

sudo-suhas commented Sep 5, 2017

ibrasho commented Sep 5, 2017

ibrasho commented Aug 3, 2017 •

edited

Loading

ibrasho commented Aug 4, 2017 •

edited

Loading

ibrasho Aug 5, 2017 •

edited

Loading