ronantakizawa/codereview-bench huggingface dataset:
Repository: sxyazi/yazi
File: yazi-scheduler/src/file/in.rs (Rust)

Code before:
			relative: false,
			delete:   false,
		}
	}
}

// --- Cut
#[derive(Clone, Debug)]
pub(crate) struct FileInCut {
	pub(crate) id:     Id,
	pub(crate) from:   UrlBuf,
	pub(crate) to:     UrlBuf,
	pub(crate) cha:    Option<Cha>,
	pub(crate) follow: bool,
	pub(crate) retry:  u8,
}

impl FileInCut {
	pub(super) fn into_link(self) -> FileInLink {
		FileInLink {
			id:       self.id,
			from:     self.from,
			to:       self.to,
			cha:      self.cha,
			resolve:  true,
			relative: false,
			delete:   true,
		}
	}
}

// --- Link
#[derive(Clone, Debug)]
pub(crate) struct FileInLink {
	pub(crate) id:       Id,
	pub(crate) from:     UrlBuf,
	pub(crate) to:       UrlBuf,
	pub(crate) cha:      Option<Cha>,
	pub(crate) resolve:  bool,
	pub(crate) relative: bool,
	pub(crate) delete:   bool,
}

// --- Hardlink
#[derive(Clone, Debug)]
pub(crate) struct FileInHardlink {
	pub(crate) id:     Id,
	pub(crate) from:   UrlBuf,
	pub(crate) to:     UrlBuf,
	pub(crate) cha:    Option<Cha>,
	pub(crate) follow: bool,

Review comment (bug):
The Drop-based completion notification mechanism has a critical flaw when combined with the `queue` method's silent failure behavior. If a task with a drop notification sender is queued (either initially or on retry), and the `queue` method's `try_send` fails (line 451 in file.rs), the task will be immediately dropped, sending a completion notification even though the task was never actually queued or completed. This can cause the waiting loop in `cut` (lines 127-129 in file.rs) to proceed prematurely, believing all symlinks are complete when some were actually dropped due to channel congestion.

Consider either:
1. Using a different completion notification mechanism that explicitly signals completion from `cut_do` only on actual completion (not retry, not dropped tasks), or
2. Ensuring the `queue` method cannot fail and properly handles backpressure without silently dropping tasks
```suggestion
// Drop-based completion notification removed. Now, completion must be signaled explicitly after task completion.
```

Code after:
			resolve:  true,
			relative: false,
			delete:   false,
		}
	}
}

// --- Cut
#[derive(Clone, Debug)]
pub(crate) struct FileInCut {
	pub(crate) id:     Id,
	pub(crate) from:   UrlBuf,
	pub(crate) to:     UrlBuf,
	pub(crate) cha:    Option<Cha>,
	pub(crate) follow: bool,
	pub(crate) retry:  u8,
	pub(crate) drop:   Option<mpsc::Sender<()>>,
}

impl Drop for FileInCut {
	fn drop(&mut self) {
		if let Some(tx) = self.drop.take() {
			tx.try_send(()).ok();
		}
	}
}

impl FileInCut {
	pub(super) fn into_link(mut self) -> FileInLink {
		FileInLink {
			id:       self.id,
			from:     mem::take(&mut self.from),
			to:       mem::take(&mut self.to),
			cha:      self.cha,
			resolve:  true,
			relative: false,
			delete:   true,
		}
	}

	pub(super) fn with_drop(mut self, drop: &mpsc::Sender<()>) -> Self {
		self.drop = Some(drop.clone());
		self
	}
}

// --- Link
#[derive(Clone, Debug)]
pub(crate) struct FileInLink {
	pub(crate) id:       Id,
	pub(crate) from:     UrlBuf,
Repository: sxyazi/yazi
File: yazi-scheduler/src/file/in.rs (Rust)

Code before:
	pub(crate) follow: bool,
	pub(crate) retry:  u8,
}

impl FileInCut {
	pub(super) fn into_link(self) -> FileInLink {
		FileInLink {
			id:       self.id,
			from:     self.from,
			to:       self.to,
			cha:      self.cha,
			resolve:  true,
			relative: false,
			delete:   true,
		}
	}
}

