971525 - Make FileCopier only delete symlinked directories that it needs to replace

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

At the moment, if FileCopier is writing to dest/, and dest/link_to_directory is a symlink to a directory, it will be deleted.  (And not noted as a deleted file or directory.)  This happens even if remove_unaccounted=False.

I propose to make FileCopier more conservative and only remove symlinks to directories that get in its way.  That is, dest/link_to_directory will be deleted if remove_unaccounted=True, or if dest/link_to_directory/foo is a path in the container, but will be left alone otherwise.

Conceptually, this treats dest/link_to_directory like a file that will be deleted and replaced with a directory if it is a directory containing a path.

Comment 1

[Nick Alexander :nalexander [he/him]]

This springs out of https://bugzilla.mozilla.org/show_bug.cgi?id=853045#c42 and https://bugzilla.mozilla.org/show_bug.cgi?id=853045#c51.

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: Make FileCopier only delete symlinked directories that it needs to replace. r=gps

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 8374617 [details] [diff] [review]
Make FileCopier only delete symlinked directories that it needs to replace. r=gps

Review of attachment 8374617 [details] [diff] [review]:
-----------------------------------------------------------------

Please put this behind a flag and add comments indicating it should only be used in exceptional circumstances.

This code was written to deal with files, not directories. Tricking things into believing a directory symlink is a file is a nice hack. But it seems fragile to me.

If you want to do this right, I'd add a new registry entry for directory symlinks. But that's a lot of code. I don't think you want to write that now. Best to hack around it with an optional, non-default flag to unblock you methinks.

Comment 4

[Nick Alexander :nalexander [he/him]]

(In reply to Gregory Szorc [:gps] from comment #3)
> Comment on attachment 8374617 [details] [diff] [review]
> Make FileCopier only delete symlinked directories that it needs to replace.
> r=gps
> 
> Review of attachment 8374617 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please put this behind a flag and add comments indicating it should only be
> used in exceptional circumstances.

Will do.

> This code was written to deal with files, not directories. Tricking things
> into believing a directory symlink is a file is a nice hack. But it seems
> fragile to me.
>
> If you want to do this right, I'd add a new registry entry for directory
> symlinks. But that's a lot of code. I don't think you want to write that
> now. Best to hack around it with an optional, non-default flag to unblock
> you methinks.

It is my understanding that on disk, a symlink is a file, no matter what it points to.  That's why stat and lstat both exist.  To my eye the File instance for a directory symlink would just be the code for a regular symlink.  It's the same, since the resulting file is the same, and the resulting over-write is the same.

But I just want movement, so I'll put this behind a flag.  Thanks for looking at it!

Comment 5

[Nick Alexander :nalexander [he/him]]

Attachment: Optionally make FileCopier only delete symlinked directories it needs to replace. r=gps

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: Optionally make FileCopier only delete symlinked directories it needs to replace. r=gps

Forget to fold small fixes in, and updated that comment I flagged.

Comment 7

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/620d43cffffe

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/620d43cffffe