memmap: opening empty path should be an error

[aviau]

@bep Can you confirm that I have updated the lstat tests correctly?

[CLAassistant]

All committers have signed the CLA.

[bep]

I think you need to start with why this is needed? Looks like a breaking change.

[aviau]

why this is needed?

Because this is how Go's os package behaves. Isn't the point of MemMapFs to behave like Go's os package?

Looks like a breaking change

This is only a breaking change for code that relied on a bug.

[bep]

Because this is how Go's os package behaves.

Go does not have a memory mapped filesystem in its Stdlib.

This is only a breaking change for code that relied on a bug.

And that makes it all good? If this is a bug, I think it would be bad to punish the users who have followed the "current spec".

I added a test in #173 to illustrate my point. This is the BaseFS, which also behaves the same in this case. Whether this is wrong in that particular case, I'm not sure. You have a BaseFS that points at a relative location, so an empty string may make sense as the relative starting point (the root) of that file system.

But we can not change the above without breaking working applications. And I think we need a better reason than "this is how the OS filesystem behaves".

Also, you need to use the OS specific filepath separator in the tests.

[aviau]

And that makes it all good?

I think yes because otherwise afero couldn't be used to replace the os filesystem in unit tests without producing different results. I think that we should do as much as we can to keep OsFs and MemMapFs compatible and interchangeable.

so an empty string may make sense as the relative starting point (the root) of that file system.

Why? An empty string isn't accepted as the starting point of the os filesystem so why would it be different in Afero? The base, whether you are using BaseFS or not, should be "/".

Also, you need to use the OS specific filepath separator in the tests.

done

[bep]

I think yes because otherwise afero couldn't be used to replace the os filesystem in unit tests without producing different results.

I would say that writing unit tests against the OS root filesystem would be a rare thing. I have never seen it. And if you do, I don't think it is this particular test that is going to prove that your program works or not.

I will end this discussion. This is not my project. @spf13 should chime in here. I will just say that if this patch is merged into master, you will most likely make more people angry than the other way around. And if so, you should fix all filesystems to behave the same.

[aviau]

I would say that writing unit tests against the OS root filesystem would be a rare thing

Indeed, this is why I use Afero's MemMapFs.

I have a program that checks whether or not a file exists. The file name was empty but the program wouldn't notice and the tests would pass because memMapFs.Stat("") does not return an error.

My tests run with MemMapFs but the program runs with OsFS.

The tests would pass but the program wouldn't work.

My tests are correct and they should have caught the bug.

you will most likely make more people angry than the other way around

Consistency with go's Os package adds much more value to the library than the convenience of accepting empty paths.

Here is a list of open bugs demanding that MemMapFs return errors in cases where it currently doesn't:

• MemMapFs.Create() works without parent directory #168 : MemMapFs.Create() works without parent directory
• MemMapFs Write() succeeds after Close() #166 : MemMapFs Write() succeeds after Close()
• MemMapFs.Mkdir() does not return error when it should #164 : MemMapFs.Mkdir() does not return error when it should
• MemMapFs.Mkdir should error if parent path does not exist #149 : MemMapFs.Mkdir should error if parent path does not exist
• MemMapFs.OpenFile allows opening, reading from, and writing to directories as regular files without error. #101 : MemMapFs.OpenFile allows opening, reading from, and writing to directories as regular files without error.

I think that the message is clear. People expect MemMapFs to work like OsFs.

@spf13 should chime in here

he hasn't commented on any of the 19 open pull requests, some of which date 2016. I don't see this happening any time soon :/

[bep]

Are any of those issues fixed by this patch?

[aviau]

Are any of those issues fixed by this patch?

That isn't really what I meant, let me rephrase. I think that the nature of these issues shows that afero's users expect that MemMapFs fail in cases where OsFs also fails. I was responding to your point that this would likely make people angry. If anything, I think that this will help people catch bugs or inconsistencies in their programs.

[bep]

I would say that writing unit tests against the OS root filesystem would be a rare thing.

The above sentence does not make much sense, I meant: "unit tests against the root in the OS filesystem".

[aviau]

I meant: "unit tests against the root in the OS filesystem".

Does the example in my above comment answer your question?

I'll add code:

func DoSomethingOnlyIfFileExists(filename string, fs afero.Fs) error {

    if err := fs.Stat(filename); err != nil {
        return err
    }

    // ...
}

This function would catch an empty filename with OsFs but not with MemMapFs.

My unit tests use MemMapFs and didn't catch a bug where the caller of the function would use an empty filename argument, but they should have!

[bep]

Does the example in my above comment answer your question?

Not really. Having client code interact directly with the root of the OS filesystem is most likely not what you would want. The beauty of Afero is no just the "unit test abstraction", there is a read only wrapper and a base filesystem, a composite etc. so you can restrict access to only the parts of the filesystem you want to expose.

So, for me, this issue falls into the big maybe category as to "is this a bug" -- and in the big probably too expensive in the "should we fix" category.

[mrjoshuak]

I think @aviau is correct that the expected behavior is consistency. In fact it is a clearly stated feature and use case in the readme. Users of this package expect the statements in bold below to be accurate. They are not. Fixes are expected. Those relying on the current broken behavior and unable to update their usage should vendor the version they rely on. This is a very common workflow, and not an unreasonable expectation in the slightest. Otherwise all progress stops.

That being said there are quite a few bugs in the current code that need work. I'd be happy to help if I had merge approval.

---

Afero Features

• A single consistent API for accessing a variety of filesystems
• Interoperation between a variety of file system types
• A set of interfaces to encourage and enforce interoperability between backends
• An atomic cross platform memory backed file system
• Support for compositional (union) file systems by combining multiple file systems acting as one
• Specialized backends which modify existing filesystems (Read Only, Regexp filtered)
• A set of utility functions ported from io, ioutil & hugo to be afero aware

Using Afero

Afero is easy to use and easier to adopt.

A few different ways you could use Afero:

• Use the interfaces alone to define you own file system.
• Wrap for the OS packages.
• Define different filesystems for different parts of your application.
• Use Afero for mock filesystems while testing

[0xmichalis]

This looks like a good fix to me that will reduce confusion wrt expectations of how MkDir is supposed to work. It is a backwards-incompatible change but thankfully afero is using semantic versioning so the next tag to be created can be 2.0.0 to signal to users that there are backwards-incompatible changes. Unless @spf13 speaks up against a 2.x cut, I will do it within the next few days.

[spf13]

My only advice on this is, if we are going to cut a 2.0 let's get all the breaking changes we want into it. I'd also suggest announcing it and giving a couple weeks for others to chime in / make suggestions for other breaking changes.

…

On Fri, Mar 27, 2020 at 5:37 PM Michail Kargakis ***@***.***> wrote: This looks like a good fix to me that will reduce confusion wrt expectations of how MkDir is supposed to work. It is a backwards-incompatible change but thankfully afero is using semantic versioning so the next tag to be created can be 2.0.0 to signal to users that there are backwards-incompatible changes. Unless @spf13 <https://github.com/spf13> speaks up against a 2.x cut, I will do it within the next few days. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#171 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABKKZGTGIUBIHXMLHICJSDRJUMCZANCNFSM4FD35U2A> .

[0xmichalis]

Makes sense. Which is the best venue to announce so folks can chime in?

…

-------- Original Message --------

On 28 Mar 2020, 00:22, Steve Francia wrote: My only advice on this is, if we are going to cut a 2.0 let's get all the breaking changes we want into it. I'd also suggest announcing it and giving a couple weeks for others to chime in / make suggestions for other breaking changes. On Fri, Mar 27, 2020 at 5:37 PM Michail Kargakis ***@***.***> wrote: > This looks like a good fix to me that will reduce confusion wrt > expectations of how MkDir is supposed to work. It is a > backwards-incompatible change but thankfully afero is using semantic > versioning so the next tag to be created can be 2.0.0 to signal to users > that there are backwards-incompatible changes. Unless @spf13 > <https://github.com/spf13> speaks up against a 2.x cut, I will do it > within the next few days. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#171 (comment)>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/AABKKZGTGIUBIHXMLHICJSDRJUMCZANCNFSM4FD35U2A> > . > — You are receiving this because you commented. Reply to this email directly, [view it on GitHub](#171 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AA5YK75WNKZEYP6YNIQK6QDRJUYKHANCNFSM4FD35U2A).

[spf13]

Not sure. Everyone is distracted right now. Maybe give it more time given that. Probably twitter and the readme are good starts. Maybe even reach out to some of the more prominent projects using Afero. https://pkg.go.dev/github.com/spf13/afero?tab=importedby Looks like a lot of the imports are through Viper, but there's a good number of direct imports too. On Fri, Mar 27, 2020 at 7:34 PM Michail Kargakis <notifications@github.com> wrote:

…

Makes sense. Which is the best venue to announce so folks can chime in? -------- Original Message -------- On 28 Mar 2020, 00:22, Steve Francia wrote: > My only advice on this is, if we are going to cut a 2.0 let's get all the > breaking changes we want into it. I'd also suggest announcing it and giving > a couple weeks for others to chime in / make suggestions for other breaking > changes. > > On Fri, Mar 27, 2020 at 5:37 PM Michail Kargakis < ***@***.***> > wrote: > >> This looks like a good fix to me that will reduce confusion wrt >> expectations of how MkDir is supposed to work. It is a >> backwards-incompatible change but thankfully afero is using semantic >> versioning so the next tag to be created can be 2.0.0 to signal to users >> that there are backwards-incompatible changes. Unless @spf13 >> <https://github.com/spf13> speaks up against a 2.x cut, I will do it >> within the next few days. >> >> — >> You are receiving this because you were mentioned. >> Reply to this email directly, view it on GitHub >> <#171 (comment)>, or >> unsubscribe >> < https://github.com/notifications/unsubscribe-auth/AABKKZGTGIUBIHXMLHICJSDRJUMCZANCNFSM4FD35U2A > >> . >> > > — > You are receiving this because you commented. > Reply to this email directly, [view it on GitHub]( #171 (comment)), or [unsubscribe]( https://github.com/notifications/unsubscribe-auth/AA5YK75WNKZEYP6YNIQK6QDRJUYKHANCNFSM4FD35U2A). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#171 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABKKZH2ZC7Y2UUJZWKL3BTRJUZY5ANCNFSM4FD35U2A> .