This should fix issue1726, Daniel's updated testcase (patch135) passes now.
The second patch is a minor cleanup of stuff I found while tracking down the issue.


Sun Jan 10 21:18:36 CET 2010  David Markvica <david@blueshellturtle.com>
  * remove the hardcoded string "_darcs" in some places and replace it with the global darcsdir

Sun Jan 10 22:29:32 CET 2010  David Markvica <david@blueshellturtle.com>
  * Resolve issue1726: Files with _darcs prefix are always boring

Attachments

• remove-the-hardcoded-string-__darcs_-in-some-places-and-replace-it-with-the-global-darcsdir.dpatch
• unnamed

Hi,

davidm <bugs@darcs.net> writes:
> Sun Jan 10 21:18:36 CET 2010  David Markvica <david@blueshellturtle.com>
>   * remove the hardcoded string "_darcs" in some places and replace it with the global darcsdir
I think there is certain ambivalence about the usefulness of the
darcsdir global. But let's put that aside (I don't particularly care.)

> Sun Jan 10 22:29:32 CET 2010  David Markvica <david@blueshellturtle.com>
>   * Resolve issue1726: Files with _darcs prefix are always boring

> hunk ./src/Darcs/Repository/Prefs.lhs 240
>  isDarcsdir "" = True
>  isDarcsdir ".." = True
>  isDarcsdir "../" = True
> -isDarcsdir fp = darcsdir `isPrefixOf` fp
> +isDarcsdir fp = darcsdir == fp
I don't think this is correct. I'd say:

isDarcsdir "_darcs" = True
isDarcsdir fp = "_darcs/" `isPrefixOf` fp

Another option would be to use System.FilePath to break up fp and check
that (take 1 $ map dropTrailingPathSeparator $ splitPath fp) ==
["_darcs"] -- this is probably safest, although most elaborate.

Yours,
   Petr.

I'll just throw in an extra suggestion to add a test case for any regressions
that would have been introduced by
isDarcsdir fp = darcsdir == fp

Perhaps that this will cause it to miss the fact that _darcs/foo is a darcs dir?

Unless I wrote it incorrectly, the regression test in patch135 already checks to 
make sure _darcs/foo gets skipped:

+touch _darcs/foo
+not darcs add _darcs/foo

Daniel Dickison <bugs@darcs.net> writes:
> Unless I wrote it incorrectly, the regression test in patch135 already checks to 
> make sure _darcs/foo gets skipped:
>
> +touch _darcs/foo
> +not darcs add _darcs/foo

This correctly tests add, but I suspect that add is not the only code
that ever uses the boring filter in possibly fancy ways. I would expect
a more thorough argument as for why this is safe, especially since the
semantic change is easily avoided.

The problem is that there are some ... creative ... pieces of code in
darcs and it's not exactly good idea to change something that could have
unintended consequences unless you are very sure that it won't. Eric
could probably extol the virtues of conservativism much better than me
though.

Yours,
   Petr.

On Jan 12, 2010, at 4:55 PM, Petr Ročkai wrote:

> Petr Ročkai <me@mornfall.net> added the comment:
> 
> Daniel Dickison <bugs@darcs.net> writes:
>> Unless I wrote it incorrectly, the regression test in patch135 already checks to 
>> make sure _darcs/foo gets skipped:
>> 
>> +touch _darcs/foo
>> +not darcs add _darcs/foo
> 
> This correctly tests add, but I suspect that add is not the only code
> that ever uses the boring filter in possibly fancy ways. I would expect
> a more thorough argument as for why this is safe, especially since the
> semantic change is easily avoided.
> 
> The problem is that there are some ... creative ... pieces of code in
> darcs and it's not exactly good idea to change something that could have
> unintended consequences unless you are very sure that it won't. Eric
> could probably extol the virtues of conservativism much better than me
> though.

