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.

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

local: info: provide st_ino and st_nlink #1053

Merged
merged 1 commit into from Sep 27, 2022

Conversation

efiop
Copy link
Member

@efiop efiop commented Sep 26, 2022

No description provided.

@@ -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

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.

@martindurant martindurant merged commit a05c896 into fsspec:master Sep 27, 2022
efiop added a commit to efiop/dvc-objects that referenced this pull request Oct 3, 2022
efiop added a commit to iterative/dvc-objects that referenced this pull request Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants