darcs

Issue 1932 isFile skips real files with colons, breaks "add" in darcs 2.4.98

Title isFile skips real files with colons, breaks "add" in darcs 2.4.98
Priority bug Status resolved
Milestone Resolved in 2.8.0
Superseder Nosy List dastapov, dmitry.kurochkin
Assigned To
Topics FilePath, Regression

Created on 2010-08-24.10:22:39 by dastapov, last changed 2010-11-17.15:36:48 by kowey.

Messages
msg12282 (view) Author: dastapov Date: 2010-08-24.10:22:38
I am using darcs with etckeeper to manage my /etc. Recently I updated to darcs 2.4.4 
(release) and suddenly "darcs add -qr ." began failing with "fromJust: Nothing" error 
message.

I've taken bleeding-edge hashed-storage and darcs and built myself a version "2.4.98.3 
(+ 126 patches)". I then grepped for all files that have "fromJust" and dont have 
"inclide impossible.h", added "include impossible.h" to src/Darcs/Commands/Add.hs and 
voila:

  darcs: bug at src/Darcs/Commands/Add.lhs:264 compiled Aug 24 2010 13:01:16
  fromJust error at src/Darcs/Commands/Add.lhs:264 compiled Aug 24 2010 13:01:16
  See http://wiki.darcs.net/BugTracker/Reporting for help on bug reporting.

In a short while I managed to track the root cause to the "isFile" in src/Darcs/URL.hs.

My /etc/ contains files like "usb_modeswitch.d/19d2:0053" which were added to the 
repository when I was using darcs 2.2.0. With darcs 2.4.4 (and beyond) they are not 
considered to be a file, hence "isRelative" returns False for them, hence 
"simpleSubPath" returns Nothing for them, and after that "fromJust" fails in Add.lhs.

There is a quick workaround: replace "map (fromJust . simpleSubPath)" with "catMaybes . 
map simpleSubPath" - offending files would be skipped and darcs would not crash. 
However, it seems to me that proper course of action would be to handle this files.

I will attach a patch with failing test case shortly,
msg12285 (view) Author: dastapov Date: 2010-08-24.10:34:08
I've attached patches with both test case and quick workaround
msg12294 (view) Author: kowey Date: 2010-08-25.15:00:39
Thanks for the bug and the patches Dmitry! Can we confirm to what extent 
this issue is a regression?  What was the last working version for you?

As you've most kindly submitted a test, verifying this should actually 
be not so painful (at least on our part if you don't have the time).

BTS training: (talking to any future Darcs Issue Managers).  Basically 
there are 3 actions you need to worry about before you can set a ticket 
to need-implementation: (1) create a minimal test case (2) find out if 
it's a regression and from what, (3) diagnose the problem.  Dmitry has 
helped us with #1 by submitting patch361 and a little bit with #3 by 
submitting patch362.  Now we're just waiting on a bit of tuning for #1 
and hopefully some more clues for #2. If we don't get them, that's fine 
because we can just reuse the test case he sent us.
msg12304 (view) Author: dastapov Date: 2010-08-25.17:34:26
The real issue at hand is with handling
"tricky" characters in file names. On Unix, I can have ':' and '?' or '<' in
my filename, while on Windows they are forbidden. This is supposed to be handled by "--reserved-ok" 
option, so I guess that the official position is that users are allowed
to shoot themselves in the foot however they like.

I am fine with that and will assume this much for the rest of the text. So,
the correct behavior for darcs would be to add files like "aaa:bbb" and even
"c:\src" to the repository, as long as those files really exist and are
really files, and "--reserved-ok" was supplied.

Now, colon has a special role in Darcs, since it is used in names of remote
scp-accessible repositories. It seems that a colon in repository name should
automatically mean that it is either a HTTP repo or an scp repo. Unless we
make a drastical change and start requiring "scp://" in front of the scp
repo names, all colons in repo dir names should be disallowed.

Then, functions in Darcs.URL clearly serve to distinguish between possible
forms of repo names.
* isUrl should be true for names like http://....
* isSsh should be true for all names that are not URLs and containin ':'
* isFile should be true whenever both isSSH and isURL are false.

I browsed through the source and isFile, isSsh, and isUrl are indeed used to
differentiate between possible forms of repository names everywhere, except
for the "isRelative" in URL.hs

My proposal is:
1)Replace isRelative from URL.hs by isRelative from System.FilePath, which
should take into account differences between platforms and handle colon
under unix in sensible way.

2)Submit the proposal to mark ssh-accesible repositories by "ssh://" (see old 
comments at the end of the Darcs.URL description!) and
replace a horrible ad-hoc implementations of isSsh and isFile with much
better ad-hoc implementation:

isSsh f = "ssh://" `isPrefixOf` f
isUrl f = not (isSsh f) && "://" `isInfixOf` f
isFile f = not (isSsh f || isUrl f)

Do you agree?

I'll be submitting the patch for (1) shortly (along with proper test-case).
msg12305 (view) Author: dastapov Date: 2010-08-25.17:39:51
I've submitted patches for proper testcase and fix via "darcs send" and 
obsoleted old patches. Hope this is ok.
msg12324 (view) Author: dastapov Date: 2010-08-26.19:32:51
For all concerned, relevant discussion seems to be moving to patch368
msg13087 (view) Author: kowey Date: 2010-11-17.15:36:47
Looks like this was fixed by

Wed Aug 25 18:30:45 BST 2010  Dmitry Astapov <dastapov@gmail.com>
  * Fix for issue1932
  Do _not_ check for colons in "isRelative" - everything is covered
  inside Add.lhs and governed by use of "--reserved-ok" option.
  
  Replace isRelative and isAbsolute with functions from System.FilePath
  for correct handling of platform specifics.
History
Date User Action Args
2010-08-24 10:22:39dastapovcreate
2010-08-24 10:32:36dastapovlinkpatch362 issues
2010-08-24 10:33:26dastapovlinkpatch363 issues
2010-08-24 10:34:09dastapovsetmessages: + msg12285
2010-08-25 15:00:42koweysetpriority: bug
status: unknown -> needs-reproduction
topic: + Regression, FilePath
messages: + msg12294
2010-08-25 17:34:26dastapovsetmessages: + msg12304
2010-08-25 17:36:24dastapovlinkpatch367 issues
2010-08-25 17:36:40dastapovlinkpatch368 issues
2010-08-25 17:39:52dastapovsetmessages: + msg12305
2010-08-26 18:51:39dastapovlinkissue1881 superseder
2010-08-26 19:32:52dastapovsetmessages: + msg12324
2010-11-17 15:36:48koweysetstatus: needs-reproduction -> resolved
messages: + msg13087
resolvedin: 2.8.0
2010-11-17 15:38:38koweylinkissue1991 superseder