From: SF/projects/mingw n. l. <min...@li...> - 2010-08-19 02:36:11
|
Patches item #3047571, was opened at 2010-08-18 05:46 Message generated for change (Comment added) made by cstrauss You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=302435&aid=3047571&group_id=2435 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: msys Group: None Status: Open Resolution: None Priority: 5 Private: No Submitted By: Ladislav Michl (ladis) Assigned to: Cesar Strauss (cstrauss) Summary: Fix order of msys_symlink arguments Initial Comment: According to POSIX standard symlink name is second argument http://www.opengroup.org/onlinepubs/009695399/functions/symlink.html No functional changes otherwise, except memory is no longer leaked when creating symlink to directory. ---------------------------------------------------------------------- >Comment By: Cesar Strauss (cstrauss) Date: 2010-08-18 23:36 Message: Looks good, thanks. Could you provide an entry for Changelog.MSYS as well? Thanks, Cesar ---------------------------------------------------------------------- Comment By: Ladislav Michl (ladis) Date: 2010-08-18 16:01 Message: Uploaded another version of patch with more meaningfull argument names. ---------------------------------------------------------------------- Comment By: Earnie Boyd (earnie) Date: 2010-08-18 10:02 Message: As it stands I don't know that there is any reason to think of any plans for merging upstream. More likely merging upstream to the fork may happen, at least that is the reason for the msys_symlink that and easily recognizing which code went with the fork. I'll let Cesar decide if the functionality should be merged back to symlink. ---------------------------------------------------------------------- Comment By: Ladislav Michl (ladis) Date: 2010-08-18 08:51 Message: Well, the patch was ment as a preparation for native symlink merge. In the long term plan, I'd like to rewrite path.cc without all cygwin leftovers and let symlink function exist only in symlink.cc. Therefore I swapped argument of msys_symlink function which is now 'internal implementation', but as symlink function only call this one, I'd like to remove it and rename msys_symlink to symlink (assuming cygwin fork happened only once, and there are no plans to merge with 'upstream') As far as RecursiveCopy goes, I did it just for convenience, to make it consistent with msys_symlink and I do agree, that these arguments would be better named src and dst. Shall I update patch this way? ---------------------------------------------------------------------- Comment By: Keith Marshall (keithmarshall) Date: 2010-08-18 08:37 Message: Thanks for the patch. I'll leave it for Cesar, to conduct a detailed review, but just one comment, on cursory inspection: The semantics of copying and linking are quite different; while it is quite correct that the POSIX spec for the symlink() function places the 'topath' argument first, followed by the 'frompath', (i.e. we follow the reference from the created link to the target data), is it really imperative that we maintain this relationship within what is purely an internal implementation detail? IOW, while it may make sense to reverse the argument order to msys_symlink(), it perhaps isn't essential. The symlink() function, as implemented in path.cc already appears to provide a POSIXly correct API; that the argument order appears to be reversed when calling the msys_symlink() helper may be a potential cause of confusion, but it is an internal detail, which isn't really problematic provided it is applied consistently at every point of use. OTOH, the reversal of the order of the first two arguments to RecursiveCopy, while also an internal detail, just seems semantically wrong. At a shell prompt, we express the command to create a symbolic link as: ln -s topath frompath with the argument order semantically matching the order of arguments to be passed to the symlink() function. However, to perform a recursive copy, we express the command as: cp -r frompath topath Note the semantic difference. In both cases, the final argument represents what is to be created, and the preceding argument specifies whence the data is to be sourced, but in the case of the copy operation, we copy the data from the source to the destination, whereas in the case of linking, we refer from the *destination* back to the source. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=302435&aid=3047571&group_id=2435 |