- Change Summary:
-
Bump? Updating this patch to be based on the release-0.5 tag. It's available from... https://github.com/atagar/rbtools/tree/p4_print_symlinks https://github.com/atagar/rbtools/commit/a96a6923e680f9d08b50164e19e05dfd3460add6 I've also expanded the patch a bit to handle 'p4 add' of symlinks to non-existant files (which similary caused things to choke)... % ln -s /tmp/foaodfnao bad_symlink % p4 add bad_symlink % post-review Traceback (most recent call last): File "/share/rbtools/postreview.py", line 1428, in <module> main() File "/share/rbtools/postreview.py", line 1366, in main diff, parent_diff = tool.diff(args) File "/share/rbtools/clients/perforce.py", line 292, in diff return self._changenum_diff(changenum) File "/share/rbtools/clients/perforce.py", line 628, in _changenum_diff ignore_unmodified=True) File "/share/rbtools/clients/perforce.py", line 655, in _do_diff new_file = _normalize_newlines(new_file) File "/share/rbtools/clients/perforce.py", line 793, in _normalize_newlines diff_file = open(source_path) IOError: [Errno 2] No such file or directory: '/my/repo/bad_symlink' There's plenty of other use cases where bad symlinks can cause postreview.py to choke, but this covers a couple of the most common ones. - Diff:
-
Revision 2 (+30 -2)
Skip files where 'p4 print' provides a symlink
Review Request #3221 — Created July 16, 2012 and submitted
The riddle: how can a file both exist and not exist? The answer: symlinks!
When a symlink is checked into perforce running 'p4 print -o' will create a
symlink, not the file that it points to. This naturally makes postreview.py
choke...
~% post-review
Traceback (most recent call last):
File "/share/rbtools/rbtools/postreview.py", line 4115, in <module>
main()
File "/share/rbtools/rbtools/postreview.py", line 4061, in main
diff, parent_diff = tool.diff(args)
File "/share/rbtools/rbtools/postreview.py", line 2002, in diff
return self._changenum_diff(changenum)
File "/share/rbtools/rbtools/postreview.py", line 2299, in _changenum_diff
self._write_file(old_depot_path, tmp_diff_from_filename)
File "/share/rbtools/rbtools/postreview.py", line 2475, in _write_file
os.chmod(tmpfile, stat.S_IREAD | stat.S_IWRITE)
OSError: [Errno 2] No such file or directory: '/tmp/tmpsCS-fw'
Resolving symlinks, especially those with a relative path would be tricky.
Also, there's no certainty that the file it points to is even in version
control. Hence I'm just dropping it from the review with a warning.
Tested this change both when reviewing a single symlink file and a batch of files containing a symlink. We've been using this change against RBTools 0.3. I've adapted the change for the current master but this copy of it hasn't been exercised.
AT
AT
- Change Summary:
-
On second thought this is trivial to apply to other operations even if I don't have a repro for those cases on hand.
- Diff:
-
Revision 3 (+50 -12)
-
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: rbtools/clients/perforce.py Ignored Files:
AT
- Change Summary:
-
Logging instead of printing... % post-review WARNING:root:Skipping symlink (/workplace/MyTestRepository/dummy_symlink) Review request #1394 posted. https://cr.someplace.com/r/1394/ Btw, is there interest upstream in changing the logging format? The readability of the debug output for RBTools took a substantial turn for the worse between 0.3 and 0.5 imho... RBTools-0.3 looked something like... >>> Running: p4 opened -c default >>> Processing edit of //MyTestPackage/mainline/INSTALL >>> Writing "//MyTestPackage/mainline/INSTALL#5" to "/tmp/tmpeUkZfw" >>> Running: p4 print -o /tmp/tmpeUkZfw -q //MyTestPackage/mainline/INSTALL#5 RBTools-0.5 uses the default logging configuration, which looks like... DEBUG:root:Running: p4 opened -c default DEBUG:root:Processing edit of //MyTestPackage/mainline/INSTALL DEBUG:root:Writing "//MyTestPackage/mainline/INSTALL#5" to "/tmp/tmpeUkZfw" DEBUG:root:Running: p4 print -o /tmp/tmpeUkZfw -q //MyTestPackage/mainline/INSTALL#5 You can easily change this by adding something like... logging.basicConfig( format = '%(asctime)s [%(levelname)s] %(message)s', datefmt = '%m/%d/%Y %H:%M:%S', ) ... to the top of postreview.py to get... 05/16/2013 14:01:23 [DEBUG] Running: p4 opened -c default 05/16/2013 14:01:24 [DEBUG] Processing edit of //MyTestPackage/mainline/INSTALL 05/16/2013 14:01:24 [DEBUG] Writing "//MyTestPackage/mainline/INSTALL#5" to "/tmp/tmpOEgqJp" 05/16/2013 14:01:24 [DEBUG] Running: p4 print -o /tmp/tmpOEgqJp -q //MyTestPackage/mainline/INSTALL#5 If a patch for this would be of interest then let me know and I'll send it.
- Diff:
-
Revision 4 (+50 -12)
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: rbtools/clients/perforce.py Ignored Files: