2 patches for repository http://darcs.net:

Wed Jun  2 12:08:17 BST 2010  Eric Kow <kowey@darcs.net>
  * Bug fix: treat filenames in darcs-2 patches as raw bytes.
  (instead of trying to decode from UTF-8)

Wed Jun  2 12:12:19 BST 2010  Eric Kow <kowey@darcs.net>
  * Extend issue1763 test to be more aggressive.

Attachments

• bug-fix_-treat-filenames-in-darcs_2-patches-as-raw-bytes_.dpatch
• unnamed

Any chance you could review this, Reinier? (I figure it's good that it
be you since [a] you and Petr both think clearly about this
Unicode/correctness stuff but [b] you have not been in recent IRC
discussions, so would serve as a third pair of eyes)

It'd be nice if Petr OK'd it too since he has been participating.

- http://irclog.perlgeek.de/darcs/2010-05-31#i_2387317
- http://irclog.perlgeek.de/darcs/2010-06-01#i_2390094

3 patches for repository http://darcs.net:

Oops, forgot a patch.  Do we even *see* inverse conflictors ever?

Wed Jun  2 12:08:17 BST 2010  Eric Kow <kowey@darcs.net>
  * Bug fix: treat filenames in darcs-2 patches as raw bytes.
  (instead of trying to decode from UTF-8)

Wed Jun  2 12:12:19 BST 2010  Eric Kow <kowey@darcs.net>
  * Extend issue1763 test to be more aggressive.

Wed Jun  2 12:20:10 BST 2010  Eric Kow <kowey@darcs.net>
  * Bug fix: inverse conflictors should also raw bytes for filenames.

Attachments

• bug-fix_-treat-filenames-in-darcs_2-patches-as-raw-bytes_.dpatch
• unnamed

Thanks to Guillaume for pointing out that I had inadvertently the verb.

3 patches for repository http://darcs.net:

Wed Jun  2 12:08:17 BST 2010  Eric Kow <kowey@darcs.net>
  * Bug fix: treat filenames in darcs-2 patches as raw bytes.
  (instead of trying to decode from UTF-8)

Wed Jun  2 12:12:19 BST 2010  Eric Kow <kowey@darcs.net>
  * Extend issue1763 test to be more aggressive.

Wed Jun  2 12:28:01 BST 2010  Eric Kow <kowey@darcs.net>
  * Bug fix: inverse conflictors should also use raw bytes for filenames.

Attachments

• bug-fix_-treat-filenames-in-darcs_2-patches-as-raw-bytes_.dpatch
• unnamed

Op woensdag 02 juni 2010 13:16 schreef je:
> Any chance you could review this, Reinier? (I figure it's good that it
> be you since [a] you and Petr both think clearly about this
> Unicode/correctness stuff but [b] you have not been in recent IRC
> discussions, so would serve as a third pair of eyes)

There is a chance I can review it, but there's also a chance that I can't 
because I'm too busy trying to organize a 2.5 release. And it's been lying 
there for more than a week. If anyone else wants to pick it up (s)he's 
welcome.

Reinier

Thanks for not letting the patch go stale.  Petr, how about you, please?

Hi all,

Here's a review of patch 258. Before pushing it, I'd like to know if it's 
intentional that all tests that the bundle adds pass, even when I apply only 
the new tests.

