1 patch for repository http://darcs.net/screened:

patch d1bbdb5f1f3cea279a92649f989f68f63c37f71e
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Sep  1 15:19:00 CEST 2019
  * fix interpretation of bundles as patchsets
  
  We previously created invalid patchsets when a tag was present in the
  context of a bundle. This worked (sort of) due to laziness but only if we
  actually have that tag in our repo. If we don't then this rather dirty hack
  interprets the bundle in a wrong context, i.e. Origin. Depending on how
  findCommonAndUncommon is implemented we either get immediate errors ("cannot
  commute common patches") or it hangs indefinitely trying to perform huge
  amounts of bugus commutes of patches that aren't in their rightful context.
  
  The same bug is still present in scanContextFile.

Attachments

• fix-interpretation-of-bundles-as-patchsets.dpatch
• patch-preview.txt
• unnamed

> 1 patch for repository http://darcs.net/screened:
> 
> patch d1bbdb5f1f3cea279a92649f989f68f63c37f71e
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Sun Sep  1 15:19:00 CEST 2019
>   * fix interpretation of bundles as patchsets
>   
>   We previously created invalid patchsets when a tag was present in the
>   context of a bundle. This worked (sort of) due to laziness but only if we
>   actually have that tag in our repo. If we don't then this rather dirty hack
>   interprets the bundle in a wrong context, i.e. Origin. Depending on how
>   findCommonAndUncommon is implemented we either get immediate errors ("cannot
>   commute common patches") or it hangs indefinitely trying to perform huge
>   amounts of bugus commutes of patches that aren't in their rightful context.
>   
>   The same bug is still present in scanContextFile.

I am working on a re-write of Darcs.Patch.Bundle to use
Darcs.Util.Parser instead of re-implementing parser combinators inlined
in the code which I find horribly unreadable. In the end this will also
contain a fix of scanContextFile along the same lines as the fix I sent
above.

> I am working on a re-write of Darcs.Patch.Bundle to use
> Darcs.Util.Parser instead of re-implementing parser combinators inlined
> in the code which I find horribly unreadable. In the end this will also
> contain a fix of scanContextFile along the same lines as the fix I sent
> above.

I have almost completed this and also the fix to scanContextFile, which
now requires another parameter (a valid PatchSet relative to which we
can anchor a tag in the context or fail if teh tag doesn't exist).

The only remaining use of the old scanContextFile is for the --context
option to 'darcs send' (internally the option is named sendToContext).
Before I start getting my hands dirty with refactoring the send command:
is this option actually something worth keeping? What is the use case?

On 03/09/2019 09:47, Ben Franksen wrote:

> The only remaining use of the old scanContextFile is for the --context
> option to 'darcs send' (internally the option is named sendToContext).
> Before I start getting my hands dirty with refactoring the send command:
> is this option actually something worth keeping? What is the use case?

I don't use it: if I want to send against a specified context (which I
do need to do sometimes) then I just clone my repository, unpull the
patches in the clone, and then send to that clone. In principle
--context might provide a nicer workflow but I don't know how I'd create
the context file.

>> The only remaining use of the old scanContextFile is for the --context
>> option to 'darcs send' (internally the option is named sendToContext).
>> Before I start getting my hands dirty with refactoring the send command:
>> is this option actually something worth keeping? What is the use case?
> 
> I don't use it: if I want to send against a specified context (which I
> do need to do sometimes) then I just clone my repository, unpull the
> patches in the clone, and then send to that clone.

Ah, now I understand what it is for. I was wondering what it means to
send against a specific context, but I understand now that it means the
bundle doesn't include patches that are in the context (but still
contains any dependencies that are not in the context).

Indeed my usual workflow for that situation is similar.

> In principle
> --context might provide a nicer workflow but I don't know how I'd create
> the context file.

'darcs log --context > filename' should do it. But that doesn't help
much because you cannot deselect patches with --context, nor are any of
the matching options honored. So I guess this is currently quite useless
but that doesn't mean the idea is bad per se.

