diskfs / go-diskfs

MIT License
494 stars 112 forks source link

fat32: make ReadDir exclude special VolumeLabel entry #239

Closed quite closed 1 month ago

quite commented 1 month ago

I think it would make sense if the ReadDir implementation for fat32 did not include the special VolumeLabel file entry among the returned entries. There is no way for the user of the API to discern this special entry from the other entries, as what's returned is []os.FileInfo. So the existence of this file is potentially only confusing. And if the user wants to deal with the volume label there are the specialized Label and SetLabel functions on the fat32 FileSystem.

I think it would be a simple matter of this small patch: (except for any broken test cases)

diff --git a/filesystem/fat32/fat32.go b/filesystem/fat32/fat32.go
index acb68ed..d5096a7 100644
--- a/filesystem/fat32/fat32.go
+++ b/filesystem/fat32/fat32.go
@@ -5,6 +5,7 @@ import (
        "fmt"
        "os"
        "path"
+       "slices"
        "sort"
        "strings"
        "time"
@@ -491,6 +492,9 @@ func (fs *FileSystem) ReadDir(p string) ([]os.FileInfo, error) {
        if err != nil {
                return nil, fmt.Errorf("error reading directory %s: %v", p, err)
        }
+       entries = slices.DeleteFunc(entries, func(e *directoryEntry ) bool {
+               return e.isVolumeLabel
+       })
        // once we have made it here, looping is done. We have found the final entry
        // we need to return all of the file info
        count := len(entries)

I can provide PR if this seems like a good idea.

deitch commented 1 month ago

Yes, I think you are right, it is a good idea, and would welcome a PR.

I think the implementation is slightly different, though. It already loops through all of the entries, so:

    for i, e := range entries {
                if e.isVolumeLabel {
                    continue
                }
        shortName := e.filenameShort
        if e.lowercaseShortname {
            shortName = strings.ToLower(shortName)
        }
quite commented 1 month ago

True. I was removing the label entry from the internal slice first, in order to be able to keep the preallocation (and putting into place by index) of os.FileInfo entries. One more loop and prealloc, or single loop and append(). Do n't know which is generally better...

On 21 July 2024 17:10:28 CEST, Avi Deitcher @.***> wrote:

Yes, I think you are right, it is a good idea, and would welcome a PR.

I think the implementation is slightly different, though. It already loops through all of the entries, so:

  for i, e := range entries {
               if e.isVolumeLabel {
                   continue
               }
      shortName := e.filenameShort
      if e.lowercaseShortname {
          shortName = strings.ToLower(shortName)
      }