Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

text-io: fix block shrinking when the recent piece is on the block's end #1126

Closed
wants to merge 1 commit into from
Closed

text-io: fix block shrinking when the recent piece is on the block's end #1126

wants to merge 1 commit into from

Conversation

vkochan
Copy link
Contributor

@vkochan vkochan commented Sep 5, 2023

Looks like the original idea was to shrink the block if the text have been deleted on the recent piece which is located at the block's end, so in that case it is enough just to resize the block without memmove.

Looks like the original idea was to shrink the block if the text have been
deleted on the recent piece which is located at the block's end, so in
that case it is enough just to resize the block without memmove.

Signed-off-by: Vadym Kochan <[email protected]>
@rnpnr
Copy link
Collaborator

rnpnr commented Sep 22, 2023

So I've looked at this a few times and I still can't quite follow the
logic. Can you explain with a little bit more detail so that my slow
brain can understand it?

@vkochan
Copy link
Contributor Author

vkochan commented Sep 22, 2023

In my understanding the original idea was "when we are deleting a text from the recent (cached) piece which
sits on the edge of the block - then we can just shrink the block size without do a memmove". For example we have a
text formed from the following block and pieces:

| [piece1][piece2][piece3 ]| -- this is a block with pieces which holds the inserted chunks
| [Some ][text in][ block] | -- these are the chunks per each piece

so, for example we are deleting the "block" from the piece3 which are at the end of the block, then
we can just re-size piece3 and block sizes.

So, I think that the original logic does not work if for example the "pos" is at the start of the
"block" word and the "len" is length till the end of the block. The real use case might be deleting a text
from the end.

@rnpnr
Copy link
Collaborator

rnpnr commented Oct 4, 2023

Ok so I'm still not convinced. I've used this for a while and haven't
noticed any issues but that could just be because this is only in
the cache path.

The main thing confusing me is that the blk->len parameter is
used to mean two different things. I can assume this fix is correct
because the check above verifies that pos + len does not lie after
the end of the block. The next logical step would be to check if
blk->len == pos +len. But on the other hand blk->len apparently
also means the insertion position. If you follow the history all
the way back this check was added when the blk->len member was
called blk->pos. Its possible that this check was wrong then as
well but that was 7+ years ago.

A minor nit: pos + len is the value stored in the end variable.
That variable should be used in both places where pos + len is
being used.

@vkochan vkochan closed this Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants