Handle new, changed and deleted symlinks in git.
Review Request #8785 — Created Feb. 28, 2017 and submitted
Mark up file diffs that add, change or remove a symlink so that a label can be shown in the UI to indicate that the file is a symlink. Also normalize a git patch containing a symlink to make the file appear as a regular file, otherwise patch fails to apply the diff, complaining about the file not being a symlink.
Unit test.
Tested with a patch adding a symlink and changing another symlink. The files
were properly marked with " (symlink)" in the UI.
Description | From | Last Updated |
---|---|---|
Since this won't necessarily be in extra_data, you'll need to change this to filediff.extra_data.get('symlink') or False. |
|
|
While here, mind splitting this across a few lines, like: filediff.extra_data = { 'symlink': f.symlink, } That'll make it easier … |
|
|
Good cleanup. For memory savings, let's use tuples instead of lists here. |
|
|
This will be a bit cheaper and more readable if you do: attr_value = info.get(attr) if isinstance(attr_value, six.binary_type): attr_value = … |
|
|
Not part of your change, but mind reversing these while you're here? Ordering is wrong. |
|
|
This will need to use our modern docstring format, which would look like: ```python """Normalize the provided patch file. This … |
|
|
Blank line between these. We also use m for regex match variable names. |
|
|
I think this would be cleaner with only one return statement: m = ... if m: mode = ... if … |
|
|
Blank line here. |
|
|
Single quotes for the string. Also, can you use format strings for the entire thing here: patch = b'%s%o%s' % … |
|
|
We don't use the if .. else form in our code, instead opting to be more verbose. In this case, … |
|
|
Thanks for all the unit tests! Much appreciated :) Can you modify the other tests to assertFalse(f.symlink), so that we … |
|
|
E222 multiple spaces after operator |
![]() |
|
F821 undefined name 'match' |
![]() |
|
F821 undefined name 'match' |
![]() |
|
This should have a trailing comma after. That makes it cleaner to add new content without modifying this line. |
|
|
Can you rename this to FILE_MODE_RE? |
|
Change Summary:
New draft that besides normalizing the patch so that it can be applied, also marks the symlink as such in the UI.
Summary: |
|
||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||
Diff: |
Revision 2 (+99 -19) |
Checks run (2 succeeded, 1 failed with error)
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+99 -19) |
Checks run (2 succeeded, 1 failed with error)
-
Thank you for your patience and gentle prodding. Let's get this change in!
I have a handful of nits, mostly around our coding/doc standards and some alternative ways parts can be written. The overall approach seems sound to me, though.
-
reviewboard/diffviewer/diffutils.py (Diff revision 3) Since this won't necessarily be in
extra_data
, you'll need to change this tofilediff.extra_data.get('symlink') or False
. -
reviewboard/diffviewer/managers.py (Diff revision 3) While here, mind splitting this across a few lines, like:
filediff.extra_data = { 'symlink': f.symlink, }
That'll make it easier to add to things later.
Also, mind changing the name (here and in other places) to
is_symlink
? Our older metadata doesn't do this, but it would allow us to reservesymlink
for additional details, if we end up with a use for that from some SCM. -
reviewboard/diffviewer/parser.py (Diff revision 3) Good cleanup.
For memory savings, let's use tuples instead of lists here.
-
reviewboard/diffviewer/parser.py (Diff revision 3) This will be a bit cheaper and more readable if you do:
attr_value = info.get(attr) if isinstance(attr_value, six.binary_type): attr_value = attr_value.decode('utf-8') setattr(file, attr, attr_value)
-
reviewboard/scmtools/git.py (Diff revision 3) Not part of your change, but mind reversing these while you're here? Ordering is wrong.
-
reviewboard/scmtools/git.py (Diff revision 3) This will need to use our modern docstring format, which would look like:
```python
"""Normalize the provided patch file.This will make new, changed, and deleted symlinks ...
...
Args:
patch (bytes):
The patch data being posted for review.filename (unicode): The name of the file being modified. revision (unicode): The revision of the file being modified.
Returns:
bytes:
The normalized patch data.
""" -
reviewboard/scmtools/git.py (Diff revision 3) Blank line between these.
We also use
m
for regex match variable names. -
reviewboard/scmtools/git.py (Diff revision 3) I think this would be cleaner with only one return statement:
m = ... if m: mode = ... if stat.S_ISLNK(mode): mode = ... patch = patch[...] + ... return patch
-
-
reviewboard/scmtools/git.py (Diff revision 3) Single quotes for the string.
Also, can you use format strings for the entire thing here:
patch = b'%s%o%s' % (patch[...], mode, patch[...])
That will ensure we have a bytestring at the end, and is more efficient than concatenating.
-
reviewboard/scmtools/git.py (Diff revision 3) We don't use the
if .. else
form in our code, instead opting to be more verbose. In this case, it's not really needed, since we don't even want to performstat.S_ISLNK
if it doesn't match. You can write this as:m = ...search(line) if m: mode = int(m.group('mode'), 8) if stat.S_ISLNK(mode): file_info.symlink = True break
-
reviewboard/scmtools/tests/test_git.py (Diff revision 3) Thanks for all the unit tests! Much appreciated :)
Can you modify the other tests to
assertFalse(f.symlink)
, so that we can verify (now and going forward) that the flag isn't accidentally set?
Change Summary:
Fixed most of Christian's comments. Haven't tested it yet though (except for unit tests) as I couldn't get the test env up and running. Will try again later.
Testing Done: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||
Diff: |
Revision 4 (+150 -19) |
Checks run (1 failed, 1 succeeded)
flake8
Change Summary:
Fix typos.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+150 -19) |
Checks run (2 succeeded)
Change Summary:
Fixed a bug where all files got the symlink status of the first file.
Testing Done: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||
Diff: |
Revision 6 (+152 -19) |
Checks run (2 succeeded)
Summary: |
|
|||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
-
Just these two things left and it'll land for RC1. (It's officially on our radar for release.)
-
reviewboard/diffviewer/managers.py (Diff revision 6) This should have a trailing comma after. That makes it cleaner to add new content without modifying this line.
-
Change Summary:
Fix Christian's comments.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+152 -19) |