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

[Rob Landley]

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. 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