Summary

Skip files where 'p4 print' provides a symlink

Review Request #3221 — Created July 16, 2012 and submitted Aug. 9, 2013, 3:20 p.m.

Information

Owner

atagar

Repository

RBTools

Branch

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

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.

Testing Done

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.

Issues

Description	From	Last Updated
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	May 9, 2013, 5:45 p.m.
This should probably either use logging.warning() or sys.stderr.write()	david	May 16, 2013, 2:16 p.m.
Same here.	david	May 16, 2013, 2:16 p.m.
And here.	david	May 16, 2013, 2:16 p.m.
Last one.	david	May 16, 2013, 2:16 p.m.

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)

Show changes

rbtools/clients/perforce.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    rbtools/clients/perforce.py
  Ignored Files:

rbtools/clients/perforce.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```

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)

Show changes

rbtools/clients/perforce.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    rbtools/clients/perforce.py
  Ignored Files:

rbtools/clients/perforce.py (Diff revision 3)

The issue has been resolved. Show all issues

This should probably either use logging.warning() or sys.stderr.write()

chipx86 May 11, 2013, 12:05 a.m.

Probably the former. We're trying to get rid of all direct stdout/stderr usage in the clients.

rbtools/clients/perforce.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Same here.
```
rbtools/clients/perforce.py (Diff revision 3)
The issue has been resolved. Show all issues
```
And here.
```
rbtools/clients/perforce.py (Diff revision 3)
```
And here...
```
rbtools/clients/perforce.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Last one.
```

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)

Show changes

rbtools/clients/perforce.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    rbtools/clients/perforce.py
  Ignored Files:

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to master (8e72821). Thanks!