[darcs-devel] Help needed with the test harness

Ben Franksen ben.franksen at online.de
Fri Aug 25 06:55:29 UTC 2017


Am 23.08.2017 um 20:58 schrieb Guillaume Hoffmann:
> I have read the first big patch, "Refactor text encoding in darcs".
> It's great that we get rid of the RenderMode parameter all over the
> place!> 

> 
> But I have some comments.
> 
> First, after this patch alone, the test switch-encoding.sh fails at
> 'darcs log -v' (`darcs: darcs-encode: commitBuffer: invalid argument
> (invalid character)`). Then this gets fixed with "fall back to UTF8
> encoding if encoding a la locale fails (Posix only)". Is it to be
> expected?

Yes. (I think I wrote most of the test after the first patch).

> Then, I'm not sure whether we should accept the branch as it is, and
> then fix the bugs that appear, or try to rebase the patches into more
> fine-grained one before merging. For instance, I see that the patch
> "Refactor text encoding" contains hunks that could be part of a more
> trivial patch:
> 
>      * import re-ordering at some places
>      * replacing String by FilePath in Darcs.Util.Lock
>      * Darcs.Util.Lock.withTempDir comment
>      * new debug messages (in Darcs.Util.Index,
> Darcs.Util.Tree.Hashed, Darcs.UI.Commands, D.UI.C.Record,
> D.R.Old/HashedIO/Hashed)
>      * indentation and replace catchall by cathIOError in Darcs.Util.Files
>      * replace getFileStatus by doesDirectoryeallyExist in D.UI.C.Optimize
>      * help string of `darcs apply`
>      * removal of copySources haddock

Yes. Apropos debug messages: I wonder if we should perhaps add an
optional numerical level argument to --debug (defaulting to 1 if not
given). When I run darcs with --debug I am often swamped with (mostly
cache related) messages that make it hard to spot the interesting things.

> If you want, during these days I could take care of this rebase. I
> find this branch important and I want it to be merged not too late :-)

Thanks, I would be very grateful if you could do that. I am in my
holidays and don't have much time for hacking ATM, at least not
continuous time. (I have written this message spread over two days,
sorry for the delay.)

> Then about the rest of the patches:
> 
> * does patch "use piName instead of (BC.unpack . _piName) in two
> modules" semantically depend on "Refactor text encoding"?

I first thought so but on further inspection (and testing): no.

> * "re-implement en/decodeUtf8": patch long comment should mention
> where the original functions come from

It's somewhat implicit from the long description which mentions
Data.Text but the origin could be made clearer.

> * "in Darcs.UI.Commands import setEnv from System.Environment" has
> code style hunks

Yes. I guess they weren't easy to separate from the main change. Perhaps
the whole environment setting business should be separated out and
applied to screened independently (unless I have overlooked some
semantic dependency).

> * does "removed redundant class constraint for
> Darcs.UI.Commands.setEnvDarcsFiles" semantically depend on previous
> patch? 

Not at all, it's just a simple cleanup change.

> maybe it could be merged into it?

No it should be pulled out of the branch, same as the other
(semantically) independent changes.

Cheers
Ben



More information about the darcs-devel mailing list