Here is the complete fix including re-write of the parsing using
Darcs.Util.Parser aka attoparsec. Everything tests fine. I am glad I took
the opportunity to clean this pice of code up, that has long been on my list
anyway. Adapting the --contest option for darcs send was easy once I
understood that we must interpret the context relative to our own
repo (we don't have anything else as reference).

I think I will screen this soon, unless you can see any obvious problems.

7 patches for repository http://darcs.net/screened:

patch d1bbdb5f1f3cea279a92649f989f68f63c37f71e
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Sep  1 15:19:00 CEST 2019
  * fix interpretation of bundles as patchsets
  
  We previously created invalid patchsets when a tag was present in the
  context of a bundle. This worked (sort of) due to laziness but only if we
  actually have that tag in our repo. If we don't then this rather dirty hack
  interprets the bundle in a wrong context, i.e. Origin. Depending on how
  findCommonAndUncommon is implemented we either get immediate errors ("cannot
  commute common patches") or it hangs indefinitely trying to perform huge
  amounts of bugus commutes of patches that aren't in their rightful context.
  
  The same bug is still present in scanContextFile.

patch 34e2246e27530d2730a9add0ec34fe1bd0e681ba
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep  3 00:03:07 CEST 2019
  * rename myLex' to lexWord

patch 46459ddcc8ded39d0fc00794485b7dd9f45133c8
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Sep  2 22:20:35 CEST 2019
  * re-write Darcs.Patch.Bundle using Darcs.Util.Parser
  
  This does not delete any of the old code yet. It merely renames the old
  parseBundle to parseBundleOld. The old code will be deleted in a later
  patch. It also doesn't re-implement scanContextFile yet.

patch 3538d094f4287c4fd4669085f8b1ed34ea14082f
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep  3 11:00:08 CEST 2019
  * fix scanContextFile in the same way as scanBundle
  
  Again, I just renamed the old function (to readContextFileOld) and provide a
  new one (readContextFile). The new function takes an extra PatchSet argument
  so we can check if a tag in the context actually exists. The old function
  isn't used anywhere and will be removed.

patch a490c66b93d7160a605fdc35172b3d4b757ddd33
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep  3 18:46:06 CEST 2019
  * remove old unused functions from Darcs.Patch.Bundle

patch 66cd950b8d127b343216548f941163ecc7925010
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep  3 19:09:49 CEST 2019
  * cleanup Darcs.Patch.Bundle
  
  This renames makeBundleN to makeBundle and simplifies it by inlining
  makeBundle2, and by removing the duplication of the patch list which didn't
  seem to have any purpose. A few other definitions are moved nearer to where
  they are used or even turned into local definitions. Import lists are also
  re-sorted.

patch 4df8081c011775f26d6c8457899ee5584e436be5
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Sep  3 19:30:22 CEST 2019
  * move patchFilename from D.P.Bundle to D.UI.C.Util
  
  Also use getUniqueDPatchName in send and pull commands, which builds on
  patchFilename but checks that it doesn't exist yet and otherwise makes it
  unique.

Attachments

• fix-interpretation-of-bundles-as-patchsets.dpatch
• patch-preview.txt
• unnamed

Screening it is fine with me.

1 patch for repository http://darcs.net/screened:

patch d845a6bf57abc13afe65df6a96f8faef85ff1421
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Sep  9 11:33:30 CEST 2019
  * adapt tests/send.sh to new behavior of --output options
  
  The patch "move patchFilename from D.P.Bundle to D.UI.C.Util" (which this
  one explicitly depends on) changed the behavior of --output-auto-name such
  that we no longer overwrite existing files. Also test that we /do/ overwrite
  an existing file if the output filename is given explicitly with --output.

Attachments

• adapt-tests_send_sh-to-new-behavior-of-__output-options.dpatch
• patch-preview.txt
• unnamed

1 patch for repository http://darcs.net/screened:

patch 55a8abed12cae6ecd032f92e9ba404c5c1b4f405
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Sep 15 16:10:10 CEST 2019
  * fix decoding of gpg clearsigned bundles
  
  This was broken after patch 46459ddcc8ded39d0fc00794485b7dd9f45133c8
    * re-write Darcs.Patch.Bundle using Darcs.Util.Parser
  
  In fact, gpg does not emit the string "-----END PGP SIGNED MESSAGE-----"
  when it clear-signs a file. Instead the signed text ends with
  "-----BEGIN PGP SIGNATURE-----" followed by the signature, followed by
  "-----END PGP SIGNATURE-----". The previous code "worked" only because it
  used takeWhile i.e. it reads until the end of the file (and later discards
  trailing stuff after the last patch). We should really use gpg --verify by
  default to decode a gpg clear-signed bundle. We should also add tests for
  generating and parsing signed bundles, even though this is not trivial to
  orchestrate.

Attachments

• fix-decoding-of-gpg-clearsigned-bundles.dpatch
• patch-preview.txt
• unnamed

>   [...] We should really use gpg --verify by
>   default to decode a gpg clear-signed bundle.

To explain this perhaps a bit cryptic remark: we currently decode a gpg
clear-signed patch bundle ourselves. This is stupid since gpg happily
does that for us, using

  gpg --verify --no-default-keyring -o <output file name>
signed-bundle.dpatch

This checks that the signature is "valid" in the sense that whoever
signed it must have had access to the private key. It also checks that
the content has not been tampered with since it was signed, which is a
nice bonus. The return value of this command is 1 if there was an error
during validation and 2 if the input file isn't gpg clear-signed. So we
can run that and if the return code is 2, then we just use the original
file contents.

Of course this cannot check that the bundle was signed by the person who
they claim to be. For this we need to pass it a keyring wherein we state
our trust of the key. We have extra options for that in darcs. But
currently we can only validate the signature if we also pass a keyring.

OTOH, if we don't have a gpg installed on the system, we need to fall
back to decoding a clear-signed bundle ourselves anyway, so perhaps this
is not such a big win...

>   * 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.

>>   * 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 agree and actually planned to do that in a follow-up.

> 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.

Yes. The way we truncate contexts for bundles and context files at the
latest clean tag (if there is one) is unsafe. I will add a comment. I
also wonder if we can make it safer somehow...

> 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.

See my reply to your patch.

> I haven't 
> screened patch1921 yet but will do so soon unless it turns out I've 
> misunderstood something fundamental about what to test.

The test is fine in principle btu I have a few nitpicks, again see my
reply to the patch for details.

>>  * 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.)

I always work with the latest from screened in order to find such issues
early. And I tried to apply a patch that I sent (I sign all my patches
by default) and got a hash sum error...

> 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.)