// --- Link
#[derive(Clone, Debug)]
pub(crate) struct FileInLink {
	pub(crate) id:       Id,
	pub(crate) from:     UrlBuf,
	pub(crate) to:       UrlBuf,
	pub(crate) cha:      Option<Cha>,
	pub(crate) resolve:  bool,
	pub(crate) relative: bool,
	pub(crate) delete:   bool,
}

// --- Hardlink
#[derive(Clone, Debug)]
pub(crate) struct FileInHardlink {
	pub(crate) id:     Id,
	pub(crate) from:   UrlBuf,
	pub(crate) to:     UrlBuf,
	pub(crate) cha:    Option<Cha>,
	pub(crate) follow: bool,
}

// --- Delete
#[derive(Clone, Debug)]
pub(crate) struct FileInDelete {
	pub(crate) id:     Id,
	pub(crate) target: UrlBuf,
	pub(crate) cha:    Option<Cha>,
}

// --- Trash
#[derive(Clone, Debug)]
pub(crate) struct FileInTrash {

Review comment (bug):
[nitpick] For clarity and safety, the `drop` field should be explicitly cleared in `into_link()` to prevent any potential issues if this method is ever called on a task with an attached drop sender. While currently safe because `into_link()` is only called on tasks without drop senders (line 104 in file.rs), adding `mem::take(&mut self.drop)` would make the intent explicit and prevent future bugs.

Consider:
```rust
pub(super) fn into_link(mut self) -> FileInLink {
    let _ = mem::take(&mut self.drop);  // Explicitly clear drop sender
    FileInLink {
        // ... rest of the fields
    }
}
```

Code after:
	pub(crate) cha:    Option<Cha>,
	pub(crate) follow: bool,
	pub(crate) retry:  u8,
	pub(crate) drop:   Option<mpsc::Sender<()>>,
}

impl Drop for FileInCut {
	fn drop(&mut self) {
		if let Some(tx) = self.drop.take() {
			tx.try_send(()).ok();
		}
	}
}

impl FileInCut {
	pub(super) fn into_link(mut self) -> FileInLink {
		FileInLink {
			id:       self.id,
			from:     mem::take(&mut self.from),
			to:       mem::take(&mut self.to),
			cha:      self.cha,
			resolve:  true,
			relative: false,
			delete:   true,
		}
	}

	pub(super) fn with_drop(mut self, drop: &mpsc::Sender<()>) -> Self {
		self.drop = Some(drop.clone());
		self
	}
}

// --- Link
#[derive(Clone, Debug)]
pub(crate) struct FileInLink {
	pub(crate) id:       Id,
	pub(crate) from:     UrlBuf,
	pub(crate) to:       UrlBuf,
	pub(crate) cha:      Option<Cha>,
	pub(crate) resolve:  bool,
	pub(crate) relative: bool,
	pub(crate) delete:   bool,
}

// --- Hardlink
#[derive(Clone, Debug)]
pub(crate) struct FileInHardlink {
	pub(crate) id:     Id,
	pub(crate) from:   UrlBuf,
	pub(crate) to:     UrlBuf,
Repository: sxyazi/yazi
File: yazi-fs/src/provider/traits.rs (Rust)

Code before:
		}

		async move {
			let cha = ok_or_not_found!(self.symlink_metadata().await, return Ok(()));
			if cha.is_link() {
				self.remove_file().await
			} else {
				remove_dir_all_impl::<Self>(self.url()).await
			}
		}
	}

	fn remove_dir_clean(&self) -> impl Future<Output = ()> {
		let root = self.url().to_owned();

		async move {
			let mut stack = vec![(root, false)];

			while let Some((dir, visited)) = stack.pop() {
				let Ok(provider) = Self::new(dir.as_url()).await else {
					continue;
				};

				if visited {
					provider.remove_dir().await.ok();
				} else if let Ok(mut it) = provider.read_dir().await {
					stack.push((dir, true));
					while let Ok(Some(ent)) = it.next().await {
						if ent.file_type().await.is_ok_and(|t| t.is_dir()) {
							stack.push((ent.url(), false));
						}
					}
				}
			}
		}
	}

	fn remove_file(&self) -> impl Future<Output = io::Result<()>>;

	fn rename<P>(&self, to: P) -> impl Future<Output = io::Result<()>>
	where
		P: AsPath;

	fn symlink<S, F>(&self, original: S, _is_dir: F) -> impl Future<Output = io::Result<()>>
	where
		S: AsStrand,
		F: AsyncFnOnce() -> io::Result<bool>;

	fn symlink_dir<S>(&self, original: S) -> impl Future<Output = io::Result<()>>
	where
		S: AsStrand,

