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

Relax checks for extended capability to support new format #5

Merged
merged 1 commit into from
Aug 8, 2019

Conversation

blueyed
Copy link

@blueyed blueyed commented Aug 3, 2019

In the 20180331 release, the format was slightly changed:

20180331
+ improve terminfo write/read by modifying the fourth item of the
extended header to denote the number of valid strings in the extended
string table (prompted by a comment in unibilium's sources).

Since the number of valid string capabilities is not necessarily the
same as extstrslen, it's not possible to sanity check the total number
of items up front anymore.

Include a new test with an updated screen terminfo dump.

@blueyed
Copy link
Author

blueyed commented Aug 3, 2019

Hmm, make test still passes with the new test, but the patch to the source removed?!

diff --git i/unibilium.c w/unibilium.c
index 8586b6f..64472de 100644
--- i/unibilium.c
+++ w/unibilium.c
@@ -339,6 +339,8 @@ unibi_term *unibi_from_mem(const char *p, size_t n) {
             extalllen += extnumlen;
             extalllen += extstrslen;

+            DEL_FAIL_IF(extofflen != extalllen + extstrslen, EINVAL, t);
+
             DEL_FAIL_IF(
                 n <
                 extboollen +

@blueyed
Copy link
Author

blueyed commented Aug 4, 2019

To be clear: I'd expected the test to fail then.
Since it is not clear to me what the patch is about, at least the test should test for it I think.

@justinmk
Copy link
Member

justinmk commented Aug 4, 2019

We need this stuff in the 0.4 release, so would be better to merge now and ask questions later ..

ping @jamessan anyway :)

@blueyed
Copy link
Author

blueyed commented Aug 4, 2019

Yeah :) - but it's not like we need to hurry it "just because", without waiting a few days at least.

@jamessan
Copy link
Member

jamessan commented Aug 5, 2019

Hmm, make test still passes with the new test, but the patch to the source removed?!

Hmm, guess I didn't choose a good terminfo entry to use for the test.

@blueyed
Copy link
Author

blueyed commented Aug 7, 2019

@jamessan
So where was the problem seen with then?
Would it happen with newer files from ncurses (development, after 6.1)?
Can we use an existing terminfo entry (from ncurses development) for the test?

@blueyed
Copy link
Author

blueyed commented Aug 7, 2019

Ok. I've used the screen terminfo from mauke#40, and when generating the test for it it fails without the patch to unibilium.c here:

% TERMINFO_DIRS=$PWD tools/gen-static-test screen > t/static_screen.c
unibi_from_term("screen"): No such file or directory

With this patch it works, and the output is the same.

So the problem with the test not failing is that the patch is needed in the first place to generate the test, which is then using unibi_from_mem directly - but with the result that unibi_dump produced.

Therefore I think the patch is good, but the test adds no benefit (at least with regard to the patch).

blueyed added a commit to blueyed/unibilium that referenced this pull request Aug 7, 2019
@blueyed
Copy link
Author

blueyed commented Aug 7, 2019

Should be good now.
Without the patch the tests will fail now:

t/new-extended-format-static_screen.t .. 1/541 Bailout called. Further testing stopped: Invalid argument

This ships t/fixtures/s/screen now (from mauke#40).
I guess this might be just an updated version, and we would see the issue with ncurses 6.2 anyway already then - but that's not clear to me.

In the 20180331 release, the format was slightly changed:

> 20180331
>         + improve terminfo write/read by modifying the fourth item of the
>           extended header to denote the number of valid strings in the extended
>           string table (prompted by a comment in unibilium's sources).

Since the number of valid string capabilities is not necessarily the
same as extstrslen, it's not possible to sanity check the total number
of items up front anymore.

Include a new test with an updated screen terminfo dump.
@blueyed blueyed merged commit 7fba99c into neovim:master Aug 8, 2019
@blueyed blueyed deleted the nvim-patch branch August 8, 2019 14:10
blueyed added a commit to blueyed/neovim that referenced this pull request Aug 8, 2019
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.

3 participants