darcs

Patch 203 Implemented all 13 possible symlink handling scenarios...

Title Implemented all 13 possible symlink handling scenarios...
Superseder Nosy List darcs-users, dastapov, kowey, mornfall, twb
Related Issues Darcs >2.3 follows directory symlinks.
View: 1645
Status accepted Assigned To twb
Milestone

Created on 2010-04-01.17:39:06 by dastapov, last changed 2011-05-10.20:36:18 by darcswatch. Tracked on DarcsWatch.

Files
File name Status Uploaded Type Edit Remove
failing-issue1645-ignore-symlinks.sh.patch dastapov, 2010-04-04.19:54:38 text/x-patch
implemented-all-13-possible-symlink-handling-scenarios-that-i-could-think-of-for-issue-1645.dpatch dastapov, 2010-04-01.17:39:06 text/x-darcs-patch
implemented-all-test-cases-from-the-discussion-of-bug-1645.dpatch dastapov, 2010-04-06.05:55:07 text/x-darcs-patch
unnamed dastapov, 2010-04-01.17:39:06 text/plain
unnamed dastapov, 2010-04-06.05:55:07 text/plain
See mailing list archives for discussion on individual patches.
Messages
msg10611 (view) Author: dastapov Date: 2010-04-01.17:39:06
1 patch for repository http://darcs.net/repos/unstable:

Thu Apr  1 20:37:47 EEST 2010  Dmitry Astapov <dastapov@gmail.com>
  * Implemented all 13 possible symlink handling scenarios that I could think of for issue 1645
Attachments
msg10614 (view) Author: kowey Date: 2010-04-01.21:17:24
Hi Trent, any chance I could ask you to look over this one, as you're
also particularly affected by issue1645 and you know how to think
clearly about these issues?
msg10615 (view) Author: kowey Date: 2010-04-01.21:44:38
Also, I'll note that issue1645 is marked as a potential blocker for
Darcs 2.4.1 (or maybe Darcs 2.4.2), so I'd like to vote this up to the
top of your Darcs pile if possible :-)
msg10617 (view) Author: twb Date: 2010-04-02.00:08:23
Thanks for extending these tests, Dmitry.

Dmitry Astapov wrote:
> -darcs init      --repo R S      # Create our test repos.
> +mkdir -p R S                    # Create our test repos.
> +(cd R; darcs init)
> +(cd S; darcs init)

Mea culpa.  That should be

    darcs init --repo R
    darcs init --repo S

> +darcs --version

Superfluous.

> +# Rationale: Since darcs does not version-control symlinks, it should do a
> +# Sensible Thing handling them.
> +# All these tests are passed with darcs-2.2

This belongs in the introductory comment at the top of the file.

> +touch log
> +add_to_boring '^log$'

It's a reasonable idea to do this up-front to avoid log causing false
positives/negatives.  A better idea might be to simply use ../log
(i.e. store the buffer outside the repo).

> -echo 'Example content.' > f
> -darcs rec -lamf                 # business as usual...
> -ln -s f l                       # darcs should just ignore l
> -not darcs add -qr .             # add -r never had -l's problem.
> -darcs w -l >log                 # w -l shouldn't "see" the link.
> -grep 'No changes!' log
> -darcs rec -laml >log            # rec -l shouldn't record it.
> -grep 'No changes!' log
> -cd ..

Above, I made sure to check add, w -l *and* rec -l, since currently
their behaviour seems inconsistent.  The new version only tests w -l;
I think that is insufficient.

> +[ -z "$(grep -vE "(^ *$|a ./non-recorded-dir)" log)" ]

grep's exit status is meaningful; you should trust it instead of
wrapping in a [ -z "$()" ].

> hunk ./tests/failing-issue1645-ignore-symlinks.sh 129
> +# Case 12: link to device file
> +ln -s /dev/zero l

Should also test what happens when the device file occurs within the
repo (cp -a it in, I guess).

                                * * *

Suggest ref. symlink(7) and path_resolution(7) in introduction.
Suggest testing

 - dangling symlinks.
 - absolute (cf. relative) links.
 - historical drive letter idiom "/../C/vms".
 - links that cross filesystems.
 - symlinks to hard links.
 - case-folding links, e.g. ln -s Makefile makefile.
 - links to char vs. block devices.
 - links to Solaris "door" inodes?
 - links to sockets.

Suggest referring to "regular file" to disambiguate, since technically
directories and such are also files.
msg10670 (view) Author: dastapov Date: 2010-04-04.19:54:38
Trent W. Buck wrote:
> Trent W. Buck <trentbuck@gmail.com> added the comment:
>
> Thanks for extending these tests, Dmitry.
>
>   
>> +darcs --version
>>     
>
> Superfluous.
>   
I put this in to make sure I am testing the version I think I am 
testing. It slipped into final commit by error.

>> +# Rationale: Since darcs does not version-control symlinks, it should do a
>> +# Sensible Thing handling them.
>> +# All these tests are passed with darcs-2.2
>>     
>
> This belongs in the introductory comment at the top of the file.
>   
Sure. Done.

