darcs

Patch 1931 remove isInverted from PatchInfo

Title remove isInverted from PatchInfo
Superseder Nosy List ganesh
Related Issues
Status needs-review Assigned To
Milestone

Created on 2019-09-21.11:13:43 by ganesh, last changed 2019-09-30.19:23:29 by ganesh.

Files
File name Status Uploaded Type Edit Remove
patch-preview.txt ganesh, 2019-09-21.11:13:43 text/x-darcs-patch
patch-preview.txt ganesh, 2019-09-22.08:45:13 text/x-darcs-patch
patch-preview.txt ganesh, 2019-09-27.10:16:09 text/x-darcs-patch
patch-preview.txt ganesh, 2019-09-30.19:23:29 text/x-darcs-patch
remove-isinverted-from-patchinfo.dpatch dead ganesh, 2019-09-22.08:45:13 application/x-darcs-patch
treat-the-patchinfo-isinverted-flag-as-legacy.dpatch dead ganesh, 2019-09-27.10:16:09 application/x-darcs-patch
treat-the-patchinfo-isinverted-flag-as-legacy.dpatch ganesh, 2019-09-30.19:23:29 application/x-darcs-patch
unnamed ganesh, 2019-09-21.11:13:43 text/plain
unnamed ganesh, 2019-09-22.08:45:13 text/plain
unnamed ganesh, 2019-09-27.10:16:09 text/plain
unnamed ganesh, 2019-09-30.19:23:29 text/plain
wip_-remove-isinverted-from-patchinfo.dpatch dead ganesh, 2019-09-21.11:13:43 application/x-darcs-patch
See mailing list archives for discussion on individual patches.
Messages
msg21516 (view) Author: ganesh Date: 2019-09-21.11:13:43
For discussion initially (and I still need to run the tests)

Now that we can't invert a Named, do we still need to
be able to express inverted PatchInfos? This patch
builds which suggests it may not be necessary.

I am a bit nervous that we might have them written out
in a repository out there somehow, though I can't think
of any actual scenarios yet.

1 patch for repository darcs-unstable@darcs.net:screened:

patch 996c6061ed4e22821689fba01e4f6fc71f6f90f4
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sat Sep 21 12:16:13 BST 2019
  * WIP: remove isInverted from PatchInfo
  
  Now that Named patches can't be inverted, it doesn't make
  sense to invert PatchInfos either.
Attachments
msg21518 (view) Author: bf Date: 2019-09-21.14:22:44
> I am a bit nervous that we might have them written out
> in a repository out there somehow, though I can't think
> of any actual scenarios yet.

I always assumed there was a tacit understanding that named patches in a
repo are never stored inverted. So i am not too worried here.

One way to view this is that for named patches (including named prims)
adding the inverse of a patch to a sequence is the same as removing that
patch i.e. a patch and its inverse cancel each other. It would be
possible, in principle, to codify this with a custom Eq2 instance for
sequences of patches with a SignedIdent instance (i.e. including
Invertible) that removes inverse pairs, but to do that cleanly requires
DerivingVia, so this is currently out of reach.

> patch 996c6061ed4e22821689fba01e4f6fc71f6f90f4
> Author: Ganesh Sittampalam <ganesh@earth.li>
> Date:   Sat Sep 21 12:16:13 BST 2019
>   * WIP: remove isInverted from PatchInfo
>   
>   Now that Named patches can't be inverted, it doesn't make
>   sense to invert PatchInfos either.

I had deliberately postponed the decision and I am glad you took the
initiative here. Your solutions all look fine to me. In particular, you
took care that makePatchname gives the same result as before which I
think is pretty important. We should fix its haddocks, though, and
inline the 'b2ps False' call, add also add a comment to explain why we
add 'BC.pack "f"'. Otherwise ready for screened.
msg21521 (view) Author: ganesh Date: 2019-09-21.21:34:25
> I always assumed there was a tacit understanding that named patches in a
> repo are never stored inverted. So i am not too worried here.

I was partly wondering if there might be cases where we store a named
patch outside the main sequence of patches in the repo.

But also now I think about it, I have a feeling that rollback used to
work differently and might have done this. I will dig in the history.

