[Toybox] [PATCH] Addition of the 'g' command to vi.c

[enh]

On Tue, Oct 10, 2023 at 1:10 AM Rob Landley <rob at landley.net> wrote:
>
> On 10/9/23 22:43, Oliver Webb via Toybox wrote:
> > Heya, I added the 'g' ex command to vi
>
> Cool. Good work, thanks.
>
> I applied all but the help text change, where the -s previously had a tab after it:
>
> 000001f0  64 20 70 72 65 73 73 20  45 4e 54 45 52 2e 0a 0a  |d press ENTER...|
> 00000200  20 20 20 20 2d 73 09 72  75 6e 20 53 43 52 49 50  |    -s.run SCRIP|
> 00000210  54 20 6f 66 20 63 6f 6d  6d 61 6e 64 73 20 6f 6e  |T of commands on|
> 00000220  20 46 49 4c 45 0a 0a 20  20 20 20 76 69 20 6d 6f  | FILE..    vi mo|
>
> Which your patch removed and replaced with two spaces. I typed up a longish
> policy explanation and then cut and pasted it to a separate message because it's
> just ridiculous. (And yet...)
>
> > The only problem with it that I see is (also a problem in cslpit with /regex/ rules)
> > that it doesn't read escaped regexp's because
> > sscanf(); doesn't work like that and I haven't found a simple solution to reading
> > escaped strings up until a delimiter (except when that delimiter is escaped)
>
> I'm pretty sure you have to parse the escape format going through from the start
> each time. I've dealt with this in a few places (fairly extensively in sh.c),
> and the problem is the delimiter could itself be escaped, so...
>
> > The only reason this command took so long to implement is because I had some errors
> > that turned out to be caused by uninitialized memory.
>
> In theory ASAN should be able to spot that one! In practice, if it's a local
> variable on the stack, not so much.

you can add `-ftrivial-auto-var-init=pattern` to your CFLAGS. (Android
globally always uses -ftrivial-auto-var-init=zero because,
surprisingly, it has better security properties than pattern. it's
also better for code size etc.)

> I think valgrind might be able to, I set
> that up recently but haven't run it on anything nontrivial yet, as I blogged
> about...
>
>   https://landley.net/notes-2023.html#19-09-2023
>
> It's on the todo heap...
>
> > I would write some test cases to go with this command, but I already have a pending patch
> > for other test cases that I have added and I didn't want to have a conflict
> > between the 2 patches
>
> Hmmm, looking at tests/vi.test...
>
> $ TEST_HOST=1 make test_vi
> scripts/test.sh vi
> SKIP: vi dd first line ascii
> SKIP: vi dd last line ascii
> SKIP: vi dw last line ascii
> SKIP: vi dw first line ascii
> SKIP: vi D first line ascii
> SKIP: vi D last line ascii
> SKIP: vi yw push ascii
> SKIP: vi insert start of file ascii
> SKIP: vi insert end of file ascii
> SKIP: vi insert after first word ascii
> SKIP: vi insert multiple times ascii
> SKIP: vi insert multi yank move and push ascii
>
> Really? It just DOESN'T try to TEST_HOST. Ahem. Anyway, it seems like we might
> want a wrapper shell function that does the repeated boilerplate stuff.
> Especially if the cmd.txt and out.txt source names remain regular (and we can
> symlink duplicates), then maybe something vaguely like:
>
> testvi() {
>   local one two
>   read -d '\0' one < "$FILES"/vi/ascii_$2.out
>   read -d '\0' two < "$FILES"/vi/ascii.txt
>   testcmd "$1" '-s "$FILES"/vi/$2.in input && cat input' \
>     "$one" "$two"
> }
>
> testvi "dd first line ascii" dd_first
> testvi "dd last line ascii" dd_last
>
> and so on.
>
> I added quotes around uses of "$FILES" because it's an absolute path to where
> the thing lives and our parent directory could have spaces in it. And testcmd is
> a variant of the testing function that supplies the command name as the first
> argument automatically. Trivial difference here but kind of necessary when
> testing things like shell builtins, to make sure you're actually testing the
> command out of the $PATH and not an alias or similar), so I removed "vi " from
> the start of the command string.
>
> We can't use the input file directly because it writes back a modified version,
> but the 4th argument to testing, when it isn't blank, populates a file called
> "input" which is automatically deleted again after the command finishes
> evaluating, so we don't need an "rm" statement. There isn't a "copy this file"
> syntax and "$(command)" has the nasty habit of deleting trailing newlines (and
> inexplicably not just stripping off ONE but ALL of them, so you can't even
> append a $'\n' to the end of your string and call it good. So instead I used
> read which with a NUL delimiter will read the trailing newlines into the
> variable. And yes I checked that mksh handles that. :)
>
> That said... I'd really prefer if all this stuff was inline in the vi.test so I
> could see what it's doing. "wc -l tests/files/vi/* | sort -n" says the longest
> file in there is 5 lines, that seems quite reasonable for inlining in the test
> script...
>
> > Also, In the error message handling, I replaced "sleep(1);" with "(void)getchar();".
>
> Hmmm... If an error happens in the middle of typing, it's easy to immediately
> dismiss it because you were typing fast. (I dunno what the right answer is here,
> what do vim and busybox do?)
>
> Rob
> _______________________________________________
> Toybox mailing list
> Toybox at lists.landley.net
> http://lists.landley.net/listinfo.cgi/toybox-landley.net