D8274: Don't create new symlinks when copying symlinks - copy the file's contents - like /usr/bin/cp

[Shlomi Fish]

shlomif abandoned this revision.
shlomif added a comment.


  Closing then. I don't have any immediate plans for reworking this patch.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8274

To: shlomif, dfaure
Cc: kde-frameworks-devel, ngraham, rkflx, dfaure, bcooksley, LeGast00n, cblack, 
michaelh, bruns

D8274: Don't create new symlinks when copying symlinks - copy the file's contents - like /usr/bin/cp

[David Faure]

dfaure added a comment.
Herald edited subscribers, added: kde-frameworks-devel; removed: Frameworks.


  @shlomif any plans on redoing this another way? Otherwise can you close the 
review request, to clean up dashboards?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8274

To: shlomif, dfaure
Cc: kde-frameworks-devel, ngraham, rkflx, dfaure, bcooksley, LeGast00n, cblack, 
michaelh, bruns, #frameworks

D8274: Don't create new symlinks when copying symlinks - copy the file's contents - like /usr/bin/cp

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  Yes, emulating `cp -a` is on purpose.
  
  A flag for a "deep copy" might be useful, but not changing the default 
behaviour.
  
  And I definitely hope this patch breaks jobtest ;-)
  
(otherwise it would mean jobtest is missing testcases)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8274

To: shlomif, dfaure
Cc: ngraham, rkflx, dfaure, bcooksley, #frameworks

D8274: Don't create new symlinks when copying symlinks - copy the file's contents - like /usr/bin/cp

[Henrik Fehlauer]

rkflx added subscribers: dfaure, rkflx.
rkflx added a comment.


  Unfortunately, this two year old patch does not apply for me anymore to the 
current git master (while it did apply to v5.15.0, it would not compile then).
  
  In addition, I'm not convinced this should be changed at the Frameworks 
level. Developers might rely on the current behaviour, so the patch should 
probably rather clarify the API docs 

 from `cp` to `cp -a`. According to @dfaure, this is on purpose 
. If your IRC logs say 
otherwise, feel free to post them here for reference.
  
  To give a practical example: Let's assume in Dolphin a user copies a folder 
to another location on the same partition, which he later plans to restore 
after trying out some things in the original folder. He would rightly expect to 
get back the original state, including all symlinks. With your patch (please 
correct me if I misunderstood) this would break.
  
  Therefore, I would encourage you (in addition to the API docs modification) 
to provide patches, or at least open bugzilla issues where still missing, 
against applications where a deep copy would be the appropriate behaviour 
indeed (e.g. Gwenview).

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8274

To: shlomif
Cc: rkflx, dfaure, bcooksley, #frameworks

D8274: Don't create new symlinks when copying symlinks - copy the file's contents - like /usr/bin/cp

[Ben Cooksley]

bcooksley added a comment.


  You don't have to worry about Phabricator going away, we were on Reviewboard 
for quite a long time before we started to move away from it.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8274

To: shlomif
Cc: bcooksley, #frameworks

D8274: Don't create new symlinks when copying symlinks - copy the file's contents - like /usr/bin/cp

[Shlomi Fish]

shlomif created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  This patch copies the symlinked file's contents upon a copy - like 
/usr/bin/cp instead of creating a new symlink. This is an attempt to fix 
https://bugs.kde.org/show_bug.cgi?id=315482 and 
https://bugs.kde.org/show_bug.cgi?id=335808 which were reported in gwenview and 
was done per consensus in an IRC discussion on #kde-devel on freenode. Also see 
 https://git.reviewboard.kde.org/r/125227/ . Please review it SOON before 
Phabricator is replaced by something else.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8274

AFFECTED FILES
  src/core/copyjob.cpp

To: shlomif
Cc: #frameworks