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

Sun Aug 22 20:38:41 COT 2010  builes.adolfo@googlemail.com
  * Solve issue1923: bad source warning mechanism warns about sources outside your control

Attachments

• solve-issue1923_-bad-source-warning-mechanism-warns-about-sources-outside-your-control.dpatch
• unnamed

For all I can tell, the patch breaks this scenario (I haven't tried, but 
it looks like it would):

mkdir x
darcs init --repo x
echo x > x/x
darcs rec -lam x --repo x
darcs get x y --lazy
darcs get y z --lazy
rm -rf y
darcs chan -v --repo z

It may even break local lazy repos altogether, though. Please check the 
above and also add tests for this behaviour (or point out that these 
things are already covered by the testsuite and I am worrying 
needlessly).

Yours,
   Petr.

On Tue, Aug 24, 2010 at 08:49:26 +0000, Petr Ročkai wrote:
> It may even break local lazy repos altogether, though. Please check the 
> above and also add tests for this behaviour (or point out that these 
> things are already covered by the testsuite and I am worrying 
> needlessly).

One more thing! (silly thing) Could you tweak your patch name so that
it starts with 'Resolve issue1923:'?

I'm always embarrassed to harp on this sort of thing, but last time I
spotted an extraneous space ("issue 1599"), bit my tongue and then later
regretted it when the dumb darcs-BTS integration failed to notice the
patch was applied.  Having the automated BTS-darcs integration work
saves a lot of effort all around (*), particularly now that we're using
it for release management.  The predictability also makes it easier for
humans to search for patches in the future.

One potentially useful hint: when I'm not sure about the exact syntax
for this sort of thing, I do an approximate search, eg.

  darcs changes -i -p 'issue'

Which gives me examples to copy

Thanks!

Eric

(*) At least I hope so!  But I think as a general rule, robots should
    do as much of the dumb stuff as possible so that humans can spend
    hacking Darcs instead of tweaking form entries.
    
    Unfortunately, making robots smart enough to recognise, say the
    difference between 'Resolve issue 1599' and 'Resolve issue1599'
    (which is what bit me last time) takes some effort.

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, try +44 (0)1273 64 2905 or
xmpp:kowey@jabber.fr (Jabber or Google Talk only)

On Tue, Aug 24, 2010 at 3:49 AM, Petr Ročkai <bugs@darcs.net> wrote:
>
> Petr Ročkai <me@mornfall.net> added the comment:
>
> For all I can tell, the patch breaks this scenario (I haven't tried, but
> it looks like it would):
>
> mkdir x
> darcs init --repo x
> echo x > x/x
> darcs rec -lam x --repo x
> darcs get x y --lazy
> darcs get y z --lazy
> rm -rf y
> darcs chan -v --repo z
>
You are right, it does break that scenario.

What we shouldn't copy are the local entries in source from the repo
we are puling from, if it is remote.

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

Sun Sep  5 21:06:27 COT 2010  'Adolfo Builes <builes.adolfo@googlemail.com>'
  * Resolve issue1923: bad source warning mechanism warns about sources outside your control

Attachments

• resolve-issue1923_-bad-source-warning-mechanism-warns-about-sources-outside-your-control.dpatch
• unnamed

Thanks!

On Mon, Sep 06, 2010 at 02:32:24 +0000, Adolfo Builes wrote:
> Sun Sep  5 21:06:27 COT 2010  'Adolfo Builes <builes.adolfo@googlemail.com>'
>   * Resolve issue1923: bad source warning mechanism warns about sources outside your control

1. You appear to have single quotes in your authors file 'Adolfo Builes'
   which is likely not intentional.  I'd suggest amending that with -A
   to fix the author (confusingly, in the amend, you have to use single
   quotes for the shell, but Darcs itself does not use them)

2. Overly long patch title, which could be nice to fix while you're at
   it: http://wiki.darcs.net/Development/GettingStarted

3. Can you include a regression shell test, as you've done for your
   other work?

I'm CC'ing Zooko in case he's interested in having me submit various
case studies of things Darcs might test for.  This is a sort of
straightforward version where we already have a sort of testing
culture (albeit not a very rigorous one): issues we have already
identified through dogfooding and which we know how to reproduce.
I think our reflex here is just to go for a functional test.

