search

jprichardson / node-fs-extra

Node.js: extra methods for the fs object like copy(), remove(), mkdirs()
MIT License
9.44k stars 776 forks source link

bug: moveSync doesn't preserve timestamp when moveAcrossDevice #992

Closed zxcat closed 1 year ago

zxcat commented 1 year ago

it's just not implemented: https://github.com/jprichardson/node-fs-extra/blob/f3a7f0beeb5858c628b10010ad819c813c7f3565/lib/move/move-sync.js#L45-L50

so if you move something from one device to another then you'll lose timestamps (even with preserveTimestamps option).

versions - **Operating System:** macos 10.15.7 - **Node.js version:** 14.19.2 - **`fs-extra` version:** 11.1.0
RyanZim commented 1 year ago

@zxcat Good catch! Any chance you could submit a PR?

RyanZim commented 1 year ago

I'm sorry, I should have double-checked before commenting. I thought you were saying that moveSync didn't implement an document option that move did; I should have double-checked the docs. Only copy* supports preserveTimestamps; move does not. In that light, this is not a bug, this would be a feature request to add a preserveTimestamps option to the move* methods.

It feels like preserveTimestamps for move is much more of a niche use-case than for copy. @manidlou any thoughts here?

zxcat commented 1 year ago

Well, my report was not detailed enough. You're right, both move and moveSync drop timestamps in some cases. I named it bug because of the following:

  1. Normal system mv preserves timestamp in any case: it just moves items with all its contents, including attributes.
  2. When moving "inside" one device, both move and moveSync work as expected: they preserve timestamp.
  3. When move from one device to another, move and moveSync kill timestamps, but normal mv preserve them. It's quite unexpected, because it's no more "moving" something, but "moving"+modifying attributes.

As about preserveTimestamps option: I just checked if it works with move to have a temporary workaround. It didn't. Anyway the proper way is to do normal move instead move+some modification.

As for PR: fastfix is to add preserveTimestamps: true to opts here (and in move.js): https://github.com/jprichardson/node-fs-extra/blob/f3a7f0beeb5858c628b10010ad819c813c7f3565/lib/move/move-sync.js#L45-L50 The proper way is to add tests too. I'm sorry, I cannot add tests now.

KostiantynPopovych commented 1 year ago

Hi! I can update my PR with the suggested approach if you decide to go with it. Feel free to ping me!

RyanZim commented 1 year ago

Sorry for the super slow response here. @zxcat Now I understand; yes, agreed, this is a bug. PR welcome to set preserveTimestamps: true for usage of copy*() in move*().