Re: [PATCH] update: do not remove cwd

[Gregory Szorc]

On Mon, Aug 29, 2016 at 3:32 PM, Matt Mackall  wrote:

> On Mon, 2016-08-29 at 11:50 -0700, Stanislau Hlebik wrote:
> > # HG changeset patch
> > # User Stanislau Hlebik 
> > # Date 1472496038 25200
> > #  Mon Aug 29 11:40:38 2016 -0700
> > # Node ID e1962781ce84040746ef79c0084b8fd70cfcd4b4
> > # Parent  318e2b600b80e4ed3c6f37df46ec7544f60d4c0b
> > update: do not remove cwd
> >
> > During update directories are deleted as soon as they have no entries.
> > But current working directory shouldn't be deleted because it
> > causes problems for users after hg finishes.
>
> It is a perfectly valid state that POSIX allows. So we shouldn't make those
> problems our problems.
>

Windows does not allow the deletion of opened files. And having
explorer.exe or a command prompt/shell in a directory is enough to maintain
an open file handle. In short, you can't delete cwd on Windows.


>
> >  Also it makes complex commands
> > like 'hg split' fail.
>
> Those failures are also arguably bugs in their own right.
>
> But this approach introduces new complexities. For instance, what happens
> if the
> current directory gets replaced by a symlink? Whether that works depends on
> where you run the command. And how does the not-deleted directory get
> cleaned up
> so as not to interfere with future operations?
>
> If we wanted to disallow this, we'd ideally abort early so the user could
> run
> the operation in a sane location. But that's not generally easy to
> discover. And
> aborting later is likely to make a mess.
>
> (Alternate idea: rename the current directory to
> .oh-no-you-ran-an-hg-command-
> that-deleted-your-current-directory-please-clean-up-your-mess-kthxbye.)
>
> --
> Mathematics is the supreme nostalgia of our time.
>
> ___
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: [PATCH] update: do not remove cwd

[Stanislau Hlebik]

Makes sense. But Durham's idea about warning seems legit. When i ran into this 
problem when doing 'hg split', i spent a few minutes figuring out where the 
problem came from. If i had warning like 'your cwd was deleted; if it causes 
problems go to the root' i'd found the solution sooner.



Sent from Samsung Mobile on O2


 Original message 
From: Matt Mackall <m...@selenic.com>
Date: 30/08/2016 20:23 (GMT+00:00)
To: Stanislau Hlebik <st...@fb.com>, mercurial-devel@mercurial-scm.org
Subject: Re: [PATCH] update: do not remove cwd

On Mon, 2016-08-29 at 11:50 -0700, Stanislau Hlebik wrote:
> # HG changeset patch
> # User Stanislau Hlebik <st...@fb.com>
> # Date 1472496038 25200
> #  Mon Aug 29 11:40:38 2016 -0700
> # Node ID e1962781ce84040746ef79c0084b8fd70cfcd4b4
> # Parent  318e2b600b80e4ed3c6f37df46ec7544f60d4c0b
> update: do not remove cwd
>
> During update directories are deleted as soon as they have no entries.
> But current working directory shouldn't be deleted because it
> causes problems for users after hg finishes.

It is a perfectly valid state that POSIX allows. So we shouldn't make those
problems our problems.

>  Also it makes complex commands
> like 'hg split' fail.

Those failures are also arguably bugs in their own right.

But this approach introduces new complexities. For instance, what happens if the
current directory gets replaced by a symlink? Whether that works depends on
where you run the command. And how does the not-deleted directory get cleaned up
so as not to interfere with future operations?

If we wanted to disallow this, we'd ideally abort early so the user could run
the operation in a sane location. But that's not generally easy to discover. And
aborting later is likely to make a mess.

(Alternate idea: rename the current directory to .oh-no-you-ran-an-hg-command-
that-deleted-your-current-directory-please-clean-up-your-mess-kthxbye.)

--
Mathematics is the supreme nostalgia of our time.

___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: [PATCH] update: do not remove cwd

[Matt Mackall]

