Patchwork hg: copy buffer state to remote ui

login
register
mail settings
Submitter Elmar Bartel
Date June 10, 2020, 10:14 p.m.
Message ID <563268fce8e7502e1781.1591827287@nuc15.leo.org>
Download mbox | patch
Permalink /patch/46494/
State New
Headers show

Comments

Elmar Bartel - June 10, 2020, 10:14 p.m.
# HG changeset patch
# User Elmar Bartel <elb_hg@leo.org>
# Date 1591826940 -7200
#      Thu Jun 11 00:09:00 2020 +0200
# Node ID 563268fce8e7502e17812fea7b90325a37c854f4
# Parent  83e41b73d115e3717943c2e5a83d36d05670384c
hg: copy buffer state to remote ui

When remoteui() creates a new ui, the buffer state of the
source ui should also be copied to the newly created ui.
Otherwise, when pushbuffer() was called on the source ui,
this "push" gets lost for the remote ui and output from the
remote side will go to default stdout but not to the pushed
buffer as is expected by the caller of pushbuffer().
Yuya Nishihara - June 11, 2020, 11:06 a.m.
On Thu, 11 Jun 2020 00:14:47 +0200, Elmar Bartel wrote:
> # HG changeset patch
> # User Elmar Bartel <elb_hg@leo.org>
> # Date 1591826940 -7200
> #      Thu Jun 11 00:09:00 2020 +0200
> # Node ID 563268fce8e7502e17812fea7b90325a37c854f4
> # Parent  83e41b73d115e3717943c2e5a83d36d05670384c
> hg: copy buffer state to remote ui
> 
> When remoteui() creates a new ui, the buffer state of the
> source ui should also be copied to the newly created ui.
> Otherwise, when pushbuffer() was called on the source ui,
> this "push" gets lost for the remote ui and output from the
> remote side will go to default stdout but not to the pushed
> buffer as is expected by the caller of pushbuffer().
> 
> diff -r 83e41b73d115 -r 563268fce8e7 mercurial/hg.py
> --- a/mercurial/hg.py	Tue Jun 09 17:13:26 2020 -0400
> +++ b/mercurial/hg.py	Thu Jun 11 00:09:00 2020 +0200
> @@ -1362,6 +1362,12 @@ def remoteui(src, opts):
>      if v:
>          dst.setconfig(b'web', b'cacerts', util.expandpath(v), b'copied')
>  
> +    # copy buffer state to the remote ui
> +    if src._buffers:
> +        dst._buffers = src._buffers
> +        dst._bufferstates = src._bufferstates
> +        dst._bufferapplylabels = src._bufferstates[-1][2]

The state would become inconsistent if you do popbuffer() later on. I don't
know why ui.copy() doesn't preserve the buffer states, but maybe that's why.

And this touches the internal of the ui, we'll need some tests so the change
wouldn't regress. It might be even better to add a function to copy ui with
buffer e.g. ui.copy_sharing_buffer() or ui.copy(share_buffer=True).
Elmar Bartel - June 11, 2020, 4:40 p.m.
Hello Yuya,

> > +    # copy buffer state to the remote ui
> > +    if src._buffers:
> > +        dst._buffers = src._buffers
> > +        dst._bufferstates = src._bufferstates
> > +        dst._bufferapplylabels = src._bufferstates[-1][2]
> 
> The state would become inconsistent if you do popbuffer() later on.
Which state: the state of the original ui or the state of
the remote-ui?
Anyway, as long as the ui-buffer interface is correctly used 
(properly matching pushpuffer() and popbuffer()) I cannot
see any state becoming inconsistent.
Furthermore the remote-ui is not accessible from the user
calling pushbuffer() on the original ui. This fact brought
me to the solution which my patch implements.

> [...]
> And this touches the internal of the ui, we'll need some tests so
> the change wouldn't regress.
Is see two things here, which could be tested:
  - the original ui is not changed, beside the desired effect of
    having additional content in the buffer.
  - the buffer does contain the content from the actions on the
    remoute-ui