Resolve issue1923: bad source warning mechanism warns about sources outside your control
----------------------------------------------------------------------------------------
> -                  here ++ [Cache Repo NotWritable repodir] ++ there
> +                  here ++ [Cache Repo NotWritable repodir] ++ filterExternalSources there

> -
> +            filterExternalSources there = if isFile repodir
> +                                          then
> +                                            there
> +                                          else
> +                                            filter (not . isFile . cacheSource) there

The patch itself seems fine to me, at least at first glance.

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, try +44 (0)1273 64 2905 or
xmpp:kowey@jabber.fr (Jabber or Google Talk only)

Hi Eric,

I have been trying to found a test case for such behavior but I
haven't been able to do it.
I know it happens when it propagates the remote local sources and
tries to use it, but  somehow it doesn't happen always.

I tried first using Petr solution with lighthttpd but someone the test
will pass when it should fail, then I setup an account in my server,
and did the same to simulate the situation you describe first, and
then when pulling it wouldn't complain.

Maybe could you give me a hand with that test case, or ping me on irc,
and we can talk more about it.

--
Adolfo

On Mon, Sep 6, 2010 at 3:51 AM, Eric Kow <kowey@darcs.net> wrote:
> Thanks!
>
> On Mon, Sep 06, 2010 at 02:32:24 +0000, Adolfo Builes wrote:
>> Sun Sep  5 21:06:27 COT 2010  'Adolfo Builes <builes.adolfo@googlemail.com>'
>>   * Resolve issue1923: bad source warning mechanism warns about sources outside your control
>
> 1. You appear to have single quotes in your authors file 'Adolfo Builes'
>   which is likely not intentional.  I'd suggest amending that with -A
>   to fix the author (confusingly, in the amend, you have to use single
>   quotes for the shell, but Darcs itself does not use them)
>
> 2. Overly long patch title, which could be nice to fix while you're at
>   it: http://wiki.darcs.net/Development/GettingStarted
>
> 3. Can you include a regression shell test, as you've done for your
>   other work?
>
> I'm CC'ing Zooko in case he's interested in having me submit various
> case studies of things Darcs might test for.  This is a sort of
> straightforward version where we already have a sort of testing
> culture (albeit not a very rigorous one): issues we have already
> identified through dogfooding and which we know how to reproduce.
> I think our reflex here is just to go for a functional test.
>
> Resolve issue1923: bad source warning mechanism warns about sources outside your control
> ----------------------------------------------------------------------------------------
>> -                  here ++ [Cache Repo NotWritable repodir] ++ there
>> +                  here ++ [Cache Repo NotWritable repodir] ++ filterExternalSources there
>
>> -
>> +            filterExternalSources there = if isFile repodir
>> +                                          then
>> +                                            there
>> +                                          else
>> +                                            filter (not . isFile . cacheSource) there
>
> The patch itself seems fine to me, at least at first glance.
>
> --
> Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
> For a faster response, try +44 (0)1273 64 2905 or
> xmpp:kowey@jabber.fr (Jabber or Google Talk only)
>

On Tue, Sep 07, 2010 at 21:30:29 -0500, Adolfo Builes wrote:
> I have been trying to found a test case for such behavior but I
> haven't been able to do it.
> I know it happens when it propagates the remote local sources and
> tries to use it, but  somehow it doesn't happen always.

Actually! I just realised that your patch may have another problem,
so perhaps we should discuss that.

I'll leave the question of testing to you and Petr (if he doesn't mind)
because I haven't fully wrapped my head around the lighthttpd stuff yet,
sorry!

> I tried first using Petr solution with lighthttpd but someone the test
> will pass when it should fail, then I setup an account in my server,
> and did the same to simulate the situation you describe first, and
> then when pulling it wouldn't complain.

For what it's worth, I've got failing tests on the lightttpd stuff with
the new infrastructure, but I haven't gotten around to figuring out why
yet.

Output below (ideas, Petr?)

> >> +            filterExternalSources there = if isFile repodir
> >> +                                          then
> >> +                                            there
> >> +                                          else
> >> +                                            filter (not . isFile . cacheSource) there

One issue here is that if the remote repository is non-local, then we don't do
*any* of the issue1599 checking, not even for paths which are within our
control.

So we have

  _darcs/sources:
    1. /some/expired/local/path
    2. cant@reach-this-either.cz

  foo@example.com:blah/_darcs/sources
    3. /example/local/path/we/dont/control
    4. http://other-bad-stuff

