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

• implemented-all-13-possible-symlink-handling-scenarios-that-i-could-think-of-for-issue-1645.dpatch
• unnamed

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?

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 :-)

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.

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

• failing-issue1645-ignore-symlinks.sh.patch

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.

Tue Apr  6 08:53:46 EEST 2010  Dmitry Astapov <dastapov@gmail.com>
  * Implemented all test cases from the discussion of bug 1645

Attachments

• implemented-all-test-cases-from-the-discussion-of-bug-1645.dpatch
• unnamed

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...

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

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

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