Review comment (bug):
The `remove_dir_clean` function only returns the result of the last `remove_dir` operation. If any intermediate directory fails to be removed, those errors are silently ignored and only the final directory's result is returned. This could result in reporting success even when some directories failed to be removed. Consider accumulating errors or returning the first error encountered.

Code after:
		}

		async move {
			let cha = ok_or_not_found!(self.symlink_metadata().await, return Ok(()));
			if cha.is_link() {
				self.remove_file().await
			} else {
				remove_dir_all_impl::<Self>(self.url()).await
			}
		}
	}

	fn remove_dir_clean(&self) -> impl Future<Output = io::Result<()>> {
		let root = self.url().to_owned();

		async move {
			let mut stack = vec![(root, false)];
			let mut result = Ok(());

			while let Some((dir, visited)) = stack.pop() {
				let Ok(provider) = Self::new(dir.as_url()).await else {
					continue;
				};

				if visited {
					result = provider.remove_dir().await;
				} else if let Ok(mut it) = provider.read_dir().await {
					stack.push((dir, true));
					while let Ok(Some(ent)) = it.next().await {
						if ent.file_type().await.is_ok_and(|t| t.is_dir()) {
							stack.push((ent.url(), false));
						}
					}
				}
			}
			result
		}
	}

	fn remove_file(&self) -> impl Future<Output = io::Result<()>>;

	fn rename<P>(&self, to: P) -> impl Future<Output = io::Result<()>>
	where
		P: AsPath;

	fn symlink<S, F>(&self, original: S, _is_dir: F) -> impl Future<Output = io::Result<()>>
	where
		S: AsStrand,
		F: AsyncFnOnce() -> io::Result<bool>;

	fn symlink_dir<S>(&self, original: S) -> impl Future<Output = io::Result<()>>
Repository: sxyazi/yazi
File: yazi-scheduler/src/hook/hook.rs (Rust)

Code before:
	}

	// --- File
	pub(crate) async fn cut(&self, task: HookInOutCut) {
		let intact = self.ongoing.lock().intact(task.id);
		if intact {
			provider::remove_dir_clean(&task.from).await.ok();
			Pump::push_move(&task.from, &task.to);
		}
		self.ops.out(task.id, FileOutCut::Clean);
	}

	pub(crate) async fn delete(&self, task: HookInOutDelete) {
		let intact = self.ongoing.lock().intact(task.id);
		if intact {
			provider::remove_dir_all(&task.target).await.ok();
			TasksProxy::update_succeed(&task.target);
			Pump::push_delete(&task.target);
		}
		self.ops.out(task.id, FileOutDelete::Clean);
	}

	pub(crate) async fn trash(&self, task: HookInOutTrash) {
		let intact = self.ongoing.lock().intact(task.id);
		if intact {
			TasksProxy::update_succeed(&task.target);
			Pump::push_trash(&task.target);
		}
		self.ops.out(task.id, FileOutTrash::Clean);
	}

	pub(crate) async fn download(&self, task: HookInOutDownload) {
		self.ops.out(task.id, FileOutDownload::Clean);
	}
}


Review comment (bug):
Unlike the `cut` and `delete` hooks which now track cleanup results with `Clean(io::Result<()>)`, the `trash` hook still uses `FileOutTrash::Clean` without tracking the cleanup result. This inconsistency means trash cleanup failures won't be properly reported or tracked in the task progress state. Consider adding a result parameter to `FileOutTrash::Clean` similar to `FileOutCut::Clean` and `FileOutDelete::Clean`.