One other question about backwards compatibility: currently the XML
output (Darcs.Patch.Info.toXml') includes a line about 'inverted'.
I initially took the conservative approach and preserved it, but
thinking about it I am inclined to remove it.

> One way to view this is that for named patches (including named prims)
> adding the inverse of a patch to a sequence is the same as removing that
> patch i.e. a patch and its inverse cancel each other. It would be
> possible, in principle, to codify this with a custom Eq2 instance for
> sequences of patches with a SignedIdent instance (i.e. including
> Invertible) that removes inverse pairs, but to do that cleanly requires
> DerivingVia, so this is currently out of reach.

A sequence containing extra patches+inverses doesn't have the same
commute behaviour as one where they have been cancelled, so this doesn't
feel right to me.
msg21524 (view) Author: bf Date: 2019-09-22.07:37:27
> But also now I think about it, I have a feeling that rollback used to
> work differently and might have done this. I will dig in the history.

Oh. Yes, I also dimly remember something like that. That was very long
ago, though. Digging it up may be a good idea.

> One other question about backwards compatibility: currently the XML
> output (Darcs.Patch.Info.toXml') includes a line about 'inverted'.
> I initially took the conservative approach and preserved it, but
> thinking about it I am inclined to remove it.

I have noticed your (conservative) choice and I liked it. It doesn't
cost us anything (except a small comment in the code) and it keeps full
compatibility. Why do you think we should remove it?

>> One way to view this is that for named patches (including named prims)
>> adding the inverse of a patch to a sequence is the same as removing that
>> patch i.e. a patch and its inverse cancel each other. It would be
>> possible, in principle, to codify this with a custom Eq2 instance for
>> sequences of patches with a SignedIdent instance (i.e. including
>> Invertible) that removes inverse pairs, but to do that cleanly requires
>> DerivingVia, so this is currently out of reach.
> 
> A sequence containing extra patches+inverses doesn't have the same
> commute behaviour as one where they have been cancelled, so this doesn't
> feel right to me.

This is something I will have to think about some more before I can
usefully comment. But it is a digression anyway, so probably not on this
ticket.
msg21525 (view) Author: ganesh Date: 2019-09-22.08:13:18
So rollback did indeed create inverted patches in the past and there are
some in the history of darcs itself - just search for "UNDO" in the
darcs changes output to find them.

In fact in this patch I introduced the logic to support commuting a
RebaseName with an inverted Named:

patch 465ce49a25f8b19f6cecef87217b48c40668e2dd
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Aug 27 19:12:02 GMT Summer Time 2012
  * don't insist on all patches in a Named having the same polarity
  Historic rollback patches wrote out inverted names, although
  modern darcs doesn't do this (so perhaps we could insist on this for
  v2 patches?)

The current behaviour of rollback was introduced in 2008:

patch 03b02776a5be47803d1f5a4cff944c295300a7b9
Author: David Roundy <droundy@darcs.net>
Date:   Wed Jan 16 20:36:44 GMT Standard Time 2008
  * reimplement rollback.

Maybe we can continue to have the flag, perhaps renamed to
legacyInverted, so we can preserve hashes and any other global
invariants we need to, but not treat it as really inverting the
PatchInfo for internal purposes? I guess that even v2 patches do need to
support it as they must have been converted as-is.

Regardless, I will add a test to make sure we catch any future attempts
to totally remove this functionality. I should have done that in 2012.

> I have noticed your (conservative) choice and I liked it. It doesn't
> cost us anything (except a small comment in the code) and it keeps
> full compatibility. Why do you think we should remove it?

My main thought was that people consuming the XML then need to consider
its existence and decide what to do with it.
msg21526 (view) Author: ganesh Date: 2019-09-22.08:45:13
latest version of isInverted removal, written before discovering
that we do need to support legacy inverted PatchInfos

This version also removes the inverted field from the XML output.

1 patch for repository darcs-unstable@darcs.net:screened:

patch 29f79c9e3a85be4e1379d13f39dbaa0004385526
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sun Sep 22 08:34:03 BST 2019
  * remove isInverted from PatchInfo
  
  Now that Named patches can't be inverted, it doesn't make
  sense to invert PatchInfos either.
Attachments
msg21527 (view) Author: bf Date: 2019-09-22.09:06:41
> So rollback did indeed create inverted patches in the past and there are
> some in the history of darcs itself - just search for "UNDO" in the
> darcs changes output to find them.

I see. Not much we can do about that, then.

> Maybe we can continue to have the flag, perhaps renamed to
> legacyInverted, so we can preserve hashes and any other global
> invariants we need to, but not treat it as really inverting the
> PatchInfo for internal purposes?

I don't think this would add much clarity. We would have to duplicate
the flag in Darcs.Patch.Invertible. Better keep things as they are i.e.
PatchInfo remains invertible, plus a few comments to warn people off
trying to do something similar in the future.

> Regardless, I will add a test to make sure we catch any future attempts
> to totally remove this functionality. I should have done that in 2012.

A very good idea.

>> I have noticed your (conservative) choice and I liked it. It doesn't
>> cost us anything (except a small comment in the code) and it keeps
>> full compatibility. Why do you think we should remove it?
> 
> My main thought was that people consuming the XML then need to consider
> its existence and decide what to do with it.

I guess the point is moot now, but in general I would rather err on the
side of compatibility here. If the XML output has any use at all then it
is to be the one format that can be relied upon to remain stable.
msg21530 (view) Author: ganesh Date: 2019-09-22.10:46:26
>> Maybe we can continue to have the flag, perhaps renamed to
>> legacyInverted, so we can preserve hashes and any other global
>> invariants we need to, but not treat it as really inverting the
>> PatchInfo for internal purposes?
> 
> I don't think this would add much clarity. We would have to duplicate
> the flag in Darcs.Patch.Invertible. Better keep things as they are i.e.
> PatchInfo remains invertible, plus a few comments to warn people off
> trying to do something similar in the future.

I don't think this is just about code clarity, but also of semantics. At
the moment we see the isInverted flag as creating a relationship in
terms of things like SignedId.invertId. We don't use that anywhere for
PatchInfo but maybe we might in future. Also the error case in
Rebase.Name is wrong. So we either need to remove it or perhaps take the
pragmatic view that noone will ever actually suspend an old UNDO patch.
I think the original code I wrote (that did an inverse commute) was also
wrong.

Semantically, my feeling is that we should forget the relationship
between the UNDO patches that got written out and the original ones. I
don't think darcs would ever have done anything with that relationship
historically, and it would now be best to set that position in stone.
It's a different concept to the temporary inversion we do in memory now,
where we do want to preserve the relationship. So even though we would
indeed end up with a duplicated flag (and hence confusing possibilities
in memory like an InvertedId of an "inverted" PatchInfo), I think we
should accept that.

> I guess the point is moot now, but in general I would rather err on the
> side of compatibility here. If the XML output has any use at all then it
> is to be the one format that can be relied upon to remain stable.

I thought the main point was to save people having to parse our
human-readable output.
msg21541 (view) Author: bf Date: 2019-09-23.11:48:16
>>> Maybe we can continue to have the flag, perhaps renamed to
>>> legacyInverted, so we can preserve hashes and any other global
>>> invariants we need to, but not treat it as really inverting the
>>> PatchInfo for internal purposes?
>>
>> I don't think this would add much clarity. We would have to duplicate
>> the flag in Darcs.Patch.Invertible. Better keep things as they are i.e.
>> PatchInfo remains invertible, plus a few comments to warn people off
>> trying to do something similar in the future.
> 
> I don't think this is just about code clarity, but also of semantics.

I didn't mean to say it was. I meant it as a tongue in cheek
circumscription of "please don't do that, it would be quite obscure".

> Semantically, my feeling is that we should forget the relationship
> between the UNDO patches that got written out and the original ones. I
> don't think darcs would ever have done anything with that relationship
> historically, and it would now be best to set that position in stone.

I would completely agree with that position if we were to abandon
compatibility and throw out PatchInfo inversion. Since we're not, I
can't quite see what good it does.

> It's a different concept to the temporary inversion we do in memory now,
> where we do want to preserve the relationship. So even though we would
> indeed end up with a duplicated flag (and hence confusing possibilities
> in memory like an InvertedId of an "inverted" PatchInfo), I think we
> should accept that.

I am not sure I can accept that. My view is that it doesn't hurt us in
any way to have the ability to invert PatchInfos. It wouldn't even hurt
if we were to write a negative Invertible to disk. However, I am willing
to rollback

patch 4b5bba9bd00b7f6887ca4f9e981a837d6e16c101
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 18:40:36 CEST 2019
  * Invertible: allow showPatch etc of Rev patches

to make sure we cannot do that, if that's what it takes.

I would also be happy to make the change you propose when we finally get
rid of Invertible, which is, to remind us, only a stop-gap measure.

>> I guess the point is moot now, but in general I would rather err on the
>> side of compatibility here. If the XML output has any use at all then it
>> is to be the one format that can be relied upon to remain stable.
> 
> I thought the main point was to save people having to parse our
> human-readable output.

You may be in a better position to judge that, historically seen. From
my perspective it is there to relieve us from having to consider
compatibility for every minor change we make to the output of darcs
commands.

Working around specific peculiarties in the output format of an external
tool such as darcs is not /that/ hard. I've done it a few times, usually
doesn't take more than few additional lines in your hacky perl script or
whatever. But if the format is a moving target i.e. if your parser
breaks with every new release, then it will soon become an
unmaintainable mess. People (who have written such a parser, however
hacky) will complain about such changes, or even worse they silently
abandon their efforts and no longer support darcs. This is not
hypothetical, it happened with redmine (which unfortunately didn't use
--xml). This is why I think stability is a strict requirement for
options like --xml and --machine-readable.
msg21570 (view) Author: ganesh Date: 2019-09-24.16:04:29
If I understand correctly your view is that we shouldn't have two kinds
of PatchInfo inversion simultaneously in the codebase, but that it'd be
ok to move the isInverted flag to "legacy" status if and when we get rid
of Invertible. I still feel they are different and that it could be
explained clearly enough in comments, but ok.

>> Semantically, my feeling is that we should forget the relationship
>> between the UNDO patches that got written out and the original ones. I
>> don't think darcs would ever have done anything with that relationship
>> historically, and it would now be best to set that position in stone.
> 
> I would completely agree with that position if we were to abandon
> compatibility and throw out PatchInfo inversion. Since we're not, I
> can't quite see what good it does.

We clearly have to maintain compatibility in the sense of being able to
read "UNDO" patches and do normal darcs operations on them. What
concerns me is how exactly they should be treated semantically within darcs.

For example, we don't currently have an IdEq2 instance for Invertible,
but if we did and it used the default implementation, we'd have that the
inverse of the original patch was equal to the rollback patch. That
seems dangerous to me.

On a more practical level, there are a couple of places within darcs
where we do look at isInverted. Firstly in 'updatePatchHeader' used by
amend-record and rebase, where the flag is preserved if the patch being
amended has it set. Secondly in commuteNameNamed/commuteNamedName in
D.P.Rebase.Name, where it's now an error.

I think the right fixes for those places is just to stop treating it
specially: if we amend an isInverted patch then the amended version is
not isInverted, and commuting a RebaseName past a Named behaves just the
same for isInverted as the normal case.

 > I am not sure I can accept that. My view is that it doesn't hurt us in
> any way to have the ability to invert PatchInfos. It wouldn't even hurt
> if we were to write a negative Invertible to disk. However, I am willing
> to rollback
> 
> patch 4b5bba9bd00b7f6887ca4f9e981a837d6e16c101
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Thu Sep 19 18:40:36 CEST 2019
>   * Invertible: allow showPatch etc of Rev patches
> 
> to make sure we cannot do that, if that's what it takes.

That sounds like a good idea.

> I would also be happy to make the change you propose when we finally get
> rid of Invertible, which is, to remind us, only a stop-gap measure.

> [...] I think stability is a strict requirement for
> options like --xml and --machine-readable.

OK. I guess the principled approach would be to adopt a versioning
scheme for our output so we can eventually retire old things, or at
least allow new tools to use our best current format, but that's way
more effort than is justified.
msg21571 (view) Author: bf Date: 2019-09-24.17:04:09
> For example, we don't currently have an IdEq2 instance for Invertible,
> but if we did and it used the default implementation, we'd have that the
> inverse of the original patch was equal to the rollback patch. That
> seems dangerous to me.

This is indeed a serious problem, regardless of a potential IdEq2
instance: there must be no inverse pairs in the same sequence, otherwise
either commute or equality tests can crash darcs, due to the error cases
in the Invertible instances. This means we /must not/ internally treat
existing legacy inverted patches as inverses of the ones they roll back.
Legacy rollback patches must internally be treated as positive patches.

Your proposal is precisely the correct way to fix that and my previous
objections are irrelevant.

> On a more practical level, there are a couple of places within darcs
> where we do look at isInverted. Firstly in 'updatePatchHeader' used by
> amend-record and rebase, where the flag is preserved if the patch being
> amended has it set. Secondly in commuteNameNamed/commuteNamedName in
> D.P.Rebase.Name, where it's now an error.
> 
> I think the right fixes for those places is just to stop treating it
> specially: if we amend an isInverted patch then the amended version is
> not isInverted

Yes, definitely.

> and commuting a RebaseName past a Named behaves just the
> same for isInverted as the normal case.

I think this is correct, yes.

>> I am willing to rollback
>>
>> patch 4b5bba9bd00b7f6887ca4f9e981a837d6e16c101
>> Author: Ben Franksen <ben.franksen@online.de>
>> Date:   Thu Sep 19 18:40:36 CEST 2019
>>   * Invertible: allow showPatch etc of Rev patches
>>
>> to make sure we cannot do that, if that's what it takes.
> 
> That sounds like a good idea.

Perhaps it would be enough to call error if the showPatchFor argument is
ForStorage?
msg21572 (view) Author: ganesh Date: 2019-09-24.17:14:18
> This is indeed a serious problem, regardless of a potential IdEq2
> instance: there must be no inverse pairs in the same sequence, otherwise
> either commute or equality tests can crash darcs, due to the error cases
> in the Invertible instances. This means we /must not/ internally treat
> existing legacy inverted patches as inverses of the ones they roll back.

Right - though in the code as it stands, we wouldn't ever get a crash (I
don't think), as it wouldn't turn a Fwd patch into a Rev just because
isInverted was True. So my concern is about possible future evolutions
of our codebase, rather than one where I can demonstrate an actual
problem right now.

Anyway, I'll send an updated version of my patch for a final look before
screening it.

> Perhaps it would be enough to call error if the showPatchFor argument is
> ForStorage?

Yes, I think that's fine. Some day I'd like to disentangle the different
ways showPatch* are used...
msg21630 (view) Author: ganesh Date: 2019-09-27.10:16:09
This version renames the isInverted flag to _piLegacyIsInverted
and removes all the code that treated it specially.

I've also added a test. I didn't try to test commuteNameNamed
as it'd be a bit of a pain to setup a test case for that.

I won't screen immediately.

1 patch for repository darcs-unstable@darcs.net:screened:

patch acd0f71ccea2796bb166c7da9594729854de0071
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Sep 27 10:29:36 BST 2019
  * treat the PatchInfo isInverted flag as legacy
  
  Now that Named patches can't be inverted, it doesn't make
  sense to invert PatchInfos either.
  
  However patches with this flag set were historically written
  out by 'darcs rollback' until around 2008, so we need some
  residual support.
  
  The approach is to treat it like the other flags in
  PatchInfo and for it not to be anything to do with actual
  patch inversion any more.
Attachments
msg21642 (view) Author: bf Date: 2019-09-27.18:38:37
hunk ./src/Darcs/UI/PatchHeader.hs 246
-        let maybe_invert = if isInverted old_pinf then invertName else id
-        new_pinf <- maybe_invert `fmap` patchinfo date new_name
new_author new_log
+        new_pinf <- patchinfo date new_name new_author new_log
I had to think a moment about this one. So before, when we amended a
patch we kept it legacy-inverted. Now we keep only the name and log and
make a new legacy-positive patch. Makes sense.

I would have made the long comment about _piLegacyIsInverted (which I
appreciate, thanks for the write-up!) part of the general PatchInfo
haddocks (perhaps adding a "see above" for the member). I guess this
will become quite unreadable when rendered with haddock (but I may be
wrong).

I don't like removing read/write of legacy-inverted patches from the QC
tests. IMO rawPatchInfo should continue to allow to set
_piLegacyIsInverted (we expressly export it for testing purposes only)
and the changes to Darcs.Test.Patch.Info/Inventory reverted.

Otherwise fine.
msg21643 (view) Author: ganesh Date: 2019-09-27.18:50:53
> I would have made the long comment about _piLegacyIsInverted (which I
> appreciate, thanks for the write-up!) part of the general PatchInfo
> haddocks (perhaps adding a "see above" for the member). I guess this
> will become quite unreadable when rendered with haddock (but I may be
> wrong).

OK, I'll see how that goes. I don't write complicated haddock much
because I know I will never remember to test it, but I should.

> I don't like removing read/write of legacy-inverted patches from the QC
> tests. IMO rawPatchInfo should continue to allow to set
> _piLegacyIsInverted (we expressly export it for testing purposes only)
> and the changes to Darcs.Test.Patch.Info/Inventory reverted.

I thought about this a bit when I was writing it. I wanted to remove it
from rawPatchInfo to make it more likely that other users of that would
do the right thing. But I just noticed that the comment says it's
exported only for tests so it's not really a problem, and I can use
TestOnly now to make sure of that.

I did leave one generator that produces it
(arbitraryUnencodedPatchInfo), which I thought might be a reasonable
balance though I didn't check how/where it's used. In fact before my
change both generators hardcoded it to False, but our semantic view of
the flag is now a bit different so there may be a stronger case for
setting it occasionally.
msg21655 (view) Author: ganesh Date: 2019-09-30.19:23:29
Final version with haddock and with _piLegacyIsInverted
potentially set in all the Arbitrary instances. Will
screen now.

2 patches for repository darcs-unstable@darcs.net:screened:

patch 62cb707c782b8379e62588376f03ae9a5f57a58b
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Sep 30 19:30:37 BST 2019
  * treat the PatchInfo isInverted flag as legacy
  
  Now that Named patches can't be inverted, it doesn't make
  sense to invert PatchInfos either.
  
  However patches with this flag set were historically written
  out by 'darcs rollback' until around 2008, so we need some
  residual support.
  
  The approach is to treat it like the other flags in
  PatchInfo and for it not to be anything to do with actual
  patch inversion any more.

patch 17a7f7f38e5c83bd3def4a1e829f408bb5f6c49b
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Sep 30 19:30:40 BST 2019
  * make rawPatchInfo export from D.P.Info TestOnly
Attachments
History
Date User Action Args
2019-09-21 11:13:43ganeshcreate
2019-09-21 11:14:13ganeshsetstatus: needs-screening -> in-discussion
2019-09-21 14:22:45bfsetmessages: + msg21518
2019-09-21 21:34:25ganeshsetmessages: + msg21521
2019-09-22 07:37:27bfsetmessages: + msg21524
2019-09-22 08:13:18ganeshsetmessages: + msg21525
2019-09-22 08:45:13ganeshsetfiles: + patch-preview.txt, remove-isinverted-from-patchinfo.dpatch, unnamed
messages: + msg21526
2019-09-22 09:06:41bfsetmessages: + msg21527
2019-09-22 10:46:27ganeshsetmessages: + msg21530
2019-09-23 11:48:17bfsetmessages: + msg21541
2019-09-24 16:04:29ganeshsetmessages: + msg21570
2019-09-24 17:04:09bfsetmessages: + msg21571
2019-09-24 17:14:18ganeshsetmessages: + msg21572
2019-09-27 09:26:19bfsetstatus: in-discussion -> followup-in-progress
2019-09-27 10:12:39ganeshsettitle: WIP: remove isInverted from PatchInfo -> remove isInverted from PatchInfo
2019-09-27 10:12:44ganeshsetstatus: followup-in-progress -> needs-screening
2019-09-27 10:16:09ganeshsetfiles: + patch-preview.txt, treat-the-patchinfo-isinverted-flag-as-legacy.dpatch, unnamed
messages: + msg21630
2019-09-27 18:38:38bfsetmessages: + msg21642
2019-09-27 18:50:53ganeshsetmessages: + msg21643
2019-09-30 19:23:19ganeshsetstatus: needs-screening -> needs-review
2019-09-30 19:23:29ganeshsetfiles: + patch-preview.txt, treat-the-patchinfo-isinverted-flag-as-legacy.dpatch, unnamed
messages: + msg21655