[darcs-devel] [patch1901] fix interpretation of bundles as patchsets

[Ganesh Sittampalam]

Ganesh Sittampalam <ganesh at earth.li> added the comment:

>   * fix interpretation of bundles as patchsets

OK. I think parseAndInterpretBundle really belongs in D.P.Bundle, not 
D.UI.C.Apply, though it's not a big deal.

I had to think a bit about the 'unsafeCoercePStarts' in interpretBundle. 
They are essentially relying on the behaviour of patchSetSplit when the 
bundle was originally created, that the context would either start at a 
clean tag or at Origin. That might benefit from some comments.

(While writing this, I briefly wondered if this would go wrong if the repo 
had an unclean tag at the very start of the repo, but that's not actually 
possible as an unclean tag must have patches before it.)

I don't think there are any existing tests to show the old broken behaviour, 
so I wrote patch1921 which shows it going wrong when the tag the bundle is 
based on is unclean in the target repo. If you have any ideas how to make a 
test that breaks when the tag is missing, that'd be great. I haven't 
screened patch1921 yet but will do so soon unless it turns out I've 
misunderstood something fundamental about what to test.


>  * rename myLex' to lexWord

Fine

>  * re-write Darcs.Patch.Bundle using Darcs.Util.Parser

Fine, that definitely looks a lot more readable (I didn't compare it in 
detail and certainly wouldn't have noticed the issue you later found with 
GPG, but the overall structure looks good.)

There appear to be some unrelated cleanups of the constraints in instance 
ReadPatch PatchInfoAndG which are also fine even if they don't logically 
belong in this patch. (Maybe I missed some connection or maybe they just 
ended up in it by accident.)

>  * fix scanContextFile in the same way as scanBundle

Fine

>  * remove old unused functions from Darcs.Patch.Bundle

Fine

>  * cleanup Darcs.Patch.Bundle

Fine. I guess that makeBundle2 was originally used externally with two 
independently read lists of patches to guarantee constant memory usage, but 
since it's not exported now there's no point in that interface.

>  * move patchFilename from D.P.Bundle to D.UI.C.Util
 
Fine. I guess it is a bit debatable what layer this logic lives in, but I 
have no particular objection to moving it to the UI. D.UI.C.Util is perhaps 
turning into a big set of unrelated things, but I don't have any particular 
proposal on how to split it up rationally.
 
>   * adapt tests/send.sh to new behavior of --output options

Fine

>  * fix decoding of gpg clearsigned bundles

Fine. I'm not sure how much we should care about this functionality. I 
certainly don't use it myself.

----------
status: needs-review -> accepted-pending-tests

__________________________________
Darcs bug tracker <bugs at darcs.net>
<http://bugs.darcs.net/patch1901>
__________________________________