On Mon, 2016-08-29 at 11:50 -0700, Stanislau Hlebik wrote:
> # HG changeset patch
> # User Stanislau Hlebik 
> # Date 1472496038 25200
> #  Mon Aug 29 11:40:38 2016 -0700
> # Node ID e1962781ce84040746ef79c0084b8fd70cfcd4b4
> # Parent  318e2b600b80e4ed3c6f37df46ec7544f60d4c0b
> update: do not remove cwd
> 
> During update directories are deleted as soon as they have no entries.
> But current working directory shouldn't be deleted because it
> causes problems for users after hg finishes.

It is a perfectly valid state that POSIX allows. So we shouldn't make those
problems our problems.

>  Also it makes complex commands
> like 'hg split' fail.

Those failures are also arguably bugs in their own right.

But this approach introduces new complexities. For instance, what happens if the
current directory gets replaced by a symlink? Whether that works depends on
where you run the command. And how does the not-deleted directory get cleaned up
so as not to interfere with future operations?

If we wanted to disallow this, we'd ideally abort early so the user could run
the operation in a sane location. But that's not generally easy to discover. And
aborting later is likely to make a mess.

(Alternate idea: rename the current directory to .oh-no-you-ran-an-hg-command-
that-deleted-your-current-directory-please-clean-up-your-mess-kthxbye.)

-- 
Mathematics is the supreme nostalgia of our time.

___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: [PATCH] update: do not remove cwd

[Stanislau Hlebik]

Yes, but it will help users of histedit, split etc.
Maybe we can even combine warning and restoring cwd.



Sent from Samsung Mobile on O2


 Original message 
From: Augie Fackler <r...@durin42.com>
Date: 30/08/2016 18:51 (GMT+00:00)
To: Stanislau Hlebik <st...@fb.com>
Cc: Durham Goode <dur...@fb.com>, mercurial-devel@mercurial-scm.org, Ryan 
McElroy <r...@fb.com>
Subject: Re: [PATCH] update: do not remove cwd

On Tue, Aug 30, 2016 at 1:41 PM, Stanislau Hlebik <st...@fb.com> wrote:
> Yeap, Augie already noted it.
> Warning is probably fine (it’s better than nothing).
> Another option is to try to save cwd before update and try to restore it 
> after.

That won't help the user, since their cwd won't be altered by changes
made in the subprocess.