>[Bug fix: treat filenames in darcs-2 patches as raw bytes.
>Eric Kow <kowey@darcs.net>**20100602110817
> Ignore-this: d283d32e56eb92ca2e7e9959d625eb48
> (instead of trying to decode from UTF-8)
>] hunk ./src/Darcs/Patch/Non.hs 70
>
> readNon :: (ReadPatch p, ParserM m) => m (Maybe (Non p C(x)))
> readNon = do Just (Sealed ps) <- readPatch' False
>-             peekfor ":" (do Just (Sealed p) <- readPatch' False
>+             peekfor ":" (do Just (Sealed p) <- readPrim NewFormat
>                              return $ Just $ Non ps p)
>                          (return Nothing)
>

Here we make sure that when reading a "Non", which is a kind of patch in the 
darcs-2 patch handling code, we use the new format. Because Non's only occur 
in darcs-2 patches, and darcs-2 patches should always be written with the new 
format, this is correct.

>hunk ./src/Darcs/Patch/Patchy.hs 203
>
> {-# INLINE bracketedFL #-}
> bracketedFL :: (ReadPatch p, ParserM m) =>
>-               Word8 -> Word8 -> m (Maybe (Sealed (FL p C(x))))
>-bracketedFL pre post =
>+               m (Maybe (Sealed p C(x))) -> Word8 -> Word8 -> m (Maybe
> (Sealed (FL p C(x)))) +bracketedFL parser pre post =
>     peekforw pre bfl (return Nothing)
>hunk ./src/Darcs/Patch/Patchy.hs 206
>-        where bfl :: (ReadPatch p, ParserM m) => m (Maybe (Sealed (FL p
> C(x)))) -              bfl = peekforw post (return $ Just $ Sealed NilFL)
>-                                  (do Just (Sealed p) <- readPatch' False
>+        where bfl = peekforw post (return $ Just $ Sealed NilFL)
>+                                  (do Just (Sealed p) <- parser
>                                       Just (Sealed ps) <- bfl
>                                       return $ Just $ Sealed (p:>:ps))
>

Here bracketedFL is parameterised with respect to the code that parses the 
string between 'pre' and 'post'.

>hunk ./src/Darcs/Patch/Read.hs 85
>
> instance ReadPatch Patch where
>  readPatch' want_eof
>-   = do mps <- bracketedFL (fromIntegral $ fromEnum '{') (fromIntegral $
> fromEnum '}')
> +   = do mps <- bracketedFL (readPatch' False) (fromIntegral
> $ fromEnum '{') (fromIntegral $ fromEnum '}')
>          case mps of
>           Just (Sealed ps) -> return $ Just $ Sealed $ ComP ps
>           Nothing -> do s <- peekInput

Use the old format for 'Patch', which is the old-fashioned patch type.

>hunk ./src/Darcs/Patch/Real.hs 746
>                  --let tracePeek x = do y <- peekInput
>                  --                     traceDoc (greenText x $$ greenText
> (show $ BC.unpack y)) return () i <- readNons
>-                 Just (Sealed ps) <- bracketedFL (fromIntegral $ fromEnum
> '[') (fromIntegral $ fromEnum ']')
> +                Just (Sealed ps) <- bracketedFL (readPrim NewFormat) 
(fromIntegral $ fromEnum '[') (fromIntegral $ fromEnum ']')
>
>                  Just p <- readNon
>                  return $ Just $ Sealed $ Conflictor i (unsafeCoerceP ps) p

Use the new format when reading conflictors using the ReadPatch instance for 
RealPatch (which is the darcs-2 patch type afaik).

>hunk ./src/Darcs/Patch/Real.hs 753
>           Just "rotcilfnoc" ->
>               do work myLex
>                  i <- readNons
>-                 Just (Sealed ps) <- bracketedFL (fromIntegral $ fromEnum
> '[') (fromIntegral $ fromEnum ']')
> +                 Just (Sealed ps) <-
> bracketedFL (readPrim NewFormat) (fromIntegral $ fromEnum '[')
> (fromIntegral $ fromEnum ']') Just p <- readNon
>                  return $ Just $ Sealed $ InvConflictor i ps p
>           _ -> do mp <- readPrim NewFormat

And also use the new format when reading inverse conflictors using the 
ReadPatch instance for RealPatch.

>[Extend issue1763 test to be more aggressive.
>Eric Kow <kowey@darcs.net>**20100602111219
> Ignore-this: 6ea29905c8c4accf4d3ca4c1748f4469
>] hunk ./tests/issue1763-pull-fails-on-non-ascii-filenames.sh 29
> ## SOFTWARE.
>
> . lib
>-rm -rf R S
>+rm -rf R
> darcs init --repo R
>hunk ./tests/issue1763-pull-fails-on-non-ascii-filenames.sh 31
>-darcs init --repo S
>
> # Set up a repo with 3 patches to a non-ASCII-named file
> cd R
>hunk ./tests/issue1763-pull-fails-on-non-ascii-filenames.sh 39
> darcs record -a -m "Add"
> echo hi >> kitöltés.lisp
> darcs record -a -m "First edit"
>+cd ..
>+
>+rm -rf S S2 S3
>+darcs get R S
>+darcs get R S2
>+darcs get R S3
>+
>+cd R
> echo hi >> kitöltés.lisp
> darcs record -a -m "Second edit"
>hunk ./tests/issue1763-pull-fails-on-non-ascii-filenames.sh 49
>+cd ..
>
> # From another repo, pull the first two, edit, pull the third to get a
> # conflict, pull again to get the crash
>hunk ./tests/issue1763-pull-fails-on-non-ascii-filenames.sh 53
>-cd ../S
>-echo yyn | darcs pull --interactive ../R
>+cd S
> echo hello >> kitöltés.lisp
> darcs record -a -m "My edit"
> darcs pull -a ../R
>hunk ./tests/issue1763-pull-fails-on-non-ascii-filenames.sh 58
> darcs pull -a ../R
>+cd ..
>
>hunk ./tests/issue1763-pull-fails-on-non-ascii-filenames.sh 60
>+# duplicates
>+cd S2
>+echo hi >> kitöltés.lisp
>+darcs record -a -m "My duplicate edit"
>+darcs pull -a ../R
>+darcs pull -a ../R
>+cd ..
>+
>+# nons
>+cd S3
>+echo hello >> kitöltés.lisp
>+darcs record -a -m "My conflicting edit"
>+echo hello >> kitöltés.lisp
>+touch non-kitöltés.lisp
>+darcs add non-kitöltés.lisp
>+darcs record -a -m "My continuation of the conflict"
>+darcs pull -a ../R
>+darcs pull -a ../R
>+cd ..

Add 3 more scenarios. I don't understand what the third one wants to check. It 
has a conflicting edit and then records something unrelated to a file named 
non-kitöltés.lisp. Is that when Non's arise?

What's more, all 3 tests succeed for me even when the other 2 patches have not 
been applied! Is that intentional?

>[Bug fix: inverse conflictors should also use raw bytes for filenames.
>Eric Kow <kowey@darcs.net>**20100602112801
> Ignore-this: e66332fcf70600acea05b9c495a7afe5
>] hunk ./src/Darcs/Patch/Real.hs 723
>         blueText "rotcilfnoc" <+> showNons i <+> blueText "[]" $$ showNon p
>     showPatch (InvConflictor i cs p) =
>         blueText "rotcilfnoc" <+> showNons i <+> blueText "[" $$
>-        showPatch cs $$
>+        showPrimFL NewFormat cs $$
>         blueText "]" $$
>         showNon p
>     showContextPatch (Normal p) = showContextPatch p

Also write inverse conflictors in the new format. Before, conflictors where 
written in the new format and inverse coflictors in the old format :-S.

Reinier

If I don't solve the testing problem, I'd suggest applying this patch bundle
in time for darcs 2.5 alpha

On Sun, Jun 13, 2010 at 19:09:31 +0000, Reinier Lamers wrote:
> Here's a review of patch 258. Before pushing it, I'd like to know if it's 
> intentional that all tests that the bundle adds pass, even when I apply only 
> the new tests.

Ah sorry about that! I've had a look, and to be honest, I'm slightly
confused because I distinctly remember failing bits when you apply
some subset of the bugfix patch but not the whole thing.  But I can't
reproduce it.

> Use the new format when reading conflictors using the ReadPatch instance for 
> RealPatch (which is the darcs-2 patch type afaik).

That's my understanding too, as documented in
  http://wiki.darcs.net/DarcsInternals/Patches

We've inherited the Darcs code from one maintainer.  We must learn it.
It will take some time.

> And also use the new format when reading inverse conflictors using the 
> ReadPatch instance for RealPatch.

I wonder if we'll ever see inverse conflictors in real life.

> >+# nons
> >+cd S3
> >+echo hello >> kitöltés.lisp
> >+darcs record -a -m "My conflicting edit"
> >+echo hello >> kitöltés.lisp
> >+touch non-kitöltés.lisp
> >+darcs add non-kitöltés.lisp
> >+darcs record -a -m "My continuation of the conflict"
> >+darcs pull -a ../R
> >+darcs pull -a ../R
> >+cd ..
> 
> Add 3 more scenarios. I don't understand what the third one wants to check. It 
> has a conflicting edit and then records something unrelated to a file named 
> non-kitöltés.lisp. Is that when Non's arise?

I confess that this was sort of random because I didn't really
understand the patch structure well enough.  It was just me trying
to make something complicated happen.

I think what I was vaguely after was the context part of a Non patch.

    conflictor [
    hunk ./kit<U+00C3><U+00B6>lt<U+00C3><U+00A9>s.lisp 2
    +hello
    ]
    <-- the stuff that would go here
    |:
    hunk ./kit<U+00C3><U+00B6>lt<U+00C3><U+00A9>s.lisp 2
    +hi

So I'm going to see now if I can construct such a scenario.

> What's more, all 3 tests succeed for me even when the other 2 patches have not 
> been applied! Is that intentional?

No, sorry! :-/

I suspect at this point I was no longer relying on the test per se but
just looking at the darcs changes output on {S,S2,S3} before and after this
patch.  They should output something consistent for the filename (keeping in
mind that this is just a *read* test).  To make the test fail, I'm going to
throw in a test that runs uniq on the filenames listed in the darcs changes
output making sure we only get one filename.

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9

Hi Reinier,

3 patches for repository http://darcs.net:

I think the other two patches in this bundle are the same as before, but
I've improved the test patch below so that (a) it's actually hunting for
the error my patch solves -- keeping in mind that my patch only affects
*reading* of patches -- and (b) so that the silly attempt at a complicated
conflict is removed until further study.

Tue Jun 15 14:15:44 BST 2010  Eric Kow <kowey@darcs.net>
  * Slightly more aggresive and extendable issue1763 test.

About the complicated conflict, I think I was looking for two things:
- a non context
- a conflictor 'ix' patch (one that conflicts with 'x' and also another
  patch in the repository, evidenced by double curly braces).

I bet that getting the 'ix' patch will be easy.  For the Non context,
I don't yet know enough about Darcs.

Thanks,

Eric
 
PS.  Working on this seemingly unrelated thing is great because I'm now a bit
more comfortable with some of the darcs-2 moving parts than in 2007!

Attachments

• bug-fix_-inverse-conflictors-should-also-use-raw-bytes-for-filenames_.dpatch
• unnamed

Hi,

These patches are ready to go in, except the "export LC_ALL=..." issue. See 
below for more explanation.

>New patches:

The two non-test patches indeed haven't changed so I'll skip those.

>[Slightly more aggresive and extendable issue1763 test.
>Eric Kow <kowey@darcs.net>**20100615131544
> Ignore-this: 797a11b3575fe4062e1a280f432e6d40
>] hunk ./tests/issue1763-pull-fails-on-non-ascii-filenames.sh 30
>
> . lib
>
>-abort_windows
>+abort_windows # FIXME! We should figure out what's going on here

I don't like such comments, but then, we have to hint somewhere that there's 
not really a good reason why this shouldn't work on Windows. Making a ticket 
clutters the issue tracker, so we make such a comment.

The reason that it clutters the issue tracker is that it'll never get done 
anyway. We really need some Windows serfs to obey the Windows Czar :).


>hunk ./tests/issue1763-pull-fails-on-non-ascii-filenames.sh 34
>-darcs init --repo S
>+
>+function check_consistent_filename {
>+  export LC_ALL='en.UTF8'
>+  count=`darcs changes -v | grep 'hunk .*\.lisp' | sed -e 's/.*hunk //' -e
> 's/.lisp.*//' | sort | uniq | wc -l`
> +  test $count -eq 1
>+}
>

This functions checks if there is only one representation in the output of 
'darcs changes'. The idea is that if there are multiple ones, it must be an 
encoding problem (like one well-encoded and one double-encoded with UTF-8).

It has a problem, however, in that the locale "en.UTF8" is very unlikely to 
exist. "en_US.UTF8" does exist and is the default on most Linux distros, but I 
don't think that it is actually required to be present. And then there are 
also operating systems that aren't Linux distros at all. Do we have to change 
locale at all? The file names should be equal, no matter what the locale is, 
shouldn't they?

> # Set up a repo with 3 patches to a non-ASCII-named file
> cd R
>hunk ./tests/issue1763-pull-fails-on-non-ascii-filenames.sh 48
> darcs record -a -m "Add"
> echo hi >> kitöltés.lisp
> darcs record -a -m "First edit"
>+cd ..
>+
>+rm -rf S S2 S3
>+darcs get R S
>+darcs get R S2
>+darcs get R S3
>+
>+cd R
> echo hi >> kitöltés.lisp
> darcs record -a -m "Second edit"

So yeah, we make three copies of a repo with 2 patches and then make a third 
patch in the original.

>hunk ./tests/issue1763-pull-fails-on-non-ascii-filenames.sh 58
>+cd ..
>
> # From another repo, pull the first two, edit, pull the third to get a
> # conflict, pull again to get the crash

>hunk ./tests/issue1763-pull-fails-on-non-ascii-filenames.sh 62
>-cd ../S
>-echo yyn | darcs pull --interactive ../R
>+cd S
> echo hello >> kitöltés.lisp
> darcs record -a -m "My edit"
> darcs pull -a ../R
>hunk ./tests/issue1763-pull-fails-on-non-ascii-filenames.sh 67
> darcs pull -a ../R
>+check_consistent_filename
>+cd ..
>

Adding check_consistent_filename shouldn't be necessary because the earlier 
case actually crashed darcs in case something was wrong. But it doesn't hurt 
either.

>hunk ./tests/issue1763-pull-fails-on-non-ascii-filenames.sh 70
>+# duplicates
>+cd S2
>+echo hi >> kitöltés.lisp
>+darcs record -a -m "My duplicate edit"
>+darcs pull -a ../R
>+darcs pull -a ../R
>+check_consistent_filename
>+cd ..

This checks duplicate patches. This test indeed fails without the functional 
changes and succeeds with them.

Reinier

3 patches for repository http://darcs.net:

Fri Jun 18 23:51:04 BST 2010  Eric Kow <kowey@darcs.net>
  * Slightly more aggresive and extendable issue1763 test.

Another modification of the test, this time setting the locale to C and in one
place since there's no block scope on environment variables.

On Fri, Jun 18, 2010 at 21:32:05 +0000, Reinier Lamers wrote:
> >+function check_consistent_filename {
> >+  export LC_ALL='en.UTF8'
> >+  count=`darcs changes -v | grep 'hunk .*\.lisp' | sed -e 's/.*hunk //' -e
> > 's/.lisp.*//' | sort | uniq | wc -l`
> > +  test $count -eq 1
> 
> It has a problem, however, in that the locale "en.UTF8" is very unlikely to 
> exist. "en_US.UTF8" does exist and is the default on most Linux distros, but I 
> don't think that it is actually required to be present.

> The file names should be equal, no matter what the locale is,
> shouldn't they?

So I think I just worked a little something out.

1. The trickiness is not the test failing, but it failing to fail.
   That is, it incorrectly passes with my en_GB.UTF8 locale.

   I'm not entirely sure why having a UTF-8 locale causes the test
   to spuriously pass... and would be happier with a good story for it.
   How does the locale affect the printing of darcs changes output
   again? :-/

2. 'en.UTF8' actually does not exist on my machine; the reason it seemed 
   to make the test do the right thing by virtue of being a nonsense
   locale, causing some sort of default to C?  Note that LC_ALL
   isn't actually set in my environment, but LANG is (en_GB.UTF-8)

Attachments

• bug-fix_-inverse-conflictors-should-also-use-raw-bytes-for-filenames_.dpatch
• unnamed

This patch bundle (with 3 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-294172bdfd85855d2fba3be313f0e6a68d111d85

This patch bundle (with 3 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-294172bdfd85855d2fba3be313f0e6a68d111d85