chrisosaurus / dodo

scriptable in place file editor
MIT License
4 stars 1 forks source link

Fix bug surrounding escape character, implement truncate command #19

Closed phillid closed 8 years ago

phillid commented 8 years ago

Addresses issues #11, #16, #17, #18 and a few code tidies here and there.

phillid commented 8 years ago

Actually, I need to put this on hold until I fix a few issues I found while doing some more thorough testing… I have noticed that the file stream is only seeked to p->offset when a b, p or e command is issued, and not w… Is this behaviour unintended?

It leads to some interesting results:

$ echo thing > test.in
$ dodo test.in <<< "e/t/ w/a/ e/h/ w/i/"
$ cat test.in
ihing

When I would have expected either:

Sandwiching a print command after the first write command fseeks the file to the correct (?) offset. Running the same commands above, except with e/t/ w/a/ p e/h/ w/i/ as dodo's program input gives the correct (?) error: Eval_expect: expected string 'h', got 'a'

Shall I fix this and append the commit to this PR? Truncate requires the program offset in offset to be accurate.

chrisosaurus commented 8 years ago

@phillid thanks for catching that, I would have expected a w// to advance the cursor by the number of bytes written

Your example then should be

$ echo thing > test.in
$ dodo test.in <<< "e/t/ w/a/ e/h/ w/i/"
$ cat test.in
aiing

That discrepancy is interesting, as it means that w// is still placed at the first item and e// is looking at the second, which is clearly very wrong.

If you have time it would be awesome if you could fix this.

chrisosaurus commented 8 years ago

@phillid once this issues around write-seek is fixed we also need to update t/tests/write-seek.out to correctly contain aiing.

chrisosaurus commented 8 years ago

@phillid I had some spare time so I quickly fixed the issue you found above, after a write we now do a seek to get past the end of the write.

If you are able to rebase your work here on top of current master it should be ready to merge, hope I didn't cause any issue by jumping on that bug.

chrisosaurus commented 8 years ago

@phillid actually no rebasing is needed, it all merges cleanly. Merging now, thank you!

If you noticed any other issues then please let me know.