Xuanwo commented on code in PR #5005:
URL: https://github.com/apache/opendal/pull/5005#discussion_r1716281282
##########
core/src/services/fs/lister.rs:
##########
@@ -60,16 +62,24 @@ impl oio::List for FsLister<tokio::fs::ReadDir> {
.replace('\\', "/"),
);
- let d = if ft.is_file() {
- oio::Entry::new(&rel_path, Metadata::new(EntryMode::FILE))
- } else if ft.is_dir() {
+ let default_meta = self.op.metakey() == Metakey::Mode;
+
+ let (metadata, is_dir) = if default_meta {
+ let ft = de.file_type().await.map_err(new_std_io_error)?;
+ get_metadata(self.op.metakey(), None, Some(ft))?
+ } else {
+ let fs_meta = de.metadata().await.map_err(new_std_io_error)?;
+ get_metadata(self.op.metakey(), Some(fs_meta), None)?
+ };
+
+ let p = if is_dir {
Review Comment:
We don't need `is_dir`, we can use `metadata.mode()` to check.
##########
core/src/services/fs/lister.rs:
##########
@@ -93,17 +103,61 @@ impl oio::BlockingList for FsLister<std::fs::ReadDir> {
// (no extra system calls needed), but some Unix platforms may
// require the equivalent call to symlink_metadata to learn about
// the target file type.
- let file_type = de.file_type().map_err(new_std_io_error)?;
- let entry = if file_type.is_file() {
- oio::Entry::new(&rel_path, Metadata::new(EntryMode::FILE))
- } else if file_type.is_dir() {
+ let default_meta = self.op.metakey() == Metakey::Mode;
+
+ let (metadata, is_dir) = if default_meta {
+ let ft = de.file_type().map_err(new_std_io_error)?;
+ get_metadata(self.op.metakey(), None, Some(ft))?
+ } else {
+ let fs_meta = de.metadata().map_err(new_std_io_error)?;
+ get_metadata(self.op.metakey(), Some(fs_meta), None)?
+ };
+
+ let p = if is_dir {
// Make sure we are returning the correct path.
- oio::Entry::new(&format!("{rel_path}/"),
Metadata::new(EntryMode::DIR))
+ &format!("{rel_path}/")
} else {
- oio::Entry::new(&rel_path, Metadata::new(EntryMode::Unknown))
+ &rel_path
};
- Ok(Some(entry))
+ Ok(Some(oio::Entry::new(p, metadata)))
}
}
+
+fn get_metadata(
+ metakey: FlagSet<Metakey>,
+ fs_meta: Option<std::fs::Metadata>,
+ file_type: Option<FileType>,
+) -> Result<(Metadata, bool)> {
+ let ft = if let Some(t) = fs_meta.as_ref() {
Review Comment:
If we expand logic, we don't need this.
##########
core/src/services/fs/lister.rs:
##########
@@ -93,17 +103,61 @@ impl oio::BlockingList for FsLister<std::fs::ReadDir> {
// (no extra system calls needed), but some Unix platforms may
// require the equivalent call to symlink_metadata to learn about
// the target file type.
- let file_type = de.file_type().map_err(new_std_io_error)?;
- let entry = if file_type.is_file() {
- oio::Entry::new(&rel_path, Metadata::new(EntryMode::FILE))
- } else if file_type.is_dir() {
+ let default_meta = self.op.metakey() == Metakey::Mode;
+
+ let (metadata, is_dir) = if default_meta {
+ let ft = de.file_type().map_err(new_std_io_error)?;
+ get_metadata(self.op.metakey(), None, Some(ft))?
+ } else {
+ let fs_meta = de.metadata().map_err(new_std_io_error)?;
+ get_metadata(self.op.metakey(), Some(fs_meta), None)?
+ };
+
+ let p = if is_dir {
// Make sure we are returning the correct path.
- oio::Entry::new(&format!("{rel_path}/"),
Metadata::new(EntryMode::DIR))
+ &format!("{rel_path}/")
} else {
- oio::Entry::new(&rel_path, Metadata::new(EntryMode::Unknown))
+ &rel_path
};
- Ok(Some(entry))
+ Ok(Some(oio::Entry::new(p, metadata)))
}
}
+
+fn get_metadata(
+ metakey: FlagSet<Metakey>,
+ fs_meta: Option<std::fs::Metadata>,
+ file_type: Option<FileType>,
+) -> Result<(Metadata, bool)> {
+ let ft = if let Some(t) = fs_meta.as_ref() {
+ t.file_type()
+ } else if let Some(t) = file_type {
+ t
+ } else {
+ panic!("At least one metadata should be provided!")
+ };
+
+ let mut meta = if ft.is_file() {
+ Metadata::new(EntryMode::FILE)
+ } else if ft.is_dir() {
+ Metadata::new(EntryMode::DIR)
+ } else {
+ Metadata::new(EntryMode::Unknown)
+ };
+
+ if fs_meta.is_some() {
+ let contains_metakey = |k| metakey.contains(k) ||
metakey.contains(Metakey::Complete);
+ let fs_meta = fs_meta.unwrap();
+
+ if contains_metakey(Metakey::ContentLength) {
Review Comment:
We don't need to check the meta key. If we have sent `metadata` syscall, we
can fill in any known data.
##########
core/src/services/fs/lister.rs:
##########
@@ -93,17 +103,61 @@ impl oio::BlockingList for FsLister<std::fs::ReadDir> {
// (no extra system calls needed), but some Unix platforms may
// require the equivalent call to symlink_metadata to learn about
// the target file type.
- let file_type = de.file_type().map_err(new_std_io_error)?;
- let entry = if file_type.is_file() {
- oio::Entry::new(&rel_path, Metadata::new(EntryMode::FILE))
- } else if file_type.is_dir() {
+ let default_meta = self.op.metakey() == Metakey::Mode;
+
+ let (metadata, is_dir) = if default_meta {
+ let ft = de.file_type().map_err(new_std_io_error)?;
+ get_metadata(self.op.metakey(), None, Some(ft))?
+ } else {
+ let fs_meta = de.metadata().map_err(new_std_io_error)?;
+ get_metadata(self.op.metakey(), Some(fs_meta), None)?
+ };
+
+ let p = if is_dir {
// Make sure we are returning the correct path.
- oio::Entry::new(&format!("{rel_path}/"),
Metadata::new(EntryMode::DIR))
+ &format!("{rel_path}/")
} else {
- oio::Entry::new(&rel_path, Metadata::new(EntryMode::Unknown))
+ &rel_path
};
- Ok(Some(entry))
+ Ok(Some(oio::Entry::new(p, metadata)))
}
}
+
+fn get_metadata(
Review Comment:
Hi, I feel like this helper function is a bit overdesigned. How about
writing the logic directly? OpenDAL doesn't have a code style yet, but we tend
to optimize for readability and avoid unnecessary abstraction, which can help a
lot.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
