pijul_org / pijul

#230 [patch] improve editor support for vim, nano, and gvim

Opened by boxofrox, on November 22, 2017
pijul-0.9
Refactoring
Closed
boxofrox commented on November 22, 2017

Not sure what the process for pull requests is with pijul. If this isn't the way to go about it, please point me in the right direction.

This patch enhances the editor config option to accept a command line string with flags. The allows users to use gvim -f to keep gvim as a foreground process and block pijul until gvim is closed.

I also replaced std::process::Command::output() with std::process::Command::status() to support vim and nano. output() captures stdin/stdout, which interferes with vim and nano, while status() passes the parent stdin/stdout to the child process and makes vim & nano happy.

Not sure which editors were working before. Let me know and I can see about testing those, also. Tested on Archlinux x64 with vim 8.0, neovim 0.2.1, and nano 2.8.7.

Update 1: rerecorded patch to sign it
Update 2: patch replaced, see discussion below

lthms commented on November 23, 2017

Yay! Thanks a lot for that, I was willing to do it since I first implemented this thing, but I wasn’t sure where to begin.

I will try your patch today and if it works, I think we can merge it.

Can you try to use the “Add patches” feature (the button close to “Close and comment”)?

Edit: I can confirme it works!

The only thing I would argue is I don't find the _exit_status very useful (you don’t even use it later).

So, why not the following:

if editor_cmd.len() == 1 {
        let _ = process::Command::new(&editor_cmd[0])
            .arg(patch_name_file.clone())
            .current_dir(repo_root)
            .status()?;
} else {
        let _ = process::Command::new(&editor_cmd[0])
            .args(&editor_cmd[1..])
            .arg(patch_name_file.clone())
            .current_dir(repo_root)
            .status()?;
}
boxofrox commented on November 23, 2017

Don't know why I didn't notice the "Add patches" button.

I'll toss out _exit_status and add the patch.

boxofrox commented on November 23, 2017

I figure, why not drop the if-statement? But if you really want it, I can do that instead. :)

lthms commented on November 23, 2017

That’s better indeed, I was not particulary enjoying it to be honest. Let me just try that, just to be sure, and then I will apply this patch.

lthms added tag
Refactoring
on November 23, 2017
lthms closed this discussion on November 23, 2017