D680: scmutil: handle conflicting files and dirs in origbackuppath

[ryanmce (Ryan McElroy)]

ryanmce accepted this revision.
ryanmce added a comment.


  It looks like all concerns have been addressed. This looks good to me.

INLINE COMMENTS

> mbthomas wrote in scmutil.py:570
> At this point the path is a vfs-path within the repo, so it should be using `/` as a separator.
> 
> In the case of UNC paths, `os.path.relpath` seems to do the right thing, so `os.path.relpath(r'\\?\c:\repo\dir\file', r'\\?\c:\repo')` gives `dir\file`, which `util.normpath` converts to `dir/file`.
> 
> It's a bit of a shame that `scmutil.origpath` takes an absolute path rather than a vfs path within the repo, as it means we have to jump through these hoops.  It doesn't look like we can easily change it, though.

I agree that it seems out of scope to fix this in this patch series. Might be worth opening a task in bugzilla to track improving this, but I think this series is already fairly long and too important to hold up on a small thing like this. Since it works, I think we should move towards shipping it.

> yuja wrote in scmutil.py:574
> It shouldn't delete files out of the origbackuppath directory
> even if the configured path points to (or under) a file.
> 
> Maybe we could use a new vfs dedicated to backup directory to
> audit operations.

@mbthomas and I had talked about adding a new vfs; thanks for pushing us in that direction. Getting a dedicated vfs feels like a good win here.

> scmutil.py:582-585
> +    origvfs = vfs.vfs(repo.wjoin(origbackuppath))
> +    origbackupdir = origvfs.dirname(filepathfromroot)
> +    if not origvfs.isdir(origbackupdir) or origvfs.islink(origbackupdir):
> +        ui.note(_('creating directory: %s\n') % origvfs.join(origbackupdir))

In the future, we may want to move this to the repo class like other `vfs` instances. That will mean we don't have to re-instantiate this each time. However, these instantiations look cheap so I'm not worried and it's outside the scope of this change.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D680

To: mbthomas, ryanmce, #hg-reviewers, durham, yuja, ikostia
Cc: kiilerix, quark, ikostia, yuja, durham, mercurial-devel