What we want to catch are 1, 2 and 4.  Your issue1599 work catches 1,2,3,4
with 3 being the case we had not considered.  The current version of the
issue1923 patch eliminates 1 and 3, where actually what we really want to
eliminate is just 3.

See what I mean?  How can we address something like this?  Ideally, we'd
know about where the source entries come from.  But maybe eliminating 1
is just unfortunate, but acceptable collateral damage?

issue1599 failing output
------------------------
issue1599-automatically-expire-unused-caches.sh (Darcs2): [Failed]
| 
| skip-formats old-fashioned
| + skip-formats old-fashioned
| + for f in '"$@"'
| + grep old-fashioned /tmp/tmp9289/.darcs/defaults
| + true
| 
| rm -rf R S log && mkdir R
| + rm -rf R S log
| + mkdir R
| cd R
| + cd R
| darcs init
| + darcs init
| echo a > a
| + echo a
| darcs rec -lam a
| + darcs rec -lam a
| Finished recording patch 'a'
| echo b > b
| + echo b
| darcs rec -lam b
| + darcs rec -lam b
| Finished recording patch 'b'
| echo c > c
| + echo c
| darcs rec -lam c
| + darcs rec -lam c
| Finished recording patch 'c'
| cd ..
| + cd ..
| 
| serve_http # sets baseurl
| + serve_http
| + cat
| + trap 'finish_http "/tmp/tmp9289"' EXIT
| + lighttpd -f light.conf
| cat light.pid
| ++ cat light.pid
| + ps
| + baseurl=http://localhost:23032
| darcs get --lazy $baseurl/R S
| + darcs get --lazy http://localhost:23032/R S
| 
| darcs failed:  Not a repository: http://localhost:23032/R (HTTP response code said error)
| finish_http "/tmp/tmp9289"
| + finish_http /tmp/tmp9289
| + test -e /tmp/tmp9289/light.pid
| cat "$1/light.pid"
| ++ cat /tmp/tmp9289/light.pid
| + kill 13672

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, try +44 (0)1273 64 2905 or
xmpp:kowey@jabber.fr (Jabber or Google Talk only)

Hi Eric,

>
>  _darcs/sources:
>    1. /some/expired/local/path
>    2. cant@reach-this-either.cz
>
>  foo@example.com:blah/_darcs/sources
>    3. /example/local/path/we/dont/control
>    4. http://other-bad-stuff
>
> What we want to catch are 1, 2 and 4.  Your issue1599 work catches 1,2,3,4
> with 3 being the case we had not considered.  The current version of the
> issue1923 patch eliminates 1 and 3, where actually what we really want to
> eliminate is just 3.
>
> See what I mean?  How can we address something like this?  Ideally, we'd
> know about where the source entries come from.  But maybe eliminating 1
> is just unfortunate, but acceptable collateral damage?
>

When we are loading the sources we know where they come from, in prefs
we have the following:

here <- parsehs `fmap` getPreffile (darcsdir ++ "/prefs/sources")
there <- (parsehs . lines . BC.unpack) `fmap`
                (gzFetchFilePS (repodir </> darcsdir </>
"prefs/sources") Cachable
                 `catchall` return B.empty)

Where "here" are the local sources and "there"  the remotes.

That's why I just apply the filter to "there".

btw, Are you able to reproduce the issue with Darcs repository ?

--
Adolfo

On Thu, Sep 09, 2010 at 08:55:37 -0500, Adolfo Builes wrote:
> When we are loading the sources we know where they come from, in prefs
> we have the following:

Sounds good!

> btw, Are you able to reproduce the issue with Darcs repository ?

No, not at the moment, but I did recently get it pulling from
branch-2.5:

  Finished pulling and applying.

  I could not reach the following repositories:
  /home/darcs-unstable/.darcs/cache
  /home/darcs-unstable/releases/branch-2.5
  /home/darcs-unstable/darcs
  If you're not using them, you should probably delete
  the corresponding entries from _darcs/prefs/sources.

I think that the way to trigger the problem is to set up some kind of situation
where Darcs might be try to access the cache entry, for example by creating
some whole new patch after you've done a get and then trying to pull it.

Perhaps if you can't automate it just yet, it could be good to try an example
by hand first and see if you can reliably reproduce it by hand first.

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, try +44 (0)1273 64 2905 or
xmpp:kowey@jabber.fr (Jabber or Google Talk only)

Hi Adolfo,

