Regression: File recursions work only when I turn on hidden files #103530

New Issue

fwfeet · 2022-12-29T10:01:38+01:00

fwfeet commented

2022-12-29 10:01:38 +01:00

System Information
Operating system: Windows-10-10.0.19045-SP0 64 Bits
Graphics card: NVIDIA GeForce RTX 4090/PCIe/SSE2 NVIDIA Corporation 4.5.0 NVIDIA 527.56

Blender Version
Broken: version: 3.5.0 Alpha, branch: master, commit date: 2022-12-27 16:50, hash: d7dad425c0
Worked: 3.4

Caused by 2165136740

Short description of error
When I wanna see file recursions, he sees all the recursions as hidden files.
So I need to turn on "show hidden" and only then the recursions are seen. Happens on disk C and other disks too

Exact steps for others to reproduce the error
Open Blender
Ctrl + O
Use recursions
Observe no change in any folder
Show hidden in the filter now you can see the folders/files

**System Information** Operating system: Windows-10-10.0.19045-SP0 64 Bits Graphics card: NVIDIA GeForce RTX 4090/PCIe/SSE2 NVIDIA Corporation 4.5.0 NVIDIA 527.56 **Blender Version** Broken: version: 3.5.0 Alpha, branch: master, commit date: 2022-12-27 16:50, hash: `d7dad425c0` Worked: 3.4 Caused by 2165136740 **Short description of error** When I wanna see file recursions, he sees all the recursions as hidden files. So I need to turn on "show hidden" and only then the recursions are seen. Happens on disk C and other disks too ![image.png](https://archive.blender.org/developer/F14098588/image.png) **Exact steps for others to reproduce the error** Open Blender Ctrl + O Use recursions ![image.png](https://archive.blender.org/developer/F14098592/image.png) Observe no change in any folder Show hidden in the filter now you can see the folders/files

fwfeet commented

2022-12-29 10:01:38 +01:00

Added subscriber: @costa

Harley Acheson commented

2022-12-29 20:36:34 +01:00

Added subscribers: @JulianEisel, @ideasman42, @Harley

Harley Acheson commented

2022-12-29 20:36:34 +01:00

Changed status from 'Needs Triage' to: 'Confirmed'

Harley Acheson commented

2022-12-29 20:36:34 +01:00

@ideasman42 & @JulianEisel : Hard to explain this one.

On the surface this is "just" that files that are not in the current folder get file attributes of "hidden", therefore not visible until we show hidden. This probably also has error behavior in Mac and Linux, but will be different.

Making this harder to diagnose is (my) lack of error checking in BLI_file_attributes. If given an invalid path, the return of GetFileAttributesW is INVALID_FILE_ATTRIBUTES, which is -1 and therefore all bits set. So the test for FILE_ATTRIBUTE_HIDDEN is true and so the file is given hidden attribute. This should probably just test for INVALID_FILE_ATTRIBUTES, do a debug-only assert but return zero in release.

Why is the path invalid though?

In filelist_readjob_list_dir the call to current_relpath_append appends the recursed path name. Since this folder name is also in the root variable we get a duplication if using BLI_path_join. Following shows the entry relpath and root, both with the same "a" folder name.

{F14099307,width=100%}

Create a full path from these parts using BLI_path_join and that folder is duplicated in the path. BLI_file_attributes return "hidden" in that error condition. The invalid path is also used for BLI_file_alias_target.

Not sure how to address if this duplication of the recursed path in relpath and root are by design. I'm guessing so since everything else seems to work.

The mentioned error check (and return zero) would fix this, sort of (at least hide it), but would result in us not being able to deal with files with special attributes, or shortcut files, Mac Aliases, files that need special preview treatment (offline), outside of the current directory.

@ideasman42 & @JulianEisel : Hard to explain this one. On the surface this is "just" that files that are not in the current folder get file attributes of "hidden", therefore not visible until we show hidden. This probably also has error behavior in Mac and Linux, but will be different. Making this harder to diagnose is (my) lack of error checking in `BLI_file_attributes`. If given an invalid path, the return of `GetFileAttributesW` is `INVALID_FILE_ATTRIBUTES`, which is -1 and therefore all bits set. So the test for FILE_ATTRIBUTE_HIDDEN is true and so the file is given hidden attribute. This should probably just test for INVALID_FILE_ATTRIBUTES, do a debug-only assert but return zero in release. Why is the path invalid though? In `filelist_readjob_list_dir` the call to `current_relpath_append` appends the recursed path name. Since this folder name is also in the root variable we get a duplication if using BLI_path_join. Following shows the entry relpath and root, both with the same "a" folder name. {[F14099307](https://archive.blender.org/developer/F14099307/image.png),width=100%} Create a full path from these parts using BLI_path_join and that folder is duplicated in the path. BLI_file_attributes return "hidden" in that error condition. The invalid path is also used for BLI_file_alias_target. Not sure how to address if this duplication of the recursed path in relpath and root are by design. I'm guessing so since everything else seems to work. The mentioned error check (and return zero) would fix this, sort of (at least hide it), but would result in us not being able to deal with files with special attributes, or shortcut files, Mac Aliases, files that need special preview treatment (offline), outside of the current directory.

Ray molenkamp commented

2022-12-29 20:49:35 +01:00

Added subscriber: @LazyDodo

Ray molenkamp commented

2022-12-29 20:49:35 +01:00

The mentioned error check (and return zero) would fix this, sort of (at least hide it)

BLI_file_attributes has no opportunity to report back "hey this file doesn't actually exist", it seems to operate on the assumption the file path being fed into is valid, adding an additional check and return early would make the issue somewhat go away since it will now return no flags rather than all flags, but the actual issue is a bad path was being presented to it. it could live with the following "fix" for BLI_file_attributes though

diff --git a/source/blender/blenlib/intern/storage.c b/source/blender/blenlib/intern/storage.c
index c04fc41ab4d..b63c061fd69 100644
--- a/source/blender/blenlib/intern/storage.c
+++ b/source/blender/blenlib/intern/storage.c
@@ -210,6 +210,10 @@ eFileAttributes BLI_file_attributes(const char *path)
     return ret;
   }
   DWORD attr = GetFileAttributesW(wline);
+  BLI_assert_msg(attr != -1, "BLI_file_attributes should only be called on existing files.")
+  if (attr == -1)
+    return ret;
+
   if (attr & FILE_ATTRIBUTE_READONLY) {
     ret |= FILE_ATTR_READONLY;
   }

The root cause of this problem still needs to be addressed though.

> The mentioned error check (and return zero) would fix this, sort of (at least hide it) `BLI_file_attributes` has no opportunity to report back "hey this file doesn't actually exist", it seems to operate on the assumption the file path being fed into is valid, adding an additional check and return early would make the issue somewhat go away since it will now return no flags rather than all flags, but the actual issue is a bad path was being presented to it. it could live with the following "fix" for `BLI_file_attributes` though ``` diff --git a/source/blender/blenlib/intern/storage.c b/source/blender/blenlib/intern/storage.c index c04fc41ab4d..b63c061fd69 100644 --- a/source/blender/blenlib/intern/storage.c +++ b/source/blender/blenlib/intern/storage.c @@ -210,6 +210,10 @@ eFileAttributes BLI_file_attributes(const char *path) return ret; } DWORD attr = GetFileAttributesW(wline); + BLI_assert_msg(attr != -1, "BLI_file_attributes should only be called on existing files.") + if (attr == -1) + return ret; + if (attr & FILE_ATTRIBUTE_READONLY) { ret |= FILE_ATTR_READONLY; } ``` The root cause of this problem still needs to be addressed though.

Pratik Borhade changed title from ~~File recursions work only when I turn on hidden files~~ to Regression: File recursions work only when I turn on hidden files

2022-12-30 03:17:06 +01:00

Pratik Borhade commented

2022-12-30 03:17:24 +01:00

Added subscriber: @PratikPB2123

Harley Acheson commented

2022-12-30 03:53:32 +01:00

Removed subscriber: @ideasman42

Harley Acheson commented

2022-12-30 03:53:32 +01:00

Thanks for bisecting @PratikPB2123 !

Harley Acheson commented

2022-12-30 17:55:24 +01:00

If the (new) current behavior (recursed directories duplicated in relname and root) is by design then this would be all that is needed to fix this bug report:

diff --git a/source/blender/editors/space_file/filelist.cc b/source/blender/editors/space_file/filelist.cc
index cd1c3f8280b1..a11877de3cf1 100644
--- a/source/blender/editors/space_file/filelist.cc
+++ b/source/blender/editors/space_file/filelist.cc
@@ -2958,21 +2958,21 @@ static int filelist_readjob_list_dir(FileListReadJob *job_params,
       FileListInternEntry *entry;
 
       if (skip_currpar && FILENAME_IS_CURRPAR(files[i].relname)) {
         continue;
       }
 
       entry = MEM_cnew<FileListInternEntry>(__func__);
       entry->relpath = current_relpath_append(job_params, files[i].relname);
       entry->st = files[i].s;
 
-      BLI_path_join(full_path, FILE_MAX, root, entry->relpath);
+      BLI_path_join(full_path, FILE_MAX, root, files[i].relname);
       char *target = full_path;
 
       /* Set initial file type and attributes. */
       entry->attributes = BLI_file_attributes(full_path);
       if (S_ISDIR(files[i].s.st_mode)
 #ifdef __APPLE__
           && !(ED_path_extension_type(full_path) & FILE_TYPE_BUNDLE)
 #endif
       ) {
         entry->typeflag = FILE_TYPE_DIR;

**If** the (new) current behavior (recursed directories duplicated in relname and root) is *by design* then this would be all that is needed to fix this bug report: ``` diff --git a/source/blender/editors/space_file/filelist.cc b/source/blender/editors/space_file/filelist.cc index cd1c3f8280b1..a11877de3cf1 100644 --- a/source/blender/editors/space_file/filelist.cc +++ b/source/blender/editors/space_file/filelist.cc @@ -2958,21 +2958,21 @@ static int filelist_readjob_list_dir(FileListReadJob *job_params, FileListInternEntry *entry; if (skip_currpar && FILENAME_IS_CURRPAR(files[i].relname)) { continue; } entry = MEM_cnew<FileListInternEntry>(__func__); entry->relpath = current_relpath_append(job_params, files[i].relname); entry->st = files[i].s; - BLI_path_join(full_path, FILE_MAX, root, entry->relpath); + BLI_path_join(full_path, FILE_MAX, root, files[i].relname); char *target = full_path; /* Set initial file type and attributes. */ entry->attributes = BLI_file_attributes(full_path); if (S_ISDIR(files[i].s.st_mode) #ifdef __APPLE__ && !(ED_path_extension_type(full_path) & FILE_TYPE_BUNDLE) #endif ) { entry->typeflag = FILE_TYPE_DIR; ```

Thomas Dinges commented

2023-01-19 11:07:53 +01:00

Added subscriber: @ThomasDinges

Thomas Dinges commented

2023-01-19 11:07:53 +01:00

@JulianEisel Hey Julian, can you please check on this report and the proposed patch?

Julian Eisel commented

2023-02-01 20:21:06 +01:00

@Harley this patch is correct. This only worked previously because entry->relpath and files- [x].relname were the same for a moment, until entry->relpath would be updated properly.

In #103530#1467389, @Harley wrote:
If the (new) current behavior (recursed directories duplicated in relname and root) is by design

Not entirely sure what you mean.
Basically what we have is:

filelist->root the actual root of the filelist, which we may recurse into.
entry->relpath - the path (including file name) relative to filelist->root.
files- [x].relname - just the file name without path.
Previously, entry->relpath would be the same as file- [x].rename at first and only later be updated to be the full relative path.

Also, the root variable is badly named in this function, since it's not filelist->root, just the currently recursed into directory.

@Harley this patch is correct. This only worked previously because `entry->relpath` and `files- [x].relname` were the same for a moment, until `entry->relpath` would be updated properly. > In #103530#1467389, @Harley wrote: > **If** the (new) current behavior (recursed directories duplicated in relname and root) is *by design* Not entirely sure what you mean. Basically what we have is: - `filelist->root` the actual root of the filelist, which we may recurse into. - `entry->relpath` - the path (including file name) relative to `filelist->root`. - `files- [x].relname` - just the file name without path. Previously, `entry->relpath` would be the same as `file- [x].rename` at first and only later be updated to be the full relative path. Also, the `root` variable is badly named in this function, since it's not `filelist->root`, just the currently recursed into directory.

Harley Acheson commented

2023-02-01 20:31:53 +01:00

@JulianEisel - Also, the root variable is badly named in this function, since it's not filelist->root, just the currently recursed into directory.

Ah, that's it. Will just propose this fix and set you as reviewer (only).

Me and Ray will separately fix the error-checking in storage.c

> @JulianEisel - Also, the `root` variable is badly named in this function, since it's not `filelist->root`, just the currently recursed into directory. Ah, that's it. Will just propose this fix and set you as reviewer (only). Me and Ray will separately fix the error-checking in storage.c

Harley Acheson self-assigned this 2023-02-01 20:33:59 +01:00

blender-admin commented

2023-02-01 23:06:38 +01:00

This issue was referenced by d82ffb9787

This issue was referenced by d82ffb9787c3181c6f4432f19309b59009aa34ec

Harley Acheson commented

2023-02-01 23:08:15 +01:00

Changed status from 'Confirmed' to: 'Resolved'

Harley Acheson closed this issue

2023-02-01 23:08:15 +01:00

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Regression: File recursions work only when I turn on hidden files #103530