Repository navigation
Conversation
github-actions
Bot
added
needs triage
zyv
added
area: core
zyv
changed the title
zyv
changed the title
zyv
left a comment
zyv
left a comment
There was a problem hiding this comment.
Thanks for looking into it!
I have had it build & run tested by the agent, and it found one related issue with "Ignore All" (see comment & commit). Do you agree?
Also, note that the commit message references the wrong ticket, please correct that.
In addition to that, I've added a regression test.
Finally, the agent found a whole bunch of less closely related bugs in copy_file_file, including data loss. How about we make the following deal: I commit them as a patch series and make a follow-up PR after this one is merged, and you review it?
| if (dst_status == DEST_SHORT_QUERY) | ||
| { | ||
| // Query to remove short file | ||
| if (query_dialog (Q_ ("DialogTitle|Copy"), _ ("Incomplete file was retrieved"), D_ERROR, 2, | ||
| _ ("&Delete"), _ ("&Keep")) | ||
| == 0) | ||
| dst_status = DEST_SHORT_DELETE; | ||
| else | ||
| dst_status = DEST_SHORT_KEEP; | ||
| } |
There was a problem hiding this comment.
The hang is fixed, but every failing file still stops the job with "Ignore all" at the modal "Incomplete file was retrieved [Delete] [Keep]" query. This seems to be the same problem as described for "Skip". Could "Ignore all" answer it as well?
if (dst_status == DEST_SHORT_QUERY)
{
if (ctx->ignore_all)
{
// "Ignore all" answers this question too, but never deletes pre-existing data
dst_status = appending ? DEST_SHORT_KEEP : DEST_SHORT_DELETE;
}
else
{
// Query to remove short file
if (query_dialog (Q_ ("DialogTitle|Copy"), _ ("Incomplete file was retrieved"), D_ERROR,
2, _ ("&Delete"), _ ("&Keep"))
== 0)
dst_status = DEST_SHORT_DELETE;
else
dst_status = DEST_SHORT_KEEP;
}
}There was a problem hiding this comment.
I think it's a separate problem which should be solved by introduction of Delete All and Keep All buttons.
…ommander#4631) Signed-off-by: Phil Krylov <phil@krylov.eu>
…errors Add tests/src/filemanager/copy_file_file.c, which copies a real file through the local VFS whose read callback is replaced to fail with EIO. With ctx->ignore_all set, copy_file_file() must give up after a single failed read and return FILE_IGNORE_ALL instead of retrying forever. The fake read stops failing after 1000 calls, so a regression makes the test fail (read_calls == 1001) rather than hang. Mark query_dialog() as MC_MOCKABLE, like message() already is, so the "Incomplete file was retrieved" prompt can be stubbed in tests. Assisted-By: Claude Opus 5.5 Signed-off-by: Yury V. Zaytsev <yury@shurup.com>
After "Ignore all", every file that fails to read still stopped the operation with the "Incomplete file was retrieved [Delete] [Keep]" query. Use "Delete" (the default button) without asking, but keep the target when appending, because it held data before this copy. Assisted-By: Claude Opus 5.5 Signed-off-by: Yury V. Zaytsev <yury@shurup.com>
|
@zyv Thanks! I've fixed the commit message. As for the Delete/Keep questions, I believe it's a separate issue which should be resolved by adding Delete All/Keep All options, and this can be done in another PR. |
2 participants
Proposed changes
On a read error that is encountered after Ignore All has been selected, don't loop forever but skip the rest of the file.
Checklist
git commit --amend -smake indent && make check)