darcs

Patch 2062 add tests for URL/path classification functions

Title add tests for URL/path classification functions
Superseder Nosy List bf
Related Issues
Status accepted Assigned To
Milestone

Created on 2020-07-20.10:16:57 by bf, last changed 2020-07-22.14:39:22 by bf.

Files
File name Status Uploaded Type Edit Remove
add-tests-for-url_path-classification-functions.dpatch dead bf, 2020-07-20.10:16:57 application/x-darcs-patch
add-tests-for-url_path-classification-functions.dpatch bf, 2020-07-21.21:46:36 application/x-darcs-patch
patch-preview.txt bf, 2020-07-20.10:16:57 text/x-darcs-patch
patch-preview.txt bf, 2020-07-21.21:46:36 text/x-darcs-patch
patch2062-output.txt ganesh, 2020-07-20.21:32:08 text/plain
unnamed bf, 2020-07-20.10:16:57 text/plain
unnamed bf, 2020-07-21.21:46:36 text/plain
See mailing list archives for discussion on individual patches.
Messages
msg22215 (view) Author: bf Date: 2020-07-20.10:16:57
Following up on review of patch2037

Will screen if/when you confirm that the test succeeds on Windows, too.

1 patch for repository http://darcs.net/screened:

patch b483885aadc9d37df9cd0839aa06b3cd3eacf010
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jul 20 12:16:33 CEST 2020
  * add tests for URL/path classification functions
Attachments
msg22221 (view) Author: ganesh Date: 2020-07-20.21:32:08
Afraid they don't pass - see attached.
Attachments
msg22225 (view) Author: bf Date: 2020-07-21.09:33:23
> Afraid they don't pass - see attached.

Hm. Indeed, the docs of System.FilePath.Windows.isValid claim that
colons in file paths are forbidden, except for specifying the "drive"
(volume).

Can you please check that this is correct i.e. Windows does not accept
paths with colons in them like those that fail the test?

If this is true, then the fix for issue2645 cannot work on Windows. This
calls the whole idea into question: if such a path is invalid on
Windows, then allowing them on a Posix system creates yet another
incompatibility when exchanging patches between repos residing on
different systems.

If this is not true, then we should report this as a bug against the
filepath library and create a work-around for it in darcs.
msg22233 (view) Author: bf Date: 2020-07-21.21:18:24
I did a bit of research and it appears that ':' is indeed not a valid
character in filenames on Windows.

> [...] then the fix for issue2645 cannot work on Windows. This
> calls the whole idea into question: if such a path is invalid on
> Windows, then allowing them on a Posix system creates yet another
> incompatibility when exchanging patches between repos residing on
> different systems.

Let me take this back, at least with regard the conclusion. This whole
thing is not about paths /inside/ a repo, but rather about paths /to/ a
repo. Besides, there are quite a lot more filename restrictions on
Windows, compared to Linux. There are even filenames consisting only of
alphabetic ASCII characters that are forbidden because they are reserved
for special things like IO ports.

My conclusion is that the text cases need to be fixed, while the patch
is fine.
msg22235 (view) Author: bf Date: 2020-07-21.21:46:36
Amended patch. The test should now succeed on Windows.

1 patch for repository http://darcs.net/screened:

patch a320b5d301ea39bb5b1deef816e904c1de7f3252
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jul 20 12:16:33 CEST 2020
  * add tests for URL/path classification functions
Attachments
msg22239 (view) Author: ganesh Date: 2020-07-21.22:00:16
Yep, all good now. I am most impressed with your remote Windows coding
skills :-)
msg22245 (view) Author: bf Date: 2020-07-22.14:39:21
self-accept
History
Date User Action Args
2020-07-20 10:16:57bfcreate
2020-07-20 21:32:08ganeshsetfiles: + patch2062-output.txt
messages: + msg22221
2020-07-21 09:33:26bfsetmessages: + msg22225
2020-07-21 21:18:25bfsetmessages: + msg22233
2020-07-21 21:46:37bfsetfiles: + patch-preview.txt, add-tests-for-url_path-classification-functions.dpatch, unnamed
messages: + msg22235
2020-07-21 22:00:16ganeshsetmessages: + msg22239
2020-07-22 14:32:31bfsetstatus: needs-screening -> needs-review
2020-07-22 14:39:22bfsetstatus: needs-review -> accepted
messages: + msg22245