(Unless you mean "try to recreate cwd as a directory if it appears to
have been removed", in which case that *might* work, but I don't think
so because the directory's inode will change out from under the user
and I think they'll still be in a busted state.)
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: [PATCH] update: do not remove cwd

[Stanislau Hlebik]

Yeap, Augie already noted it.
Warning is probably fine (it’s better than nothing).
Another option is to try to save cwd before update and try to restore it after.

On 8/30/16, 5:45 PM, "Durham Goode"  wrote:



On 8/29/16 11:50 AM, Stanislau Hlebik wrote:
> # HG changeset patch
> # User Stanislau Hlebik 
> # Date 1472496038 25200
> #  Mon Aug 29 11:40:38 2016 -0700
> # Node ID e1962781ce84040746ef79c0084b8fd70cfcd4b4
> # Parent  318e2b600b80e4ed3c6f37df46ec7544f60d4c0b
> update: do not remove cwd
>
> During update directories are deleted as soon as they have no entries.
> But current working directory shouldn't be deleted because it
> causes problems for users after hg finishes. Also it makes complex 
commands
> like 'hg split' fail.
Won't this cause update to fail if the parent directory is to be deleted 
and replaced with a file?  I'd almost err towards just printing a 
warning message when the current working directory is deleted (and 
recommending they cd to the root of their repo).
>
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -1043,7 +1043,7 @@
>   repo.ui.note(_("removing %s\n") % f)
>   audit(f)
>   try:
> -unlink(wjoin(f), ignoremissing=True)
> +unlink(wjoin(f), ignoremissing=True, donotremovecwd=True)
>   except OSError as inst:
>   repo.ui.warn(_("update failed to remove %s: %s!\n") %
>(f, inst.strerror))
> diff --git a/mercurial/posix.py b/mercurial/posix.py
> --- a/mercurial/posix.py
> +++ b/mercurial/posix.py
> @@ -23,6 +23,7 @@
>   from .i18n import _
>   from . import (
>   encoding,
> +osutil,
>   )
>   
>   posixfile = open
> @@ -496,7 +497,31 @@
>   def makedir(path, notindexed):
>   os.mkdir(path)
>   
> -def unlinkpath(f, ignoremissing=False):
> +def removedirs(name, donotremovecwd=False):
> +"""special version of os.removedirs that does not remove symlinked
This mentions symlinks are also affected.  That seems unrelated to the 
purpose of this patch though, and I think we do want to allow symlinks 
to be deleted if their target contains files.
> +directories or junction points if they actually contain files"""
> +if not donotremovecwd:
> +return os.removedirs(name)
> +
> +cwd = os.getcwd()
> +if cwd == name:
> +return
> +if osutil.listdir(name):
> +return
> +os.rmdir(name)
> +head, tail = os.path.split(name)
> +if not tail:
> +head, tail = os.path.split(head)
> +while head and tail:
> +try:
> +if osutil.listdir(head) or cwd == head:
> +return
> +os.rmdir(head)
> +except (ValueError, OSError):
> +break
> +head, tail = os.path.split(head)
> +
> +def unlinkpath(f, ignoremissing=False, donotremovecwd=False):
>   """unlink and remove the directory if it is empty"""
>   try:
>   os.unlink(f)
> @@ -505,7 +530,7 @@
>   raise
>   # try removing directories that might now be empty
>   try:
> -os.removedirs(os.path.dirname(f))
> +removedirs(os.path.dirname(f), donotremovecwd)
>   except OSError:
>   pass
>   
> diff --git a/mercurial/windows.py b/mercurial/windows.py
> --- a/mercurial/windows.py
> +++ b/mercurial/windows.py
> @@ -369,11 +369,14 @@
>   If gid is None, return the name of the current group."""
>   return None
>   
> -def removedirs(name):
> +def removedirs(name, donotremovecwd=False):
>   """special version of os.removedirs that does not remove symlinked
>   directories or junction points if they actually contain files"""
>   if osutil.listdir(name):
>   return
> +cwd = os.getcwd()
> +if donotremovecwd and cwd == name:
> +return
>   os.rmdir(name)
>   head, tail = os.path.split(name)
>   if not tail:
> @@ -382,12 +385,14 @@
>   try:
>   if osutil.listdir(head):
>   return
> +if donotremovecwd and cwd == head:
> +return
>   os.rmdir(head)
>   except (ValueError, OSError):
>   break
>   head, tail = os.path.split(head)
>   
> -def unlinkpath(f, ignoremissing=False):
> +def unlinkpath(f, ignoremissing=False, donotremovecwd=False):
>   """unlink and remove the directory if it is empty"""
>   try:
>   unlink(f)
 

Re: [PATCH] update: do not remove cwd

[Augie Fackler]

On Mon, Aug 29, 2016 at 11:50:28AM -0700, Stanislau Hlebik wrote:
> # HG changeset patch
> # User Stanislau Hlebik 
> # Date 1472496038 25200
> #  Mon Aug 29 11:40:38 2016 -0700
> # Node ID e1962781ce84040746ef79c0084b8fd70cfcd4b4
> # Parent  318e2b600b80e4ed3c6f37df46ec7544f60d4c0b
> update: do not remove cwd

I like it! Good improvement. However, see below for a concern.

[...]

>
> diff --git a/tests/test-update-names.t b/tests/test-update-names.t
> --- a/tests/test-update-names.t
> +++ b/tests/test-update-names.t
> @@ -72,3 +72,13 @@
>$ cd ..
>
>  #endif
> +
> +Test that cwd is not deleted during update
> +  $ hg init r4 && cd r4
> +  $ mkdir dir
> +  $ cd dir
> +  $ echo a > a
> +  $ hg add a
> +  $ hg ci -m "file and dir"
> +  $ hg up -q null
> +  $ cd .

This wants to be 'cd ..'


What happens in this case:

  $ hg init r5 && cd r5
  $ echo foo > foo
  $ hg addr && hg ci -m foo
  $ hg co null
  $ mkdir foo
  $ echo bar > foo/bar
  $ hg addr && hg ci -m foo/bar
  $ cd foo
Move from r1 to r0, which will want to replace cwd with a file
  $ hg co 0

I suspect this is broken somehow, but didn't test it myself.

> ___
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

[PATCH] update: do not remove cwd

[Stanislau Hlebik]

# HG changeset patch
# User Stanislau Hlebik 
# Date 1472496038 25200
#  Mon Aug 29 11:40:38 2016 -0700
# Node ID e1962781ce84040746ef79c0084b8fd70cfcd4b4
# Parent  318e2b600b80e4ed3c6f37df46ec7544f60d4c0b
update: do not remove cwd

During update directories are deleted as soon as they have no entries.
But current working directory shouldn't be deleted because it
causes problems for users after hg finishes. Also it makes complex commands
like 'hg split' fail.

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -1043,7 +1043,7 @@
 repo.ui.note(_("removing %s\n") % f)
 audit(f)
 try:
-unlink(wjoin(f), ignoremissing=True)
+unlink(wjoin(f), ignoremissing=True, donotremovecwd=True)
 except OSError as inst:
 repo.ui.warn(_("update failed to remove %s: %s!\n") %
  (f, inst.strerror))
diff --git a/mercurial/posix.py b/mercurial/posix.py
--- a/mercurial/posix.py
+++ b/mercurial/posix.py
@@ -23,6 +23,7 @@
 from .i18n import _
 from . import (
 encoding,
+osutil,
 )
 
 posixfile = open
@@ -496,7 +497,31 @@
 def makedir(path, notindexed):
 os.mkdir(path)
 
-def unlinkpath(f, ignoremissing=False):
+def removedirs(name, donotremovecwd=False):
+"""special version of os.removedirs that does not remove symlinked
+directories or junction points if they actually contain files"""
+if not donotremovecwd:
+return os.removedirs(name)
+
+cwd = os.getcwd()
+if cwd == name:
+return
+if osutil.listdir(name):
+return
+os.rmdir(name)
+head, tail = os.path.split(name)
+if not tail:
+head, tail = os.path.split(head)
+while head and tail:
+try:
+if osutil.listdir(head) or cwd == head:
+return
+os.rmdir(head)
+except (ValueError, OSError):
+break
+head, tail = os.path.split(head)
+
+def unlinkpath(f, ignoremissing=False, donotremovecwd=False):
 """unlink and remove the directory if it is empty"""
 try:
 os.unlink(f)
@@ -505,7 +530,7 @@
 raise
 # try removing directories that might now be empty
 try:
-os.removedirs(os.path.dirname(f))
+removedirs(os.path.dirname(f), donotremovecwd)
 except OSError:
 pass
 
diff --git a/mercurial/windows.py b/mercurial/windows.py
--- a/mercurial/windows.py
+++ b/mercurial/windows.py
@@ -369,11 +369,14 @@
 If gid is None, return the name of the current group."""
 return None
 
-def removedirs(name):
+def removedirs(name, donotremovecwd=False):
 """special version of os.removedirs that does not remove symlinked
 directories or junction points if they actually contain files"""
 if osutil.listdir(name):
 return
+cwd = os.getcwd()
+if donotremovecwd and cwd == name:
+return
 os.rmdir(name)
 head, tail = os.path.split(name)
 if not tail:
@@ -382,12 +385,14 @@
 try:
 if osutil.listdir(head):
 return
+if donotremovecwd and cwd == head:
+return
 os.rmdir(head)
 except (ValueError, OSError):
 break
 head, tail = os.path.split(head)
 
-def unlinkpath(f, ignoremissing=False):
+def unlinkpath(f, ignoremissing=False, donotremovecwd=False):
 """unlink and remove the directory if it is empty"""
 try:
 unlink(f)
@@ -396,7 +401,7 @@
 raise
 # try removing directories that might now be empty
 try:
-removedirs(os.path.dirname(f))
+removedirs(os.path.dirname(f), donotremovecwd)
 except OSError:
 pass
 
diff --git a/tests/test-update-names.t b/tests/test-update-names.t
--- a/tests/test-update-names.t
+++ b/tests/test-update-names.t
@@ -72,3 +72,13 @@
   $ cd ..
 
 #endif
+
+Test that cwd is not deleted during update
+  $ hg init r4 && cd r4
+  $ mkdir dir
+  $ cd dir
+  $ echo a > a
+  $ hg add a
+  $ hg ci -m "file and dir"
+  $ hg up -q null
+  $ cd .
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel