Handle new, changed and deleted symlinks in git.

Review Request #8785 - Created Feb. 28, 2017 and submitted

Erik Johansson
Review Board
master
a88cf7f...
reviewboard
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.

  • 0
  • 0
  • 16
  • 1
  • 17
Description From Last Updated
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/scmtools/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/scmtools/git.py
    
    
  2. 
      
Erik Johansson
Erik Johansson
Christian Hammond
  1. 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.

  2. reviewboard/diffviewer/diffutils.py (Diff revision 3)
     
     

    Since this won't necessarily be in extra_data, you'll need to change this to filediff.extra_data.get('symlink') or False.

    1. Isn't that what the second argument to get is there for?

    2. Is get('symlink', False) ok Christian or should I change to get('symlink') or False?

    3. I'm not entirely sure what I was originally thinking here. I was reviewing this while dealing with some other stuff. Please ignore it.

  3. 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 reserve symlink for additional details, if we end up with a use for that from some SCM.

  4. reviewboard/diffviewer/parser.py (Diff revision 3)
     
     
     
     
     

    Good cleanup.

    For memory savings, let's use tuples instead of lists here.

  5. 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)
    
  6. reviewboard/scmtools/git.py (Diff revision 3)
     
     
     

    Not part of your change, but mind reversing these while you're here? Ordering is wrong.

  7. 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.
    """

  8. reviewboard/scmtools/git.py (Diff revision 3)
     
     
     

    Blank line between these.

    We also use m for regex match variable names.

  9. 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
    
  10. reviewboard/scmtools/git.py (Diff revision 3)
     
     
     

    Blank line here.

  11. 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.

  12. 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 perform stat.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
    
  13. 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?

    1. Added it to a couple of other tests.

  14. 
      
Erik Johansson
Review request changed

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:

   

Unit test.

   
~  

Tested with a patch adding a symlink and changing another symlink. The files were properly marked with " (symlink)" in th UI.

  ~

(TODO: perform test with latest code) Tested with a patch adding a symlink and changing another symlink. The files were properly marked with " (symlink)" in the UI.

Commit:

-77b51e19b430438a2faedb1896c17f16d805fb9e
+64d4fdb83db852fe2d6d4806d26e92fdb9cc5340

Diff:

Revision 4 (+150 -19)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Erik Johansson
Erik Johansson
Erik Johansson
Christian Hammond
  1. Just these two things left and it'll land for RC1. (It's officially on our radar for release.)

  2. 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.

  3. reviewboard/scmtools/git.py (Diff revision 6)
     
     

    Can you rename this to FILE_MODE_RE?

  4. 
      
Erik Johansson
Christian Hammond
  1. Ship It!
  2. 
      
David Trowbridge
  1. Ship It!
  2. 
      
Erik Johansson
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (0719f97)
Loading...