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
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