Just a follow-up on this patch.  It turns out that my issue1599 test 
failures were due to stupid configuration on my part.

I'm behind a proxy server.  One day, I stupidly emptied out my 
"no_proxy" environment variable to work around this gnome-terminal-
related bug: https://bugs.launchpad.net/ubuntu/lucid/+source/gnome-
terminal/+bug/534225

So the failure here was caused by the request to localhost going through 
my University's proxy server.  Setting no_proxy to include localhost 
fixed this.  There was also a long timeout on the 10.1.2.3 test.  Since 
it seemed a bit weird to ask people to arbitrarily put 10.1.2.3 in their 
no_proxy settings, I modified the issue1599 test to skip 10.1.2.3 if 
http_proxy was set.

OK!  Are you still working on testing issue1923, or would you like us to 
take over?  If you could amend your patch as requested in msg12469, that 
would be helpful; but if you don't manage to do so soon, I'll just do it 
myself :-)

Hi Eric,

I haven't add time to think about the regression test, but I will
allocate a couple of hours during the weekend for that.

--
Adolfo
On Fri, Sep 17, 2010 at 8:12 AM, Eric Kow <bugs@darcs.net> wrote:
>
> Eric Kow <kowey@darcs.net> added the comment:
>
> Hi Adolfo,
>
> Just a follow-up on this patch.  It turns out that my issue1599 test
> failures were due to stupid configuration on my part.
>
> I'm behind a proxy server.  One day, I stupidly emptied out my
> "no_proxy" environment variable to work around this gnome-terminal-
> related bug: https://bugs.launchpad.net/ubuntu/lucid/+source/gnome-
> terminal/+bug/534225
>
> So the failure here was caused by the request to localhost going through
> my University's proxy server.  Setting no_proxy to include localhost
> fixed this.  There was also a long timeout on the 10.1.2.3 test.  Since
> it seemed a bit weird to ask people to arbitrarily put 10.1.2.3 in their
> no_proxy settings, I modified the issue1599 test to skip 10.1.2.3 if
> http_proxy was set.
>
> OK!  Are you still working on testing issue1923, or would you like us to
> take over?  If you could amend your patch as requested in msg12469, that
> would be helpful; but if you don't manage to do so soon, I'll just do it
> myself :-)
>
> ----------
> assignedto:  -> abuiles
>
> __________________________________
> Darcs bug tracker <bugs@darcs.net>
> <http://bugs.darcs.net/patch360>
> __________________________________
>

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

Fri Sep 17 16:06:53 COT 2010  Adolfo Builes <builes.adolfo@googlemail.com>
  * Solve issue1923: cache mechanism warns about sources outside your control

Attachments

• solve-issue1923_-cache-mechanism-warns-about-sources-outside-your-control.dpatch
• unnamed

Sorry, I should have chased you, Adolfo the Monday the weekend you said
you'd look at the patch :-) (the rule is "don't forget to follow up")

Anyway, I'll take over trying to test this (last day of Darcs Hacking
Sprint 5) unless you happen to be free today (Sunday 17 Oct).

Hi Eric,

Sorry I have been so absent lately but I have been through lots of stuff.

I just saw this message today, did you have any success with the test ? I
will have more free time to allocate again to Darcs in a couple of weeks,
since we are in the last weeks of Uni, which is consuming all my time plus
I'm working part-time.

--
Adolfo


On Sun, Oct 17, 2010 at 4:16 AM, Eric Kow <bugs@darcs.net> wrote:

>
> Eric Kow <kowey@darcs.net> added the comment:
>
> Sorry, I should have chased you, Adolfo the Monday the weekend you said
> you'd look at the patch :-) (the rule is "don't forget to follow up")
>
> Anyway, I'll take over trying to test this (last day of Darcs Hacking
> Sprint 5) unless you happen to be free today (Sunday 17 Oct).
>
> ----------
> assignedto: abuiles -> kowey
> nosy: +kowey
>
> __________________________________
> Darcs bug tracker <bugs@darcs.net>
> <http://bugs.darcs.net/patch360>
> __________________________________
>

Attachments

• unnamed

Hi Adolfo,

I managed to get that test written the day after the sprint.  Please
have a look at it (I'm pushing it to unstable) and let me know what you
think.

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-69c1bd5e36da0deb6cc2c4dd78cd43668eae4a66

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-69c1bd5e36da0deb6cc2c4dd78cd43668eae4a66