Failed copy/move operations make ETA inaccurate

[Nick]

When I copied ~400GB of files, some of the files were corrupted and MC subsequently skipped them. However, the overall progress (ETA) didn't reflect this. The result was that at the end of the copy operation (12 hours later), the operation suddenly finished even though the ETA was still at ~3 minutes. This gave the impression that the operation had finished prematurely, when infact the ETA simply hadn't taken the skipped files into account.

It would obviously be better for the ETA to reach 0 when the copy/move operation finishes.

Also, the number of processed files reading doesn't take skipped files into account, meaning at the end of the copy/move operation it will say something like '497/500 files'. I think the processed files count should include skipped files, so that at the end of the operation, the count would be 500/500 files every time, otherwise it seems like the operation has finished prematurely.

[onlyjob]

Just yesterday when I was copying over 50000 files from slow network location I've noticed that ETA was shown as follows in MC_4.8.12:

Time: 0:16.42 ETA -2147483648:-2147483648.-2147483648 (11.39 KB

So I hope ETA values can be checked for overflows etc.

[SkunkWorks]

In the above example, it is said the '497/500 files' should actually be '500/500 files'.
In my opinion, it should end with '497/497 files', if there are 3 files skipped.

A change of 3 files out of 500 can be seen as a minor bug, but imagine that you are refreshing a destination directory tree and 250 of 500 are skipped. Or 450 of 500...

I think each skipped file shoud be removed from the total number of files AND it's volume substracted from the total volume. That way, the estimated speed would allways stay correct, and the estimated time would be updated at each skip.

[andrew_b]

Replying to cri:

A very small reget bug I only noticed now: when doing a "reget", the ETA time is always overestimated; I haven't looked at the code, but it seems the ETA is wrongly computed using the size of the whole file; of course it should be computed only on the remaining part still to be transferred.

[cri]

Replying to andrew_b:

Replying to cri:

A very small reget bug I only noticed now: when doing a "reget", the ETA time is always overestimated; I haven't looked at the code, but it seems the ETA is wrongly computed using the size of the whole file; of course it should be computed only on the remaining part still to be transferred.

(thanks for pointing me to the right ticket)

But this should be then universal, so not only FTP transfer and was present before, right?

I noticed the wrong ETA using FTP reget, so this might or might not be related to the skipped files scenario described by OP. I can confirm the problem was already present before the changes introduced to fix #4563

[andrew_b]

Branch: 3205_eta
Initial changeset:f911cf30cb97eda6be2768f49f9554d87c6ddd51

[zaytsev]

CI is not happy, did you get an email as expected?

[zaytsev]

I cannot check every line, but I like your refactorings, I think they are really good. One minor suggestion:

- /* Start of file transferring */
+ /* File transfer start time */

Please feel free to merge when you fix the warnings:

  file.c:962:50: error: variable 't' may be uninitialized when used here [-Werror,-Wconditional-uninitialized]
    962 |         ctx->pauses += g_get_monotonic_time () - t;
        |                                                  ^
  file.c:945:13: note: initialize the variable 't' to silence this warning
    945 |     gint64 t;
        |             ^
        |              = 0
  file.c:1171:26: error: implicit conversion turns floating-point number into integer: 'double' to 'long' [-Werror,-Wfloat-conversion]
   1171 |     ctx->bps = file_part / dt;
        |              ~ ~~~~~~~~~~^~~~
  file.c:1185:39: error: implicit conversion turns floating-point number into integer: 'double' to 'size_t' (aka 'unsigned long') [-Werror,-Wfloat-conversion]
   1185 |         ctx->total_bps = copied_bytes / dt;
        |                        ~ ~~~~~~~~~~~~~^~~~
  3 errors generated.

[andrew_b]

Replying to zaytsev:

CI is not happy, did you get an email as expected?

Yes. But I cannot view logs because I'm not logged Github in. I'm not logged Github in because of fucking forced 2FA. Is there any way to make the logs visible to everyone?

[zaytsev]

I do think so - only the jobs are results, but not the logs. We don't require 2FA in the organization, but GitHub want to enforce it for everyone eventually. Just get a TOTP on the computer, I'm afraid that the resistance is futile :-(

../../../src/filemanager/file.c: In function ‘do_file_error’:
../../../src/filemanager/file.c:1108:57: error: passing argument 3 of ‘real_do_file_error’ makes integer from pointer without a cast [-Werror=int-conversion]
 1108 |     return real_do_file_error (Foreground, allow_retry, str);
      |                                                         ^~~
      |                                                         |
      |                                                         const char *
../../../src/filemanager/file.c:942:80: note: expected ‘gboolean’ {aka ‘int’} but argument is of type ‘const char *’
  942 | real_do_file_error (file_op_context_t * ctx, enum OperationMode mode, gboolean allow_retry,
      |                                                                       ~~~~~~~~~^~~~~~~~~~~
../../../src/filemanager/file.c:1108:12: error: too few arguments to function ‘real_do_file_error’
 1108 |     return real_do_file_error (Foreground, allow_retry, str);
      |            ^~~~~~~~~~~~~~~~~~
../../../src/filemanager/file.c:942:1: note: declared here
  942 | real_do_file_error (file_op_context_t * ctx, enum OperationMode mode, gboolean allow_retry,
      | ^~~~~~~~~~~~~~~~~~
../../../src/filemanager/file.c: In function ‘files_error’:
../../../src/filemanager/file.c:1145:27: error: passing argument 1 of ‘do_file_error’ makes integer from pointer without a cast [-Werror=int-conversion]
 1145 |     return do_file_error (ctx, TRUE, buf);
      |                           ^~~
      |                           |
      |                           file_op_context_t *
../../../src/filemanager/file.c:1106:25: note: expected ‘gboolean’ {aka ‘int’} but argument is of type ‘file_op_context_t *’
 1106 | do_file_error (gboolean allow_retry, const char *str)
      |                ~~~~~~~~~^~~~~~~~~~~
In file included from /usr/lib/x86_64-linux-gnu/glib-2.0/include/glibconfig.h:9,
                 from /usr/include/glib-2.0/glib/gtypes.h:32,
                 from /usr/include/glib-2.0/glib/galloca.h:32,
                 from /usr/include/glib-2.0/glib.h:30,
                 from ../../../lib/global.h:10,
                 from ../../../src/filemanager/file.c:63:
/usr/include/glib-2.0/glib/gmacros.h:880:17: error: passing argument 2 of ‘do_file_error’ makes pointer from integer without a cast [-Werror=int-conversion]
  880 | #define TRUE    (!FALSE)
      |                 ^~~~~~~~
      |                 |
      |                 int
../../../src/filemanager/file.c:1145:32: note: in expansion of macro ‘TRUE’
 1145 |     return do_file_error (ctx, TRUE, buf);
      |                                ^~~~
../../../src/filemanager/file.c:1106:50: note: expected ‘const char *’ but argument is of type ‘int’
 1106 | do_file_error (gboolean allow_retry, const char *str)
      |                                      ~~~~~~~~~~~~^~~
../../../src/filemanager/file.c:1145:12: error: too many arguments to function ‘do_file_error’
 1145 |     return do_file_error (ctx, TRUE, buf);
      |            ^~~~~~~~~~~~~
../../../src/filemanager/file.c:1106:1: note: declared here
 1106 | do_file_error (gboolean allow_retry, const char *str)
      | ^~~~~~~~~~~~~
../../../src/filemanager/file.c: In function ‘file_error’:
../../../src/filemanager/file.c:3742:27: error: passing argument 1 of ‘do_file_error’ makes integer from pointer without a cast [-Werror=int-conversion]
 3742 |     return do_file_error (ctx, allow_retry, buf);
      |                           ^~~
      |                           |
      |                           file_op_context_t *
../../../src/filemanager/file.c:1106:25: note: expected ‘gboolean’ {aka ‘int’} but argument is of type ‘file_op_context_t *’
 1106 | do_file_error (gboolean allow_retry, const char *str)
      |                ~~~~~~~~~^~~~~~~~~~~
../../../src/filemanager/file.c:3742:32: error: passing argument 2 of ‘do_file_error’ makes pointer from integer without a cast [-Werror=int-conversion]
 3742 |     return do_file_error (ctx, allow_retry, buf);
      |                                ^~~~~~~~~~~
      |                                |
      |                                gboolean {aka int}
../../../src/filemanager/file.c:1106:50: note: expected ‘const char *’ but argument is of type ‘gboolean’ {aka ‘int’}
 1106 | do_file_error (gboolean allow_retry, const char *str)
      |                                      ~~~~~~~~~~~~^~~
../../../src/filemanager/file.c:3742:12: error: too many arguments to function ‘do_file_error’
 3742 |     return do_file_error (ctx, allow_retry, buf);
      |            ^~~~~~~~~~~~~
../../../src/filemanager/file.c:1106:1: note: declared here
 1106 | do_file_error (gboolean allow_retry, const char *str)
      | ^~~~~~~~~~~~~
../../../src/filemanager/file.c: In function ‘do_file_error’:
../../../src/filemanager/file.c:1109:1: error: control reaches end of non-void function [-Werror=return-type]
 1109 | }
      | ^
../../../src/filemanager/file.c: At top level:

[zaytsev]

Now the CI is green again on all systems.

[andrew_b]

Merged to master: [f85f7fec2920c4d6c9976acaafd230b51da5ccfa].

git log --oneline c5b8b6937..f85f7fec2