Code after:
		Self { ops: ops.into(), ongoing: ongoing.clone() }
	}

	// --- File
	pub(crate) async fn cut(&self, task: HookInOutCut) {
		if !self.ongoing.lock().intact(task.id) {
			return;
		}

		let result = ok_or_not_found(provider::remove_dir_clean(&task.from).await);
		Pump::push_move(&task.from, &task.to);

		self.ops.out(task.id, FileOutCut::Clean(result));
	}

	pub(crate) async fn delete(&self, task: HookInOutDelete) {
		if !self.ongoing.lock().intact(task.id) {
			return;
		}

		let result = ok_or_not_found(provider::remove_dir_all(&task.target).await);
		TasksProxy::update_succeed(&task.target);
		Pump::push_delete(&task.target);

		self.ops.out(task.id, FileOutDelete::Clean(result));
	}

	pub(crate) async fn trash(&self, task: HookInOutTrash) {
		let intact = self.ongoing.lock().intact(task.id);
		if intact {
			TasksProxy::update_succeed(&task.target);
			Pump::push_trash(&task.target);
		}
		self.ops.out(task.id, FileOutTrash::Clean);
	}

	pub(crate) async fn download(&self, task: HookInOutDownload) {
		self.ops.out(task.id, FileOutDownload::Clean);
	}
}
Repository: sxyazi/yazi
File: yazi-scheduler/src/hook/hook.rs (Rust)

Code before:
	}

	// --- File
	pub(crate) async fn cut(&self, task: HookInOutCut) {
		let intact = self.ongoing.lock().intact(task.id);
		if intact {
			provider::remove_dir_clean(&task.from).await.ok();
			Pump::push_move(&task.from, &task.to);
		}
		self.ops.out(task.id, FileOutCut::Clean);
	}

	pub(crate) async fn delete(&self, task: HookInOutDelete) {
		let intact = self.ongoing.lock().intact(task.id);
		if intact {
			provider::remove_dir_all(&task.target).await.ok();
			TasksProxy::update_succeed(&task.target);
			Pump::push_delete(&task.target);
		}
		self.ops.out(task.id, FileOutDelete::Clean);
	}

	pub(crate) async fn trash(&self, task: HookInOutTrash) {
		let intact = self.ongoing.lock().intact(task.id);
		if intact {
			TasksProxy::update_succeed(&task.target);
			Pump::push_trash(&task.target);
		}
		self.ops.out(task.id, FileOutTrash::Clean);
	}

	pub(crate) async fn download(&self, task: HookInOutDownload) {
		self.ops.out(task.id, FileOutDownload::Clean);
	}
}


Review comment (bug):
The `download` hook unconditionally reports successful cleanup with `FileOutDownload::Clean`, but unlike `cut` and `delete` hooks, it doesn't actually perform any cleanup operation or track the result. This could lead to incorrect task state reporting if cleanup is expected but not performed. Consider either implementing actual cleanup logic or ensuring this matches the intended behavior for download tasks.

Code after:
		Self { ops: ops.into(), ongoing: ongoing.clone() }
	}

	// --- File
	pub(crate) async fn cut(&self, task: HookInOutCut) {
		if !self.ongoing.lock().intact(task.id) {
			return;
		}

		let result = ok_or_not_found(provider::remove_dir_clean(&task.from).await);
		Pump::push_move(&task.from, &task.to);

		self.ops.out(task.id, FileOutCut::Clean(result));
	}

	pub(crate) async fn delete(&self, task: HookInOutDelete) {
		if !self.ongoing.lock().intact(task.id) {
			return;
		}

		let result = ok_or_not_found(provider::remove_dir_all(&task.target).await);
		TasksProxy::update_succeed(&task.target);
		Pump::push_delete(&task.target);

		self.ops.out(task.id, FileOutDelete::Clean(result));
	}

	pub(crate) async fn trash(&self, task: HookInOutTrash) {
		let intact = self.ongoing.lock().intact(task.id);
		if intact {
			TasksProxy::update_succeed(&task.target);
			Pump::push_trash(&task.target);
		}
		self.ops.out(task.id, FileOutTrash::Clean);
	}

	pub(crate) async fn download(&self, task: HookInOutDownload) {
		self.ops.out(task.id, FileOutDownload::Clean);
	}
}
Repository: sxyazi/yazi
File: yazi-vfs/src/provider/sftp/gate.rs (Rust)

