martanne / vis

A vi-like editor based on Plan 9's structural regular expressions
Other
4.19k stars 259 forks source link

vis:pipe returns wrong exit status (when non-zero) #1130

Closed pippi1otta closed 8 months ago

pippi1otta commented 9 months ago

A repro snippet (comparison with a vis:communicate handler):

vis.events.subscribe(vis.events.INIT, function()
    vis:map(vis.modes.NORMAL, '<M-h>', function()
        local code = vis:pipe(nil, nil, "exit 2")
        vis:info(tostring(code))
    end)
    vis:map(vis.modes.NORMAL, '<M-g>', function()
        vis:communicate("", "exit 2")
        vis.events.subscribe(vis.events.PROCESS_RESPONSE, function(name, rtype, code, buffer)
            vis:info(tostring(code))
        end)
    end)
end)

In this example, vis:pipe returns 512, while the vis:communicate handler returns 2. (Checked with emacs, it also displays 2).

I believe the fix is something along these lines:

diff --git a/vis.c b/vis.c
index 004295b..04b0959 100644
--- a/vis.c
+++ b/vis.c
@@ -1960,6 +1960,8 @@ err:
        vis->interrupted = false;
        vis->ui->terminal_restore(vis->ui);

+       if WIFEXITED(status)
+               return WEXITSTATUS(status);
        return status;
 }
mcepl commented 9 months ago

Applied in the devel branch of https://git.sr.ht/~mcepl/vis and it seems to work just fine. I could easily reproduce the error before, but not after. Will probably move to my master branch soon.

rnpnr commented 8 months ago

Sorry for the slow reply. This fix is correct but I modified it slightly and included an explanation in the commit message.

I think there is an opportunity to share some code between vis_pipe() and vis_process_communicate() so that these differences can be avoided. But that can be a different commit.