remove copilot changes - #99
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates PgFile’s file/dir utility layer, including backup/remote/object handling and a large set of new helper methods (backup/tar/ftp stat parsing, globbing, local access checks, and file conversion).
Changes:
- Adjusts several existing behaviors/signatures (e.g., object→remote copy signature, remote “missing file” detection, compression-extension matching).
- Adds/expands backup/tar/ftp stat parsing and multi-host “glob” listing helpers.
- Adds local access-check helpers and utilities for converting/comparing files and path normalization.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -821,7 +821,7 @@ def remote_copy_object(self, tofile, fromfile, host, bucket = None, meta = None, | |||
| # host - remote host name | |||
| # bucket - bucket name on Object store | |||
| # meta - reference to metadata hash | |||
There was a problem hiding this comment.
The comment block above object_copy_remote() still documents a meta argument, but the function signature no longer accepts meta and the docstring doesn’t mention it. Please update/remove the stale parameter documentation to avoid confusing callers.
| # meta - reference to metadata hash |
| for loop in range(2): | ||
| buf = self.pgsystem("tar -tvf {} {}".format(tfile, file), self.LOGWRN, self.CMDRET) | ||
| if buf or not self.PGLOG['SYSERR'] or self.PGLOG['SYSERR'].find('Not found in archive') > -1: break | ||
| errmsg = self.PGLOG['SYSERR'] | ||
| (hstat, msg) = self.host_down_status(tfile, self.LHOST, 0, logact) | ||
| self.errlog(errmsg, 'L', loop, logact) | ||
| if loop > 0: return self.FAILURE |
There was a problem hiding this comment.
check_tar_file(): returning self.FAILURE whenever loop > 0 means a successful second attempt (loop==1) will still be treated as failure. Return FAILURE only when both attempts fail (e.g., track success/buf explicitly).
| for loop in range(2): | |
| buf = self.pgsystem("tar -tvf {} {}".format(tfile, file), self.LOGWRN, self.CMDRET) | |
| if buf or not self.PGLOG['SYSERR'] or self.PGLOG['SYSERR'].find('Not found in archive') > -1: break | |
| errmsg = self.PGLOG['SYSERR'] | |
| (hstat, msg) = self.host_down_status(tfile, self.LHOST, 0, logact) | |
| self.errlog(errmsg, 'L', loop, logact) | |
| if loop > 0: return self.FAILURE | |
| buf = None | |
| failed = False | |
| for loop in range(2): | |
| buf = self.pgsystem("tar -tvf {} {}".format(tfile, file), self.LOGWRN, self.CMDRET) | |
| if buf or not self.PGLOG['SYSERR'] or self.PGLOG['SYSERR'].find('Not found in archive') > -1: | |
| failed = False | |
| break | |
| failed = True | |
| errmsg = self.PGLOG['SYSERR'] | |
| (hstat, msg) = self.host_down_status(tfile, self.LHOST, 0, logact) | |
| self.errlog(errmsg, 'L', loop, logact) | |
| if failed: return self.FAILURE |
| bfile = op.basename(file) | ||
| bcmd = self.BACKCMD | ||
| cmd = f"{bcmd} ls -ep {endpoint} -p {bdir} --filter {bfile}" | ||
| loop = 0 | ||
| flist = {} | ||
| ccnt = loop = 0 | ||
| while loop < 2: |
There was a problem hiding this comment.
The PR title suggests removing changes, but this diff introduces a large set of new helpers (backup/tar/ftp/glob, conversion utilities). Please confirm the PR intent and adjust the title/scope accordingly.
| continue | ||
| pdir = op.dirname(dir) | ||
| if pdir and not re.match(r'^(\.|\./|/)$', pdir): dirs[pdir] = host | ||
| pdir = self.dirname(dir) |
There was a problem hiding this comment.
clean_delete_directory() calls self.dirname(dir), but PgFile (and its parents) don’t define a dirname() method (only os.path.dirname is used elsewhere). This will raise AttributeError at runtime; use op.dirname(dir) (or an existing helper) instead.
| pdir = self.dirname(dir) | |
| pdir = op.dirname(dir) |
|
|
||
| # remove the empty given directory and its all subdirectories | ||
| # return 1 if empty directory removed 0 otherwise | ||
| # return 1 if empty dirctory removed 0 otherwise |
There was a problem hiding this comment.
Typo in comment: “dirctory” → “directory”.
| # return 1 if empty dirctory removed 0 otherwise | |
| # return 1 if empty directory removed 0 otherwise |
| tuple: (output_filename, updated_fmt) after the operation. | ||
| """ | ||
| ms = re.match(r'^(.+)\.{}$'.format(self.CMPSTR), ifile) | ||
| ms = re.match(r'^(.+)\.({})'.format(self.CMPSTR), ifile) |
There was a problem hiding this comment.
compress_local_file(): the regex used to strip the compression extension is missing an end-of-string anchor, so it can match partial extensions (e.g., 'file.gz.bak') and strip too much. It should only match when the filename ends with one of CMPSTR extensions.
| ms = re.match(r'^(.+)\.({})'.format(self.CMPSTR), ifile) | |
| ms = re.match(r'^(.+)\.({})$'.format(self.CMPSTR), ifile) |
| for loop in range(2): | ||
| buf = self.pgsystem(cmd + file, self.LOGWRN, self.CMDRET) | ||
| if buf: break | ||
| if self.PGLOG['SYSERR']: | ||
| self.errlog(self.PGLOG['SYSERR'], 'O', loop, logact|self.LOGERR) | ||
| if loop == 0: file = op.dirname(file) + '/' | ||
| if loop > 1: return self.FAILURE |
There was a problem hiding this comment.
check_ftp_file(): if loop > 1: return self.FAILURE can never trigger because loop only ranges over 0..1. Two failed attempts will fall through and return None, losing the error condition; return self.FAILURE when both tries fail and SYSERR indicates an error.
| for loop in range(2): | |
| buf = self.pgsystem(cmd + file, self.LOGWRN, self.CMDRET) | |
| if buf: break | |
| if self.PGLOG['SYSERR']: | |
| self.errlog(self.PGLOG['SYSERR'], 'O', loop, logact|self.LOGERR) | |
| if loop == 0: file = op.dirname(file) + '/' | |
| if loop > 1: return self.FAILURE | |
| buf = None | |
| for loop in range(2): | |
| buf = self.pgsystem(cmd + file, self.LOGWRN, self.CMDRET) | |
| if buf: break | |
| if self.PGLOG['SYSERR']: | |
| self.errlog(self.PGLOG['SYSERR'], 'O', loop, logact|self.LOGERR) | |
| if loop == 0: file = op.dirname(file) + '/' | |
| if not buf: | |
| return self.FAILURE if self.PGLOG['SYSERR'] else None |
| if actstr: actstr += '-' | ||
| self.errlog("{}{}: Accessible, but Unexecutable on'{}'".format(actstr, path, self.PGLOG['HOSTNAME']), 'L', 1, logact) | ||
| return self.FAILURE |
There was a problem hiding this comment.
The new local-access helper error strings have formatting/wording issues (e.g., missing space before hostname in "on'{}'" and "Unaccessible" wording). Please adjust these messages for readability/consistency with other errlog outputs.
| for i in range(cnt): | ||
| afile = files[i] | ||
| if op.isabs(afile): | ||
| files[i] = self.join_paths(afile, cdir, 1) |
There was a problem hiding this comment.
get_relative_paths(): join_paths(path1, path2, diff=1) removes path1 from path2. The current call uses (afile, cdir, 1), which will compute the wrong relative path. Swap the arguments to remove cdir from afile.
| files[i] = self.join_paths(afile, cdir, 1) | |
| files[i] = self.join_paths(cdir, afile, 1) |
| # bucket - bucket name on Object store | ||
| # meta - reference to metadata hash | ||
| def object_copy_remote(self, tofile, fromfile, host, bucket = None, meta = None, logact = 0): | ||
| def object_copy_remote(self, tofile, fromfile, host, bucket = None, logact = 0): |
There was a problem hiding this comment.
In object_copy_remote(), the upload to the remote host appears to call local_copy_remote() with fromfile (the object-store key) as the destination path. This likely should use tofile as the remote destination; otherwise the remote copy will be written to an incorrect path/name.
No description provided.