>> +touch log
>> +add_to_boring '^log$'
>>     
>
> It's a reasonable idea to do this up-front to avoid log causing false
> positives/negatives.  A better idea might be to simply use ../log
> (i.e. store the buffer outside the repo).
>   
I decided that I'd better keep all the relevant info inside ./R.
> Above, I made sure to check add, w -l *and* rec -l, since currently
> their behaviour seems inconsistent.  The new version only tests w -l;
> I think that is insufficient.
>   
I did not know that "rec" could behave differently. I'm extending the 
tests to cover it as well.
>   
>> +[ -z "$(grep -vE "(^ *$|a ./non-recorded-dir)" log)" ]
>>     
>
> grep's exit status is meaningful; you should trust it instead of
> wrapping in a [ -z "$()" ].
>   
Right. Totally forgot about that.
>   
>> hunk ./tests/failing-issue1645-ignore-symlinks.sh 129
>> +# Case 12: link to device file
>> +ln -s /dev/zero l
>>     
>
> Should also test what happens when the device file occurs within the
> repo (cp -a it in, I guess).
>   
You mean, test the handling of the device file itself, not symlink to it?
I believe this would be better placed into a separate test.

>                                 * * *
>
> Suggest ref. symlink(7) and path_resolution(7) in introduction.
> Suggest testing
>
>  - dangling symlinks.
>   
Already covered - "Case 10"
>  - absolute (cf. relative) links.
>   
Right. Adding ...
>  - historical drive letter idiom "/../C/vms".
>  - links that cross filesystems.
>   
What could possibly (additionaly) go wrong in those two cases?
>  - symlinks to hard links.
>   
Could you please elaborate on this one?
>  - case-folding links, e.g. ln -s Makefile makefile.
>   
Adding this as well.
>  - links to char vs. block devices.
>  - links to Solaris "door" inodes?
>  - links to sockets.
>   
Don't you think that those are kinda superfluous?

What is the best way to sumbit amended patch right now? Should I "darcs 
amend" what I already have and re-sent it, or record additional patch 
and send two of them once again, or ...?

For now, I would attach amended patch to this letter.
Attachments
msg10680 (view) Author: kowey Date: 2010-04-05.20:03:51
Hi Dmitry,

The best way to get an amended patch to us is with (i) darcs
amend-record and (ii) darcs send --subject='[patch203]'.  

Note that in between your patch and Trent's review, I pushed a minor
fixup patch for the issue1645 test that I had mentioned.  Sorry if that
creates problems. I think if you just pull my patch and amend-record
yours, the duplicate patch should go away.
msg10681 (view) Author: dastapov Date: 2010-04-06.05:55:07
Tue Apr  6 08:53:46 EEST 2010  Dmitry Astapov <dastapov@gmail.com>
  * Implemented all test cases from the discussion of bug 1645
Attachments
msg10684 (view) Author: mornfall Date: 2010-04-06.09:02:56
Dmitry Astapov <bugs@darcs.net> writes:

>> Should also test what happens when the device file occurs within the
>> repo (cp -a it in, I guess).
>>   
> You mean, test the handling of the device file itself, not symlink to it?
> I believe this would be better placed into a separate test.

You can't create device nodes as non-root...
msg10697 (view) Author: kowey Date: 2010-04-08.11:06:18
On Tue, Apr 06, 2010 at 05:55:08 +0000, Dmitry Astapov wrote:
> Tue Apr  6 08:53:46 EEST 2010  Dmitry Astapov <dastapov@gmail.com>
>   * Implemented all test cases from the discussion of bug 1645

I've had a quick glance at this and applied this.
Thanks!

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
msg10699 (view) Author: darcswatch Date: 2010-04-08.11:25:55
This patch bundle (with 1 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-f5cf7becfa339d1c63fc214129aaba025d441f20
msg14285 (view) Author: darcswatch Date: 2011-05-10.20:36:18
This patch bundle (with 1 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-f5cf7becfa339d1c63fc214129aaba025d441f20
History
Date User Action Args
2010-04-01 17:39:06dastapovcreate
2010-04-01 17:40:29darcswatchsetdarcswatchurl: http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-cb07d806c1fd184896303fb29ca9a6cb3b522afb
2010-04-01 21:17:24koweysetnosy: + kowey, twb
messages: + msg10614
issues: + Darcs >2.3 follows directory symlinks.
assignedto: twb
2010-04-01 21:44:39koweysetmessages: + msg10615
2010-04-02 00:08:23twbsetmessages: + msg10617
2010-04-04 19:54:39dastapovsetfiles: + failing-issue1645-ignore-symlinks.sh.patch
messages: + msg10670
2010-04-05 20:03:51koweysetmessages: + msg10680
2010-04-06 05:55:08dastapovsetfiles: + implemented-all-test-cases-from-the-discussion-of-bug-1645.dpatch, unnamed
messages: + msg10681
2010-04-06 05:56:17darcswatchsetdarcswatchurl: http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-cb07d806c1fd184896303fb29ca9a6cb3b522afb -> http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-f5cf7becfa339d1c63fc214129aaba025d441f20
2010-04-06 09:02:56mornfallsetnosy: + mornfall
messages: + msg10684
2010-04-08 11:06:19koweysetmessages: + msg10697
2010-04-08 11:25:55darcswatchsetstatus: needs-review -> accepted
messages: + msg10699
2011-05-10 18:35:56darcswatchsetdarcswatchurl: http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-f5cf7becfa339d1c63fc214129aaba025d441f20 -> http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-cb07d806c1fd184896303fb29ca9a6cb3b522afb
2011-05-10 20:36:18darcswatchsetmessages: + msg14285