Probably by accident.

>>  * 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.

That sounds plausible. Not a good API in any case and not well
documented, so good riddance.

>>  * 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.

Yeah, I more or less use it as a dump for anything that we want to share
between command implementations and that doesn't seem to fit anywhere else.

In this case I am pretty sure this belongs to the UI layer. This has
nothing at all to do with the /internals/ of a bundle, which is what
Darcs.Patch.Bundle is about.

>>  * 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.

Well, it may be questionable whether /clear/-sign is a good idea, but I
personally do care: signing patch bundles is currently the only way to
do some sort of automatic validation based on known identity of the
sender. (BTW, I think our infrastructure is supposed to apply signed
patches of known developers to screened, but at least for me that never
worked, otherwise I wouldn't sign them by default.) The recent problems
with applying patch bundles clearly show that bundles aren't safe. If
darcs were just a little bit more popular I am sure some idiot would
find it funny to send random broken patches that cause darcs to hang
forever trying to apply them or cause other sorts of harm.

> Yeah, I more or less use it as a dump for anything that we want to share
> between command implementations and that doesn't seem to fit anywhere else.
> 
> In this case I am pretty sure this belongs to the UI layer. This has
> nothing at all to do with the /internals/ of a bundle, which is what
> Darcs.Patch.Bundle is about.

Maybe filenames belong in the Repository layer, which already does
things on disk. Or maybe not. Not worth more effort now, anyway.

>> Yeah, I more or less use it as a dump for anything that we want to share
>> between command implementations and that doesn't seem to fit anywhere else.
>>
>> In this case I am pretty sure this belongs to the UI layer. This has
>> nothing at all to do with the /internals/ of a bundle, which is what
>> Darcs.Patch.Bundle is about.
> 
> Maybe filenames belong in the Repository layer, which already does
> things on disk. Or maybe not. Not worth more effort now, anyway.

(Agreed. Still these file names are /not/ about the Repository layer
which is concerned with the /internal/ structure of the repo on disk and
not about files that darcs creates for the user do do with as they
please. In principle we could as well print to stdout and let the user
redirect it to a file, like 'darcs log --context does', btw.)