Code before:
		self
	}

	fn create_new(&mut self, create_new: bool) -> &mut Self {
		self.0.create_new(create_new);
		self
	}

	async fn open<U>(&self, url: U) -> io::Result<Self::File>
	where
		U: AsUrl,
	{
		let url = url.as_url();
		let (path, (name, config)) = match url {
			Url::Sftp { loc, domain } => (*loc, Vfs::service::<&ServiceSftp>(domain).await?),
			_ => Err(io::Error::new(io::ErrorKind::InvalidInput, format!("Not an SFTP URL: {url:?}")))?,
		};

		let flags = Flags::from(*self);
		let attrs = super::Attrs(self.0.attrs).try_into().unwrap_or_default();
		Ok(Conn { name, config }.roll().await?.open(path, flags, &attrs).await?)
	}

	fn read(&mut self, read: bool) -> &mut Self {
		self.0.read(read);
		self
	}

	fn truncate(&mut self, truncate: bool) -> &mut Self {
		self.0.truncate(truncate);
		self
	}

	fn write(&mut self, write: bool) -> &mut Self {
		self.0.write(write);
		self
	}
}


Review comment (bug):
The condition `status.is_failure()` only checks for `StatusCode::Failure` (value 4). However, SFTP servers may also return `StatusCode::FileAlreadyExists` (value 11) for file creation operations when the file already exists. This status code should also be checked to avoid making an unnecessary `lstat()` call. Consider checking for both `status.is_failure()` and checking if the status code is `FileAlreadyExists`, or use a more comprehensive check that covers all error cases where a subsequent lstat might be needed.
```suggestion
			&& let Err(yazi_sftp::Error::Status(_status)) = &result
```

Code after:
		self
	}

	fn create_new(&mut self, create_new: bool) -> &mut Self {
		self.0.create_new(create_new);
		self
	}

	async fn open<U>(&self, url: U) -> io::Result<Self::File>
	where
		U: AsUrl,
	{
		let url = url.as_url();
		let (path, (name, config)) = match url {
			Url::Sftp { loc, domain } => (*loc, Vfs::service::<&ServiceSftp>(domain).await?),
			_ => Err(io::Error::new(io::ErrorKind::InvalidInput, format!("Not an SFTP URL: {url:?}")))?,
		};

		let conn = Conn { name, config }.roll().await?;
		let flags = Flags::from(*self);
		let attrs = super::Attrs(self.0.attrs).try_into().unwrap_or_default();

		let result = conn.open(path, flags, &attrs).await;
		if self.0.create_new
			&& let Err(yazi_sftp::Error::Status(status)) = &result
			&& status.is_failure()
			&& conn.lstat(path).await.is_ok()
		{
			return Err(io::Error::from(io::ErrorKind::AlreadyExists));
		}

		Ok(result?)
	}

	fn read(&mut self, read: bool) -> &mut Self {
		self.0.read(read);
		self
	}

	fn truncate(&mut self, truncate: bool) -> &mut Self {
		self.0.truncate(truncate);
		self
	}

	fn write(&mut self, write: bool) -> &mut Self {
		self.0.write(write);
		self
	}
}
Repository: sxyazi/yazi
File: yazi-plugin/preset/plugins/archive.lua (Lua)

Code before:
local M = {}

function M:peek(job)
	local limit = job.area.h
	local filepath = tostring(job.file.path)
	local files, bound, err = nil, nil, nil

	if M.is_nested_tar_path(filepath) then
		files, bound, err = self.list_files_tar({ "-p", filepath }, job.skip, limit)
	else
		files, bound, err = self.list_files({ "-p", filepath }, job.skip, limit)
	end

	return M:display_response(job, files, bound, err)
end

-- Display a response to an archive list. Handles errors and empty archives.
-- Can be called by other plugins, to implement their own archive peek logic - the API of this function should remain stable.
-- @param job Job
-- @param files table List of files in the archive - { { path=string, size=integer, attr=string }, ... }
-- @param bound integer Last bound reached
-- @param err Error? Error encountered during listing
function M:display_response(job, files, bound, err)
	local limit = job.area.h
	if err then
		return ya.preview_widget(job, err)
	elseif job.skip > 0 and bound < job.skip + limit then
		return ya.emit("peek", { math.max(0, bound - limit), only_if = job.file.url, upper_bound = true })