Is this the test you suggest?

> It might be even better to add a function to copy ui with
> buffer e.g. ui.copy_sharing_buffer() or ui.copy(share_buffer=True).
I completly agree here.
This is the place where copying things should happen.
And then, remoteui() would just call ui.copy(share_buffer=True).

Nevertheless: the problems you anticipate (and I do not yet see)
are then moved to the ui.copy() function. Since what can really
be done by ui.copy(share_buffer==True)?
I see two posibilities:

 1) ui.copy(share_buffer==True) merely does the same, as my patch.
 2) or, deep-copy the buffer structure to the destination ui.
    
With 1) we'd run into the problems you anticipate.
With 2) the problem arises when/where to copy back the buffered
content to the buffers of the original ui. 

Sorry for the many questions.

Yours,
Elmar.
Yuya Nishihara - June 12, 2020, 1:05 p.m.
On Thu, 11 Jun 2020 18:40:01 +0200, Elmar Bartel wrote:
> > > +    # copy buffer state to the remote ui
> > > +    if src._buffers:
> > > +        dst._buffers = src._buffers
> > > +        dst._bufferstates = src._bufferstates
> > > +        dst._bufferapplylabels = src._bufferstates[-1][2]
> > 
> > The state would become inconsistent if you do popbuffer() later on.
> Which state: the state of the original ui or the state of
> the remote-ui?

ui._bufferapplylabels of the ui which buffer has been popped by the other.

Perhaps, the simplest workaround is to insert a sentinel entry to
ui._bufferstates by default, and remove ui._bufferapplylabels.

> Anyway, as long as the ui-buffer interface is correctly used
> (properly matching pushpuffer() and popbuffer()) I cannot
> see any state becoming inconsistent.
> Furthermore the remote-ui is not accessible from the user
> calling pushbuffer() on the original ui. This fact brought
> me to the solution which my patch implements.

I think remote-ui instance can be obtained from peer.ui.

> > And this touches the internal of the ui, we'll need some tests so
> > the change wouldn't regress.
> Is see two things here, which could be tested:
>   - the original ui is not changed, beside the desired effect of
>     having additional content in the buffer.
>   - the buffer does contain the content from the actions on the
>     remoute-ui
> Is this the test you suggest?

Something like that. Maybe you can add the scenario which you're
trying to fix.

> > It might be even better to add a function to copy ui with
> > buffer e.g. ui.copy_sharing_buffer() or ui.copy(share_buffer=True).
> I completly agree here.
> This is the place where copying things should happen.
> And then, remoteui() would just call ui.copy(share_buffer=True).
> 
> Nevertheless: the problems you anticipate (and I do not yet see)
> are then moved to the ui.copy() function. Since what can really
> be done by ui.copy(share_buffer==True)?
> I see two posibilities:
> 
>  1) ui.copy(share_buffer==True) merely does the same, as my patch.
>  2) or, deep-copy the buffer structure to the destination ui.
>     
> With 1) we'd run into the problems you anticipate.
> With 2) the problem arises when/where to copy back the buffered
> content to the buffers of the original ui.

Yep, (2) wouldn't be useful. (1) isn't always correct since there's no
parent-child relation between two ui instances, but I think it makes
more sense.

  ui0.pushbuffer()
  ui1 = ui0.copy(share_buffer=True)
  ...
  ui0.popbuffer()
  # ui1 may outlive and its buffer would be popped implicitly, which might
  # not always be what we want, but that's unavoidable.

So let's implement (1) and document the caveats.
Elmar Bartel - June 12, 2020, 6:53 p.m.
Hallo Yuya,

> ui._bufferapplylabels of the ui which buffer has been popped by the other.
Ah, thanks! It is pretty clear now:

