Tool: Pyflakes
Processed Files:
    reviewboard/scmtools/git.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/scmtools/git.py

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.

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

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 = attr_value.decode('utf-8')

setattr(file, attr, 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 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.

"""

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 stat.S_ISLNK(mode):
        mode = ...
        patch = patch[...] + ...

return patch

Blank line here.

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.

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

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?

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

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?

Ship It!

Ship It!