That makes sense.  I assumed whatever code detects the _darcs directory for `add' is the same in every other situation, but I'm not familiar with darcs internals.

If you wouldn't mind a possibly naïve question, why is it necessary to special case the _darcs directory when the default boring file already contains the following line?
(^|/)_darcs($|/)

Daniel Dickison <bugs@darcs.net> writes:
> If you wouldn't mind a possibly naïve question, why is it necessary to
> special case the _darcs directory when the default boring file already
> contains the following line?  (^|/)_darcs($|/)
I assume this is because the boring file can be overriden. If someone
managed to create patches referring to files under _darcs (by removing
that _darcs entry from their boring file), I can only assume that BAD
THINGS would happen.

Yours,
   Petr.

Could we please get a status update?  Looks like this one has been sitting 
since about mid-January.

Thanks!
Jason

On Mar 17, 2010, at 11:03 PM, Jason Dagit wrote:

> Jason Dagit <dagitj@gmail.com> added the comment:
> 
> Could we please get a status update?  Looks like this one has been sitting 
> since about mid-January.
> 
> Thanks!
> Jason

Back in January I added new test cases to patch135 to test 'whatsnew', 'record', and 'add' with various variations of _darcs and _darcs/foo.  Let me know if there are other possible regressions that it needs test for. (Though I can't really help with the code, unfortunately.)

Daniel

On Thu, Mar 18, 2010 at 5:56 AM, Daniel Dickison <bugs@darcs.net> wrote:

>
> Daniel Dickison <danieldickison@gmail.com> added the comment:
>
> On Mar 17, 2010, at 11:03 PM, Jason Dagit wrote:
>
> > Jason Dagit <dagitj@gmail.com> added the comment:
> >
> > Could we please get a status update?  Looks like this one has been
> sitting
> > since about mid-January.
> >
> > Thanks!
> > Jason
>
> Back in January I added new test cases to patch135 to test 'whatsnew',
> 'record', and 'add' with various variations of _darcs and _darcs/foo.  Let
> me know if there are other possible regressions that it needs test for.
> (Though I can't really help with the code, unfortunately.)
>

So then, if the tests pass we can be reasonably confident that this fix is
ready to go?

Jason

Attachments

• unnamed

On Mar 18, 2010, at 11:52 AM, Jason Dagit wrote:

> 
> On Thu, Mar 18, 2010 at 5:56 AM, Daniel Dickison <bugs@darcs.net> wrote:
> 
> Daniel Dickison <danieldickison@gmail.com> added the comment:
> 
> On Mar 17, 2010, at 11:03 PM, Jason Dagit wrote:
> 
> > Jason Dagit <dagitj@gmail.com> added the comment:
> >
> > Could we please get a status update?  Looks like this one has been sitting
> > since about mid-January.
> >
> > Thanks!
> > Jason
> 
> Back in January I added new test cases to patch135 to test 'whatsnew', 'record', and 'add' with various variations of _darcs and _darcs/foo.  Let me know if there are other possible regressions that it needs test for. (Though I can't really help with the code, unfortunately.)
> 
> So then, if the tests pass we can be reasonably confident that this fix is ready to go?
> 
> Jason

I'm far from being a darcs expert so I was hoping someone more knowledgeable could chime in here.  I just don't know if these changes are safe or if the tests cover enough situations.

I intend to apply the refactor patch (now that we've reached consensus
about the global darcsdir).

Petr's review for the other patch requested an amend, so which I believe
is still relevant.

Also, Salvatore pointed out that the filepath code in Darcs is pretty
horrible in some cases.  Petr's right that we should be quite
fine-toothed-comb-ish about this.  It may be worth checking out bugs on
the tracker with the FilePath topic.

Hi David,  I'm trying to give the patch tracker a bit of a tidy (as we
head towards the Darcs 2.5 release). Have you had a chance to amend this
patch as Petr suggested in msg9786?  Thanks!

Hej Eric,

On 2010-06-24, at 14:57, Eric Kow wrote:
> Hi David,  I'm trying to give the patch tracker a bit of a tidy (as we
> head towards the Darcs 2.5 release). Have you had a chance to amend this
> patch as Petr suggested in msg9786?  Thanks!

I'm having trouble with darcs send/msmtp, so I'm attaching the amended patch.

Attachments

• issue1726.dpatch

Hi Ganesh, Any chance you could give David's amended script
(issue1726.dpatch) a quick review? Thanks!

Sorry, missed this earlier. It looks obviously correct.

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

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