Review comment (refactor):
Can you revert the refactor to `M:peek`, for example the new `display_response` method? Since it's only called once, I don't see the need for it to be a separate method, and if it's for code cleanliness, it's better done in a separate refactor PR.

Code after:
local M = {}

function M:peek(job)
	local limit = job.area.h
	local files, bound, err = self.list_archive({ "-p", tostring(job.file.path) }, job.skip, limit)

	local first = (#files == 1 and files[1]) or (#files == 0 and M.list_if_only_one(job.file.path))
	if first and M.should_decompress_tar(first) then
		files, bound, err = self.list_compressed_tar({ "-p", tostring(job.file.path) }, job.skip, limit)
	end

	if err then
		return ya.preview_widget(job, err)
	elseif job.skip > 0 and bound < job.skip + limit then
		return ya.emit("peek", { math.max(0, bound - limit), only_if = job.file.url, upper_bound = true })
	elseif #files == 0 then
		files = { { path = job.file.url.stem, size = 0, packed_size = 0, attr = "" } }
	end

	local left, right = {}, {}
	for _, f in ipairs(files) do
		local icon = File({
			url = Url(f.path),
			cha = Cha { mode = tonumber(f.attr:sub(1, 1) == "D" and "40700" or "100644", 8) },
		}):icon()

		if f.size > 0 then
			right[#right + 1] = string.format(" %s ", ya.readable_size(f.size))
Repository: sxyazi/yazi
File: yazi-plugin/preset/plugins/archive.lua (Lua)

Code before:
		if event == 1 and M.is_encrypted(next) then
			code = 2
			break
		elseif event == 1 then
			code = 3
		elseif event == 0 then
			head = head .. next
		else
			break
		end

		typ = head:gmatch("--[\r\n]+Path = .-[\r\n]+Type = (.-)[\r\n]+")()
		if typ then
			break
		end
	end

	child:start_kill()
	return typ ~= "" and typ or nil, code
end

function M.is_encrypted(s) return s:find(" Wrong password", 1, true) end

function M.is_tar(path) return M.list_meta { "-p", tostring(path) } == "tar" end

function M.is_nested_tar_path(path)
    -- stylua: ignore
    local exts = {
        -- Formats supported by 7z for tar archives
        ".tar.gz", ".tgz", ".tar.gzip",
        ".tar.bz2", ".tbz", ".tbz2",
        ".tar.xz", ".txz",
        ".tar.lzma", ".tlz",
        -- Might be supported one day (and are supported by https://github.com/mcmilk/7-Zip-zstd)
        ".tar.zst", ".tzst",
        ".tar.br",
        ".tar.lz4", ".tar.lz5",
    }

	for _, ext in ipairs(exts) do
		if path:sub(-#ext) == ext then
			return true
		end
	end

	return false
end

return M


Review comment (suggestion):
7-Zip's output for `*.tar.*` contains only a single file named `*.tar` and it's better to check the file type from that output since it's more accurate than relying on the extension.

Code after:
	local key, value, stderr = "", "", {}
	repeat
		local next, event = child:read_line()
		if event == 1 and M.is_encrypted(next) then
			err = Err("File list in this archive is encrypted")
			break
		elseif event == 1 then
			stderr[#stderr + 1] = next
			goto continue
		elseif event ~= 0 then
			break
		end

		if next == "\n" or next == "\r\n" then
			i = i + 1
			if files[#files].path ~= "" then
				files[#files + 1] = { path = "", size = 0, packed_size = 0, attr = "" }
			end
			goto continue
		elseif i < skip then
			goto continue
		end

		key, value = next:match("^(%u[%a ]+) = (.-)[\r\n]+")
		if key == "Path" then
			files[#files].path = value
		elseif key == "Size" then
			files[#files].size = tonumber(value) or 0
		elseif key == "Packed Size" then
			files[#files].packed_size = tonumber(value) or 0
		elseif key == "Attributes" then
			files[#files].attr = value
		end

		::continue::
	until i >= skip + limit

	if files[#files].path == "" then
		files[#files] = nil
	end
	if #stderr ~= 0 then
		err = Err("7-zip errored out while listing files, stderr: %s", table.concat(stderr, "\n"))
	end
	return files, i, err
end

return M