Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

local: info: provide st_ino and st_nlink #1053

Merged
merged 1 commit into from
Sep 27, 2022
Merged

local: info: provide st_ino and st_nlink #1053

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion fsspec/implementations/local.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ def info(self, path, **kwargs):
"created": out.st_ctime,
"islink": link,
}
for field in ["mode", "uid", "gid", "mtime"]:
for field in ["mode", "uid", "gid", "mtime", "ino", "nlink"]:
Copy link
Member Author

@efiop efiop Sep 26, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These could be provided by sshfs as well and maybe some other implementations.

I was thinking about maybe creating stat and lstat fields in info instead, but that seemed excessive for now (it will probably make sense to provide both of those in the future since we already have them, but there is a question of whether providing raw stat_result structure in stat and lstat info fields is a good practice or if we should add an info-like field link_info instead). I saw that uid and gid were provided here already, which are quite specific, so felt like inode and nlink were alright to provide too. Obviously open to suggestions. 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear: the reason why I'm proposing adding these two is that we use them in dvc and right now have to do additional stat calls.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The philosophy in the various backends has been to pass through as many dict keys as seemed reasonable, but only insist on name/type/size. I think the dict version is simpler and so superior to making a stat-like class. As ever, we are somewhat stuck between providing an identical API and specialising to the backends - link_info being a perfect case. Maybe it applies to two FSs rather than one, but most do not have this concept. So in info() we want consistency, but we can make extra methods for calling code that knows to look for them.

I'm proposing adding these two is that we use them in dvc and right now have to do additional stat calls.

That's a good enough reason!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Then I guess we'll end up adding _stat_to_into() helper and including link_info into the current info() result sometime in the future.

result[field] = getattr(out, "st_" + field)
if result["islink"]:
result["destination"] = os.readlink(path)
Expand Down