[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