The use of ui._bufferapplylabels as "cache" for ui._bufferstates[-1][2]
shows a fundamental problem (with shared bufferrs). The condition

	ui._bufferstates[-1][2] == ui._bufferapplylabels

is not neccessarily True for all ui created with
ui.copy(share_buffer==True). As soon as pushbuffer() or popbuffer() is
called on any of these ui, the ui._bufferapplylabels value get updated
only on the ui where the *buffer() method was called on.
All other ui will see the change in the (shared)
ui._bufferstates[-1][2], but not on their ui._bufferapplylabels.

> Perhaps, the simplest workaround is to insert a sentinel entry to
> ui._bufferstates by default, and remove ui._bufferapplylabels.
Yep. Removing ui._bufferapplylabels and checking ui._bufferstates[-1][2]
directly will resolve the consistency problem.
The performance penalty is minimal: an addtional check if there is
a buffer and if so an index operation.
This effort is not prohibitive.

> I think remote-ui instance can be obtained from peer.ui.
True!

> > Is see two things here, which could be tested:
> >   - the original ui is not changed, beside the desired effect of
> >     having additional content in the buffer.
> >   - the buffer does contain the content from the actions on the
> >     remoute-ui
> > Is this the test you suggest?
> 
> Something like that. Maybe you can add the scenario which you're
> trying to fix.
Ok, I'll try to setup something.

> [...]
>   ui0.pushbuffer()
>   ui1 = ui0.copy(share_buffer=True)
>   ...
>   ui0.popbuffer()
>   # ui1 may outlive and its buffer would be popped implicitly, which might
>   # not always be what we want, but that's unavoidable.
What could be done about this: 
We record in ui._bufferstates[] (as fourth element) the ui which did
the push and allow/do the pop only when it is done by the same ui.
But this also would not fix all possible calling-sequence-scenrios
and then its debatable whether this effort is worth it.

> So let's implement (1) and document the caveats.
I'll do that and come back then.

Thank's for your feedback!
Yours,
Elmar.
Yuya Nishihara - June 13, 2020, 3:46 a.m.
On Fri, 12 Jun 2020 20:53:20 +0200, Elmar Bartel wrote:
> > [...]
> >   ui0.pushbuffer()
> >   ui1 = ui0.copy(share_buffer=True)
> >   ...
> >   ui0.popbuffer()
> >   # ui1 may outlive and its buffer would be popped implicitly, which might
> >   # not always be what we want, but that's unavoidable.
> What could be done about this: 
> We record in ui._bufferstates[] (as fourth element) the ui which did
> the push and allow/do the pop only when it is done by the same ui.
> But this also would not fix all possible calling-sequence-scenrios
> and then its debatable whether this effort is worth it.

Another option is to create a shallow-copy of the buffers stack, so the
underlying buffer is shared, but the buffers stack itself isn't.

  newui._buffers = oldui._buffers[:]
  ...

Both oldui and newui can/should popbuffer() and will get the data between
oldui.pushbuffer() and self.popbuffer(). newui will remain buffered even
after oldui do popbuffer().

This behavior might be expected if oldui/newui aren't parent/child and should
work as if the underlying fout were replaced with the buffer. But that
wouldn't be always true. So I don't thinks there's the single best way to
implement buffer sharing.

Patch

diff -r 83e41b73d115 -r 563268fce8e7 mercurial/hg.py
--- a/mercurial/hg.py	Tue Jun 09 17:13:26 2020 -0400
+++ b/mercurial/hg.py	Thu Jun 11 00:09:00 2020 +0200
@@ -1362,6 +1362,12 @@  def remoteui(src, opts):
     if v:
         dst.setconfig(b'web', b'cacerts', util.expandpath(v), b'copied')
 
+    # copy buffer state to the remote ui
+    if src._buffers:
+        dst._buffers = src._buffers
+        dst._bufferstates = src._bufferstates
+        dst._bufferapplylabels = src._bufferstates[-1][2]
+
     return dst