From 9f61224538f80f3227882e39ba5d18ed71ce0f49 Mon Sep 17 00:00:00 2001 From: Kory Draughn Date: Thu, 14 Mar 2024 15:20:27 -0400 Subject: [PATCH] [#7537] Add --ignore-symlinks alias for --link option. The --link option does not provide enough information to help the user understand its purpose. This commit addresses that by adding an alias that is more explicit and direct. This commit updates the option list of iput and irsync. --- .../src/irods_parse_command_line_options.cpp | 67 ++++++++++--------- lib/core/src/parseCommandLine.cpp | 3 +- scripts/irods/test/test_iput.py | 36 +++++++++- scripts/irods/test/test_irsync.py | 35 ++++++++++ 4 files changed, 107 insertions(+), 34 deletions(-) diff --git a/lib/core/src/irods_parse_command_line_options.cpp b/lib/core/src/irods_parse_command_line_options.cpp index 53ff77a8ac..32632b3162 100644 --- a/lib/core/src/irods_parse_command_line_options.cpp +++ b/lib/core/src/irods_parse_command_line_options.cpp @@ -26,38 +26,41 @@ static int parse_program_options( path_list_t& _paths ) { namespace po = boost::program_options; - po::options_description opt_desc( "options" ); + po::options_description opt_desc("options"); + // clang-format off opt_desc.add_options() - ( "help,h", "show command usage" ) - ( "all,a", "all - update all existing copies" ) - ( "bulk,b", "bulk upload to reduce overhead" ) - ( "force,f", "force - write data-object even it exists already; overwrite it" ) - ( "redirect,I", "redirect connection - redirect the connection to connect directly to the resource server." ) - ( "checksum,k", "checksum - calculate a checksum on the data server-side, and store it in the catalog" ) - ( "verify_checksum,K", "verify checksum - calculate and verify the checksum on the data, both client-side and server-side, without storing in the catalog." ) - ( "repl_num,n", po::value(), "replNum - the replica to be replaced, typically not needed" ) - ( "num_threads,N", po::value(), "numThreads - the number of threads to use for the transfer. A value of 0 means no threading. By default (-N option not used) the server decides the number of threads to use." ) - ( "physical_path,p", po::value(), "physicalPath - the absolute physical path of the uploaded file on the server" ) - ( "progress,P", "output the progress of the upload." ) - ( "rbudp,Q", "use RBUDP (datagram) protocol for the data transfer" ) - ( "recursive,r", "recursive - store the whole subdirectory" ) - ( "dest_resc,R", po::value(), "resource - specifies the resource to store to. This can also be specified in your environment or via a rule set up by the administrator" ) - ( "ticket,t", po::value(), "ticket - ticket (string) to use for ticket-based access" ) - ( "renew_socket,T", "renew socket connection after 10 minutes" ) - ( "verbose,v", "verbose" ) - ( "very_verbose,V", "very verbose" ) - ( "data_type,D", po::value(), "dataType - the data type string" ) - ( "restart_file,X", po::value(), "restartFile - specifies that the restart option is on and the restartFile input specifies a local file that contains the restart information." ) - ( "link", "ignore symlink." ) - ( "lfrestart", po::value(), "lfRestartFile - specifies that the large file restart option is on and the lfRestartFile input specifies a local file that contains the restart information." ) - ( "retries", po::value(), "count - Retry the iput in case of error. The 'count' input specifies the number of times to retry. It must be used with the -X option" ) - ( "wlock", "use advisory write (exclusive) lock for the upload" ) - ( "rlock", "use advisory read lock for the download" ) - ( "purgec", "Purge the staged cache copy after uploading an object to a" ) - ( "kv_pass", po::value(), "pass key-value strings through to the plugin infrastructure" ) - ( "metadata", po::value(), "atomically assign metadata after a data object is put" ) - ( "acl", po::value(), "atomically apply an access control list after a data object is put" ) - ( "path_args", po::value( &_paths )->composing(), "some files and stuffs" ); + ("help,h", "show command usage") + ("all,a", "all - update all existing copies") + ("bulk,b", "bulk upload to reduce overhead") + ("force,f", "force - write data-object even it exists already; overwrite it") + ("redirect,I", "redirect connection - redirect the connection to connect directly to the resource server.") + ("checksum,k", "checksum - calculate a checksum on the data server-side, and store it in the catalog") + ("verify_checksum,K", "verify checksum - calculate and verify the checksum on the data, both client-side and server-side, without storing in the catalog.") + ("repl_num,n", po::value(), "replNum - the replica to be replaced, typically not needed") + ("num_threads,N", po::value(), "numThreads - the number of threads to use for the transfer. A value of 0 means no threading. By default (-N option not used) the server decides the number of threads to use.") + ("physical_path,p", po::value(), "physicalPath - the absolute physical path of the uploaded file on the server") + ("progress,P", "output the progress of the upload.") + ("rbudp,Q", "use RBUDP (datagram) protocol for the data transfer") + ("recursive,r", "recursive - store the whole subdirectory") + ("dest_resc,R", po::value(), "resource - specifies the resource to store to. This can also be specified in your environment or via a rule set up by the administrator") + ("ticket,t", po::value(), "ticket - ticket (string) to use for ticket-based access") + ("renew_socket,T", "renew socket connection after 10 minutes") + ("verbose,v", "verbose") + ("very_verbose,V", "very verbose") + ("data_type,D", po::value(), "dataType - the data type string") + ("restart_file,X", po::value(), "restartFile - specifies that the restart option is on and the restartFile input specifies a local file that contains the restart information.") + ("link", "[Deprecated] ignore symlinks. Use --ignore-symlinks.") + ("ignore-symlinks", "ignore symlinks.") + ("lfrestart", po::value(), "lfRestartFile - specifies that the large file restart option is on and the lfRestartFile input specifies a local file that contains the restart information.") + ("retries", po::value(), "count - Retry the iput in case of error. The 'count' input specifies the number of times to retry. It must be used with the -X option") + ("wlock", "use advisory write (exclusive) lock for the upload") + ("rlock", "use advisory read lock for the download") + ("purgec", "Purge the staged cache copy after uploading an object to a") + ("kv_pass", po::value(), "pass key-value strings through to the plugin infrastructure") + ("metadata", po::value(), "atomically assign metadata after a data object is put") + ("acl", po::value(), "atomically apply an access control list after a data object is put") + ("path_args", po::value(&_paths)->composing(), "some files and stuffs"); + // clang-format on po::positional_options_description pos_desc; pos_desc.add( "path_args", -1 ); @@ -200,7 +203,7 @@ static int parse_program_options( return INVALID_ANY_CAST; } } - if ( global_prog_ops_var_map.count( "link" ) ) { + if (global_prog_ops_var_map.count("ignore-symlinks") || global_prog_ops_var_map.count("link")) { _rods_args.link = 1; } if ( global_prog_ops_var_map.count( "lfrestart" ) ) { diff --git a/lib/core/src/parseCommandLine.cpp b/lib/core/src/parseCommandLine.cpp index 5fb75857ac..2259de9957 100644 --- a/lib/core/src/parseCommandLine.cpp +++ b/lib/core/src/parseCommandLine.cpp @@ -18,6 +18,7 @@ CommandLineOptions document so we can keep it all consistent. #include "irodsntutil.hpp" #endif +#include /* Input: @@ -51,7 +52,7 @@ parseCmdLineOpt( int argc, char **argv, const char *optString, int includeLong, /* handle the long options first */ if ( includeLong ) { for ( int i = 0; i < argc; i++ ) { - if ( strcmp( "--link", argv[i] ) == 0 ) { + if (std::strcmp("--ignore-symlinks", argv[i]) == 0 || std::strcmp("--link", argv[i]) == 0) { rodsArgs->link = True; argv[i] = "-Z"; /* ignore symlink */ } diff --git a/scripts/irods/test/test_iput.py b/scripts/irods/test/test_iput.py index cfe7f7233a..76e3f2b177 100644 --- a/scripts/irods/test/test_iput.py +++ b/scripts/irods/test/test_iput.py @@ -821,6 +821,41 @@ def test_iput_does_not_ignore_symlinks_by_default__issue_5359(self): finally: self.user.assert_icommand(['ils', '-lr'], 'STDOUT', self.user.session_collection) # debugging + def test_iput_ignore_symlinks_option_is_an_alias_of_the_link_option__issue_7537(self): + # Create a file. + test_file_basename = 'iput_issue_7537.txt' + test_file = f'{self.user.local_session_dir}/{test_file_basename}' + with open(test_file, 'w') as f: + f.write('This file will be ignored by iput due to the symlink.') + + # Create a directory for the symlink. + # The symlink file must be placed in a separate directory due to the recursive flag for iput. + symlink_dir_basename = 'iput_issue_7537_symlink_dir' + symlink_dir = f'{self.user.local_session_dir}/{symlink_dir_basename}' + os.mkdir(symlink_dir) + + # Create a symlink to the file. + symlink_file_basename = f'{test_file_basename}.symlink' + symlink_file = f'{symlink_dir}/{symlink_file_basename}' + os.symlink(test_file, symlink_file) + self.assertTrue(os.path.exists(symlink_file)) + + # Try to upload the symlink file into iRODS. + self.user.assert_icommand(['iput', '--ignore-symlinks', symlink_file]) + self.user.assert_icommand(['ils', '-lr'], 'STDOUT') # Debugging + + # Show no data object exists, proving the symlink file was ignored. + logical_path = f'{self.user.session_collection}/{symlink_file_basename}' + self.assertFalse(lib.replica_exists(self.user, logical_path, 0)) + + # Try to upload the symlink file again using the recursive flag and the parent directory. + self.user.assert_icommand(['iput', '--ignore-symlinks', '-r', symlink_dir]) + self.user.assert_icommand(['ils', '-lr'], 'STDOUT') # Debugging + + # Show no data object exists, proving the symlink file was ignored. + logical_path = f'{self.user.session_collection}/{symlink_dir_basename}/{symlink_file_basename}' + self.assertFalse(lib.replica_exists(self.user, logical_path, 0)) + class test_iput_with_checksums(session.make_sessions_mixin(rodsadmins, rodsusers), unittest.TestCase): def setUp(self): @@ -985,4 +1020,3 @@ def overwrite_verify_stale_no_checksum(self, filename, expected_checksum): def overwrite_verify_stale_with_checksum(self, filename, expected_checksum): pass - diff --git a/scripts/irods/test/test_irsync.py b/scripts/irods/test/test_irsync.py index ae657c0202..8db8373dcb 100644 --- a/scripts/irods/test/test_irsync.py +++ b/scripts/irods/test/test_irsync.py @@ -646,3 +646,38 @@ def test_irsync_does_not_ignore_symlinks_by_default__issue_5359(self): finally: self.user0.assert_icommand(['ils', '-lr'], 'STDOUT', self.user0.session_collection) # debugging + + def test_irsync_ignore_symlinks_option_is_an_alias_of_the_link_option__issue_7537(self): + # Create a file. + test_file_basename = 'irsync_issue_7537.txt' + test_file = f'{self.user0.local_session_dir}/{test_file_basename}' + with open(test_file, 'w') as f: + f.write('This file will be ignored by irsync due to the symlink.') + + # Create a directory for the symlink. + # The symlink file must be placed in a separate directory due to the recursive flag for irsync. + symlink_dir_basename = 'irsync_issue_7537_symlink_dir' + symlink_dir = f'{self.user0.local_session_dir}/{symlink_dir_basename}' + os.mkdir(symlink_dir) + + # Create a symlink to the file. + symlink_file_basename = f'{test_file_basename}.symlink' + symlink_file = f'{symlink_dir}/{symlink_file_basename}' + os.symlink(test_file, symlink_file) + self.assertTrue(os.path.exists(symlink_file)) + + # Try to upload the symlink file into iRODS. + self.user0.assert_icommand(['irsync', '--ignore-symlinks', symlink_file, f'i:{symlink_file_basename}']) + self.user0.assert_icommand(['ils', '-lr'], 'STDOUT') # Debugging + + # Show no data object exists, proving the symlink file was ignored. + logical_path = f'{self.user0.session_collection}/{symlink_file_basename}' + self.assertFalse(lib.replica_exists(self.user0, logical_path, 0)) + + # Try to upload the symlink file again using the recursive flag and the parent directory. + self.user0.assert_icommand(['irsync', '--ignore-symlinks', '-r', symlink_dir, f'i:{symlink_dir_basename}']) + self.user0.assert_icommand(['ils', '-lr'], 'STDOUT') # Debugging + + # Show no data object exists, proving the symlink file was ignored. + logical_path = f'{self.user0.session_collection}/{symlink_dir_basename}/{symlink_file_basename}' + self.assertFalse(lib.replica_exists(self.user0, logical_path, 0))