[darcs-devel] Help needed with the test harness

[Ben Franksen]

Hi Guillaume

Am 27.08.2017 um 04:36 schrieb Guillaume Hoffmann:
> In http://bugs.darcs.net/patch1591 and http://bugs.darcs.net/patch1592
> I screened almsot all your branch.

Thanks for doing this. Great work.

> The last patch I want to screen is the one about separating patch
> display for screen or for disk.
> 
> I think we need a shell test for this patch. If you cannot write one
> in the coming days, I'll see if I can do it.

This last patch is technically the most challenging. It is also the one
that led to my problems with the test suite. I agree it makes sense to
send it in a separate bundle.

I need to read up on what happened in the meantime before I can respond
in detail.

Cheers
Ben

> 2017-08-25 3:55 GMT-03:00 Ben Franksen <ben.franksen at online.de>:
>> 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
>>
>> _______________________________________________
>> darcs-devel mailing list
>> darcs-devel at osuosl.org
>> https://lists.osuosl.org/mailman/listinfo/darcs-devel