Handle symlinks better

[csilvers]

Right now, if you have two files in the current tree, one of which is a symlink to the other, slicker will try to apply the rewrite rules to both files, which means it will try to modify a single file two times. At best we'll get bad results, at worst we'll probably crash. We should deal with symlinks better.

I feel there are two fundamentally different use-cases we have to deal with: one is a symlink being given as input, and one is looking at the list of files to operate on when applying the fix-uses suggestor.

The first case, I think we just want to disallow the input (alternately we could follow the symlink and work on that file). The second case, I think we want to resolve the symlink and work on that file (uniquifying first so we don't work on the same file twice).

In each case we have to think about what we do when the resolved-symlink points outside our tree. Probably we die for inputs, and warn-and-ignore for suggestors.

What I'm struggling with is I don't know a good way to distinguish the fix-uses suggestor from the other suggestors. In practice, the distinction is meaningful because a) fix-uses suggestor always modifies a file in place, while the other suggestors can move a file around (i.e. add/delete files), and b) the fix-uses suggestor can work on an arbitrary file in our tree, while the other suggestors always only work on one of the files specified on the commandline. But both those facts are implicit in the suggestors.

This suggests another place to try to do this: in _run_suggestor_on_file. The idea here is this: for every file in patches_by_file, we store whether a) it's a symlink to another file, or b) another file in patches_by_file symlinks to it. (Ideally we'd want to know if any file on the filesystem symlinks to it, but that's too hard.) Then for each file we'd look at its patches. If any patch would delete the file, we error-exit if the file is a symlink, and warn if the file is pointed to by a symlink. If all the patches are modifications, and the file is a symlink, we see if it's a symlink to another file that's in files_to_patch. If so, we silently ignore it. If not, but it points to another file in our tree (that is, another file that the path_filter would return), we normalize the filename to point to the destination and apply the patch there. Perhaps we warn as well. If it points outside our tree, we ignore it and warn.

I'm not sure what we want the behavior to be when the symlink is to a file that's in our tree but is explicitly excluded by the path-filter. This falls into the case "file is a symlink, not a symlink to another file in files_to_patch, but is another file in our tree." Perhaps we apply the patch but warn.

I think it's the most principled solution we've got. It's pretty complicated though, and I can't convince myself it does the right thing all the time. Maybe we should go the sed -i/perl -i route and just say: 'it's broken, don't do that,' and call it a day. :-)

[csilvers]

@benjaminjkraft 's feedback:

Yes, great point. I like just disallowing the input -- it's very unclear what the user would expect in this case since the file could be referenced by either/both names. (If this is ever a problem we can have a force flag, but I doubt it will be. Possibly --no-automove should disable it, too.) Warn and ignore seems right for symlinks outside the tree; touching anything outside the tree strikes me as dangerous since it's more likely to, say, not be in git.

I don't think I've totally wrapped my head around how this would look, but the distinction between deletions and edits seems like a good one. The only problem is that in some cases where we delete a file, we really just blank(ish) it and then the cleanup suggestor(s) delete it. So I don't know how we handle that -- it seems like stopping after the diffs but before the deletion is the worst case here.
It seems to me -- and I haven't fully thought this through -- like the proposed behavior could be done in more or less the approach you already took, too. I think that's the one that makes the most sense to me; resolving the inputs or in make_fixes is certainly where the "die if the moved file is a symlink" behavior would belong, at least. But again, I haven't fully thought through either approach so I'm just speculating here.

FWIW it looks like rope just skips over symlinks.