New hook 'destroyed-symlinks' #511

m-khvoinitsky · 2020-08-02T19:10:14Z

The hook detects symlinks which are changed to regular files with a content of a path which that symlink was pointing to.
This usually happens on Windows in case when user without a permission for creating symlinks clones repository with symlinks.

~~Some implementation notes: bytes is used for filenames in order to avoid having any troubles with encodings. After all, the hook check for destroyed symlinks, not for bad file encoding. ;)~~

P. S. If you don't think that this hook would be useful for general audience, I'm fine with hosting it in the separate repo. Just wanted to try.

m-khvoinitsky · 2020-08-02T22:08:11Z

No idea why coverage report is different for pypy, will investigate it.

m-khvoinitsky · 2020-11-03T18:12:07Z

I've finally managed to find a time to dig into pypy issue. The reason was the following. In CPython in code like this:

print('something', file=open(somefile, 'a'))

file object losts its last reference it this function call and it is destroyed forcing file buffers to flush. So, for some reason, this doesn't happen in pypy. Workaround would be using explicit flush=True or using context manager which I did.
@asottile, please, take a look.

pre_commit_hooks/destroyed_symlinks.py

asottile · 2020-11-03T18:15:16Z

pre_commit_hooks/destroyed_symlinks.py

+    ).split(b'\0'):
+        splitted = line.split(b' ')
+        if splitted and splitted[0] == ORDINARY_CHANGED_ENTRIES_MARKER:
+            # variable names are taken from


these variable names are terrible, I don't particularly care if they match upstream let's make them readable

asottile · 2020-11-03T18:15:28Z

pre_commit_hooks/destroyed_symlinks.py

+import sys
+from operator import methodcaller
+from subprocess import check_call
+from subprocess import check_output


the rest of the project prefers not to from import from the standard library

asottile · 2020-11-03T18:15:46Z

pre_commit_hooks/destroyed_symlinks.py

+def find_destroyed_symlinks(autofix: bool) -> Sequence[bytes]:
+    destroyed_links = []
+    for line in check_output(
+        ['git', 'status', '--porcelain=v2', '-z'],


https://github.com/asottile/scratch/wiki/Prefer-tuples-to-lists

pre_commit_hooks/destroyed_symlinks.py

asottile · 2020-11-03T18:18:43Z

pre_commit_hooks/destroyed_symlinks.py

+                    # by something like trailing-whitespace and/or
+                    # mixed-line-ending hooks so we need to go deeper
+                    index_size = int(
+                        check_output(['git', 'cat-file', '-s', hI]).strip(),


there's a cmd_output helper in pre_commit_hooks/util.py that probably makes most of this easier to read

kwargs.setdefault('stderr', subprocess.PIPE)

This eats stderr output which is probably not the best idea.

that's just the default, you can configure it otherwise. and actually it's probably better as a pipe so the errors only display when the tool breaks

asottile · 2020-11-03T18:19:15Z

pre_commit_hooks/destroyed_symlinks.py

+                    index_size = int(
+                        check_output(['git', 'cat-file', '-s', hI]).strip(),
+                    )
+                    # Most filesystems limit path length to 4096 bytes.


windows allows paths longer than this with UNC naming

Well, there definitely should be a limit in order to avoid wasting RAM in case when symlink was deliberately replaced with big file, which, on the other hand is not good idea in git so I don't know what is better.

you can probably limit the read to the original symlink destination filename's length (that's how windows usually represents these on disk anyway iirc)

you can probably limit the read to the original symlink destination filename's length

Plus a few bytes for various newlines. That's a good idea.

asottile · 2020-11-03T18:19:50Z

pre_commit_hooks/destroyed_symlinks.py

+                    if autofix:
+                        check_call([
+                            'git',
+                            'update-index',


pre-commit's stance is to never modify the staging area, I believe this does that so it can't be allowed -- printing this command seems fine though

Actually, update-index is redundant here and even harmful — if the file was modified by some other hooks, symlinks would be broken. Instead, I should just unstage the file. Is this allowed?

I would just leave it and print a command for the user to run

Even if this is opt-in feature (which it is)? May be rename --autofix to --unstage to make it more explicit?
Printing the command (and may be some help) if --autofix is not enabled is good idea though.

there's kinda two problems:

pre-commit won't notice the differences

it might get auto committed when wrong

if there's a way to modify the file to look like it's the symlink again in the unstaged portion that would be best

I've removed --autofix, now only the message with an instruction is displayed.

if there's a way to modify the file to look like it's the symlink again in the unstaged portion that would be best

If it was possible, git would have done this. That's a problem.

asottile · 2020-11-03T18:20:23Z

.pre-commit-hooks.yaml

+    entry: destroyed-symlinks
+    language: python
+    types: [file]
+    pass_filenames: false


this should still take filenames and filter the results to that -- that way during merge conflict resolution if someone else makes a mistake this doesn't blame the user performing the merge

asottile · 2020-11-03T18:21:53Z

tests/destroyed_symlinks_test.py

+    source_repo = tmpdir.join('src')
+    os.makedirs(source_repo, exist_ok=True)
+    test_repo = tmpdir.join('test')
+    with source_repo.as_cwd():


note that you can use git -C source_repo ... instead of as_cwd()

I know but this way it's a bit more readable IMO.

asottile · 2020-11-18T18:35:54Z

pre_commit_hooks/destroyed_symlinks.py

+            *itertools.chain(
+                ('git', 'status', '--porcelain=v2', '-z', '--'),
+                files,
+            ),


this can be:

cmd_output('git', 'status', '--porcelain=v2', '-z', '--', *files)

asottile · 2020-11-18T18:36:02Z

README.md

+Detects symlinks which are changed to regular files with a content of a path which that symlink was pointing to.
+This usually happens on Windows in case when user without a permission for creating symlinks clones repository with symlinks.
+The following argument is available:
+- `--autofix` - unstage detected broken symlinks so they won't be commited.


I believe this option is gone now

asottile · 2020-11-18T18:36:35Z

pre_commit_hooks/destroyed_symlinks.py

+                mode_HEAD == PERMS_LINK,
+                mode_index != PERMS_LINK,
+                mode_index != PERMS_NONEXIST,
+            )):


all(( here is going to be slower than putting and to join these conditions

asottile · 2020-11-18T18:37:33Z

pre_commit_hooks/destroyed_symlinks.py

+
+
+def normalize_content(content: bytes) -> bytes:
+    return content.rstrip()


I'd probably just inline this

asottile · 2020-11-18T18:38:13Z

pre_commit_hooks/destroyed_symlinks.py

+        print('You should unstage affected files:')
+        print(
+            ' git reset HEAD -- {}'.format(
+                ' '.join(map(shlex.quote, destroyed_links)),


please don't use map / filter / reduce -- a list comprehension is more readable

Tastes differ.

asottile · 2020-11-18T18:38:24Z

pre_commit_hooks/destroyed_symlinks.py

+        print(
+            'And retry commit. As a long term solution '
+            'you may try to explicitly tell git that your '
+            'envionment does not support symlinks:',


environment

asottile · 2020-11-18T18:38:35Z

pre_commit_hooks/destroyed_symlinks.py

+            'you may try to explicitly tell git that your '
+            'envionment does not support symlinks:',
+        )
+        print(' git config --local core.symlinks false')


iirc --local is the default for this command

asottile · 2020-11-18T18:39:21Z

pre_commit_hooks/util.py

@@ -13,7 +15,11 @@ def added_files() -> Set[str]:
    return set(cmd_output(*cmd).splitlines())


-def cmd_output(*cmd: str, retcode: Optional[int] = 0, **kwargs: Any) -> str:
+def cmd_output(
+    *cmd: Union[str, bytes],


mixing here used to have problems on windows, I'm not sure if it's still a problem in python3 though

Reverted. It was needed in the previous version where everything was bytes. I don't have Windows nearby to check at the moment, however, tests weren't failing.
They use os.fsdecode (subprocess.py#L561) which, quoting the doc, decode the path-like filename from the filesystem encoding with 'surrogateescape' error handler, or 'strict' on Windows; return str unchanged (good bad example of mixing str and bytes).

pre_commit_hooks/destroyed_symlinks.py

… regular files with a content of a path which that symlink was pointing to; move zsplit to util

asottile

m-khvoinitsky · 2020-11-18T20:00:21Z

Thanks!

m-khvoinitsky force-pushed the master branch from 6bb4c2a to 433aa14 Compare August 2, 2020 19:35

m-khvoinitsky force-pushed the master branch from 433aa14 to 7d9da14 Compare November 3, 2020 18:00

asottile reviewed Nov 3, 2020

View reviewed changes

m-khvoinitsky force-pushed the master branch 3 times, most recently from 6a43b59 to 0ba63bb Compare November 18, 2020 17:37

asottile reviewed Nov 18, 2020

View reviewed changes

m-khvoinitsky force-pushed the master branch from 0ba63bb to fe30ede Compare November 18, 2020 19:18

m-khvoinitsky commented Nov 18, 2020

View reviewed changes

pre_commit_hooks/destroyed_symlinks.py Outdated Show resolved Hide resolved

New hook 'destroyed-symlinks' to detect symlinks which are changed to…

1e87d59

… regular files with a content of a path which that symlink was pointing to; move zsplit to util

asottile force-pushed the master branch from 1844158 to 1e87d59 Compare November 18, 2020 19:45

asottile approved these changes Nov 18, 2020

View reviewed changes

asottile merged commit 4b863f1 into pre-commit:master Nov 18, 2020

dependabot bot mentioned this pull request Mar 18, 2021

Bump pre-commit-hooks from 3.3.0 to 3.4.0 hacf-fr/synologydsm-api#37

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New hook 'destroyed-symlinks' #511

New hook 'destroyed-symlinks' #511

m-khvoinitsky commented Aug 2, 2020 •

edited

Loading

m-khvoinitsky commented Aug 2, 2020

m-khvoinitsky commented Nov 3, 2020

asottile Nov 3, 2020

asottile Nov 3, 2020

asottile Nov 3, 2020

asottile Nov 3, 2020

m-khvoinitsky Nov 18, 2020

asottile Nov 18, 2020

asottile Nov 3, 2020

m-khvoinitsky Nov 3, 2020

asottile Nov 3, 2020

m-khvoinitsky Nov 3, 2020

asottile Nov 3, 2020

m-khvoinitsky Nov 3, 2020

asottile Nov 3, 2020

m-khvoinitsky Nov 3, 2020 •

edited

Loading

asottile Nov 3, 2020

m-khvoinitsky Nov 18, 2020

m-khvoinitsky Nov 18, 2020

asottile Nov 3, 2020

asottile Nov 3, 2020

m-khvoinitsky Nov 3, 2020

asottile Nov 18, 2020

asottile Nov 18, 2020

asottile Nov 18, 2020

asottile Nov 18, 2020

asottile Nov 18, 2020

m-khvoinitsky Nov 18, 2020

asottile Nov 18, 2020

asottile Nov 18, 2020

asottile Nov 18, 2020

m-khvoinitsky Nov 18, 2020

asottile left a comment

m-khvoinitsky commented Nov 18, 2020



		def normalize_content(content: bytes) -> bytes:
		return content.rstrip()

New hook 'destroyed-symlinks' #511

New hook 'destroyed-symlinks' #511

Conversation

m-khvoinitsky commented Aug 2, 2020 • edited Loading

m-khvoinitsky commented Aug 2, 2020

m-khvoinitsky commented Nov 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-khvoinitsky Nov 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asottile left a comment

Choose a reason for hiding this comment

m-khvoinitsky commented Nov 18, 2020

m-khvoinitsky commented Aug 2, 2020 •

edited

Loading

m-khvoinitsky Nov 3, 2020 •

edited

Loading