darcs

Issue 2139 darcs mv broken when target is a directory

Title darcs mv broken when target is a directory
Priority bug Status resolved
Milestone 2.8.0 Resolved in 2.10.0 HEAD
Superseder Nosy List galbolle, ganesh, kowey, mndrix
Assigned To ganesh
Topics Regression, UI

Created on 2012-02-06.11:52:05 by kowey, last changed 2012-04-13.20:30:43 by noreply.

Files
File name Uploaded Type Edit Remove
failing-issue2139-mv-to-dir.sh kowey, 2012-02-11.14:26:50 application/octet-stream
Messages
msg15093 (view) Author: kowey Date: 2012-02-06.11:52:03
Uh-oh.  I think I've some dogfood stuck in my throat.  Luckily it's not 
a "deep" problem, UI-level with easy workaround, but probably important 
to fix for the release :-(

These commands did funny things because in all cases the target was a 
directory:

# . is the project root
darcs mv src/genibatch .

darcs failed:  The filename  is not valid under Windows.

# . is some subdirectory
darcs mv ../etc/macstuff .

darcs failed:  A file or dir named geni-wx (or perhaps differing only in 
case)
already exists in working directory.
Use --case-ok to allow files differing only in case.

mkdir src
darcs add src
darcs mv NLP src/
# sorry I lost the error message here

Things I don't know: is this in the beta? Maybe I just have an old 
version of darcs? (2.7.3, sorry!).  To reproduce.
msg15094 (view) Author: kowey Date: 2012-02-06.11:52:39
(And I noticed that darcs-2.5 did the right sorts of things here. Kinda 
funny, would have thought this sort of thing would have been covered in 
our functional tests by now)
msg15100 (view) Author: mulander Date: 2012-02-09.23:00:04
Could not reproduce on Windows Vista using powershell with darcs 2.7.98.3 (+ 1 patch)
Note I used 'darcs' at first (2.5.2 (release)) and later redid the test with darcs.exe which is the freshly built one.

PS D:\darcs\branch-2.8\t> .\darcs.exe init
WARNING: creating a nested repository.
PS D:\darcs\branch-2.8\t> ls


    Directory: D:\darcs\branch-2.8\t


Mode                LastWriteTime     Length Name
----                -------------     ------ ----
d----        2012-02-09     23:38            _darcs
-a---        2012-02-09     23:35   16915650 darcs.exe


PS D:\darcs\branch-2.8\t> touch a.txt
PS D:\darcs\branch-2.8\t> .\darcs.exe add .\a.txt
PS D:\darcs\branch-2.8\t> .\darcs.exe record
Each patch is attributed to its author, usually by email address (for
example, `Fred Bloggs <fred@example.net>').  Darcs could not determine
your email address, so you will be prompted for it.

Your address will be stored in _darcs/prefs/author.
It will be used for all patches recorded in this repository.
If you move that file to %APPDATA%\darcs\author, it will be used for patches
you record in ALL repositories.
What is your email address? Adam Wolk <netprobe@gmail.com>
addfile ./a.txt
Shall I record this change? (1/1)  [ynW...], or ? for more options: y
What is the patch name? empty file
Do you want to add a long comment? [yn]n
Finished recording patch 'empty file'
PS D:\darcs\branch-2.8\t> ls


    Directory: D:\darcs\branch-2.8\t


Mode                LastWriteTime     Length Name
----                -------------     ------ ----
d----        2012-02-09     23:55            _darcs
-----        2012-02-09     23:38          0 a.txt
-a---        2012-02-09     23:35   16915650 darcs.exe


PS D:\darcs\branch-2.8\t> mkdir txt


    Directory: D:\darcs\branch-2.8\t


Mode                LastWriteTime     Length Name
----                -------------     ------ ----
d----        2012-02-09     23:55            txt


PS D:\darcs\branch-2.8\t> darcs add txt
PS D:\darcs\branch-2.8\t> darcs record
adddir ./txt
Shall I record this change? (1/1)  [ynW...], or ? for more options: y
What is the patch name? Adding a directory
Do you want to add a long comment? [yn]n
Finished recording patch 'Adding a directory'
PS D:\darcs\branch-2.8\t> darcs mv .\a.txt .\txt
PS D:\darcs\branch-2.8\t> ls .\txt


    Directory: D:\darcs\branch-2.8\t\txt


Mode                LastWriteTime     Length Name
----                -------------     ------ ----
-a---        2012-02-09     23:38          0 a.txt


PS D:\darcs\branch-2.8\t> darcs mv .\txt\a.txt .
PS D:\darcs\branch-2.8\t> ls


    Directory: D:\darcs\branch-2.8\t


Mode                LastWriteTime     Length Name
----                -------------     ------ ----
d----        2012-02-09     23:56            txt
d----        2012-02-09     23:56            _darcs
-a---        2012-02-09     23:38          0 a.txt
-a---        2012-02-09     23:35   16915650 darcs.exe


PS D:\darcs\branch-2.8\t>

PS D:\darcs\branch-2.8\t> .\darcs.exe mv .\a.txt .\txt
PS D:\darcs\branch-2.8\t> ls .\txt


    Directory: D:\darcs\branch-2.8\t\txt


Mode                LastWriteTime     Length Name
----                -------------     ------ ----
-a---        2012-02-09     23:38          0 a.txt



PS D:\darcs\branch-2.8\t> .\darcs.exe mv .\txt\a.txt .
PS D:\darcs\branch-2.8\t> ls


    Directory: D:\darcs\branch-2.8\t


Mode                LastWriteTime     Length Name
----                -------------     ------ ----
d----        2012-02-09     23:59            txt
d----        2012-02-09     23:59            _darcs
-a---        2012-02-09     23:38          0 a.txt
-a---        2012-02-09     23:35   16915650 darcs.exe
msg15101 (view) Author: ganesh Date: 2012-02-09.23:00:38
I also can't repro from linux
msg15102 (view) Author: ganesh Date: 2012-02-09.23:10:01
(I tested with 2.8 beta 3)
msg15104 (view) Author: mulander Date: 2012-02-10.00:11:49
Were the tests done on Mac OS X?
I think there are some interesting hints in the first error message:

"darcs failed:  The filename  is not valid under Windows."

1. The error is raised from Move.hs from line:

152:      fail $ "The filename " ++ new ++ " is not valid under Windows.\n" ++

Notice ++ new ++ seeing that the messages in your output are only 2 spaces between filename and this indicates that new 
was equal "" at that point in time.

The path validation is performed by System.FilePath.Windows.isValid.

The result for an empty string passed to that function is False as proved by this test:

import qualified System.FilePath.Windows as WindowsFilePath

main = (putStrLn (show (WindowsFilePath.isValid "")))

PS D:\darcs\tests> ghc -o path .\path.hs
[1 of 1] Compiling Main             ( path.hs, path.o )
Linking path.exe ...
PS D:\darcs\tests> .\path.exe
False

The question is why would the path be "" here.

2. Maybe the path is actually expanded with the directory separator which for OS X would be ":" which is an invalid 
component of a file name on MS Windows. Changing the above test to isValid ":" will also result in False being printed.

This of course should not be possible as the output indicates that the 'new' is actually equal "" - correct?

Having the issue appear on OS X only would make the 2 possibility more plausible.
msg15109 (view) Author: kowey Date: 2012-02-11.14:26:50
Attaching possible test for it (I forget if I was ready to actually send 
the test in as a patch and just want to get this out there before I 
forget).

darcs annotate suggests that this may be related to our posthoc move 
work
Attachments
msg15116 (view) Author: kowey Date: 2012-02-18.21:14:43
OK, crucial missing detail from me: the files being moved were 
directories (sorry!).  Also, I was on a Mac, but I suspect this is not 
Mac specific at all

I think we have some test cases here, so pushing up to need-design 
(rename to need-diagnosis-or-design?)

I'd done an informal spot-diagnosis, but didn't mention it to anybody, 
except for vaguely mumbling about darcs annotate:
http://irclog.perlgeek.de/darcs/2012-02-18#i_5170747

Ganesh seems to have a clearer understanding, which is that it's more to 
do with some refactors from Alexey
msg15124 (view) Author: ganesh Date: 2012-02-18.22:34:53
I think the source of this bug is this cleanup patch:

Tue Mar 16 22:02:11 GMT 2010  Alexey Levan <exlevan@gmail.com>
  * Separate argument handling from repository work in Move.lhs

Unfortunately it's actually hard to separate the argument processing from the repository 
reading for move. If two arguments are passed - "move X Y", you need to read the repo to 
tell whether Y is an already-existing directory or not. If it is, the operation is to move 
X to Y/X ; if it's not, the operation is to move X to Y. I think this logic got lost in 
the refactoring.
msg15189 (view) Author: noreply Date: 2012-02-28.12:23:25
The following patch sent by Florent Becker <florent.becker@ens-lyon.org> updated issue issue2139 with
status=has-patch

* resolve issue2139: detect what to do when using darcs mv into/onto a directory 
Ignore-this: d37322c51d6ae3bd4f5ce22848f15c99
msg15563 (view) Author: noreply Date: 2012-04-13.20:30:42
The following patch sent by Florent Becker <florent.becker@ens-lyon.org> updated issue issue2139 with
status=resolved;resolvedin=2.10.0 HEAD

* resolve issue2139: detect what to do when using darcs mv into/onto a directory 
Ignore-this: d37322c51d6ae3bd4f5ce22848f15c99
History
Date User Action Args
2012-02-06 11:52:06koweycreate
2012-02-06 11:52:40koweysetmessages: + msg15094
2012-02-09 23:00:06mulandersetmessages: + msg15100
2012-02-09 23:00:39ganeshsetmessages: + msg15101
2012-02-09 23:10:02ganeshsetmessages: + msg15102
2012-02-10 00:11:50mulandersetmessages: + msg15104
2012-02-11 14:26:51koweysetfiles: + failing-issue2139-mv-to-dir.sh
nosy: + galbolle
messages: + msg15109
2012-02-18 21:14:44koweysetassignedto: ganesh
messages: + msg15116
nosy: + ganesh
2012-02-18 22:34:56ganeshsetstatus: needs-reproduction -> needs-implementation
messages: + msg15124
2012-02-24 20:38:06mndrixsetnosy: + mndrix
2012-02-28 12:23:25noreplysetstatus: needs-implementation -> has-patch
messages: + msg15189
2012-04-13 20:30:43noreplysetstatus: has-patch -> resolved
messages: + msg15563
resolvedin: 2.10.0 HEAD