darcs

Patch 1952 make diff use execInteractive

Title make diff use execInteractive
Superseder Nosy List raichoo
Related Issues
Status accepted Assigned To
Milestone

Created on 2020-01-17.18:07:00 by raichoo, last changed 2020-01-30.18:02:54 by bf.

Files
File name Status Uploaded Type Edit Remove
add-gui-flag-to-_diff_.dpatch raichoo, 2020-01-20.17:59:34 application/octet-stream
add-interactive-flag-to-diff-command.dpatch raichoo, 2020-01-17.18:07:00 application/octet-stream
fix-diff_command-for-tools-like-vimdiff.dpatch raichoo, 2020-01-19.18:51:10 application/octet-stream
make-diff-use-execinteractive.dpatch raichoo, 2020-01-24.10:45:08 application/octet-stream
make-diff-use-execinteractive.dpatch raichoo, 2020-01-24.17:36:30 application/octet-stream
See mailing list archives for discussion on individual patches.
Messages
msg21684 (view) Author: raichoo Date: 2020-01-17.18:07:00
Hi,

I was having some issues using `nvim` as a diff command for diff so
I hacked around a bit and added interactive as a flag to `diff` to make this work. This is just a suggestion if there is another way to make this work please let me know since this is kind of essential for a code review workflow I'm currently developing.
Attachments
msg21685 (view) Author: raichoo Date: 2020-01-19.18:51:10
This patch is now explicitly using `execInteractive`.
Attachments
msg21686 (view) Author: raichoo Date: 2020-01-20.17:59:34
The I/O behavior is now configurable via the `gui` flag.
Additionally I've introduced `darcs review` to allow diff to be
configured with different defaults.
Attachments
msg21687 (view) Author: bf Date: 2020-01-21.13:19:23
Hi raichoo

thanks for the patch(es). It took me a while to figure out what the problem is that you are fixing here, it would have helped to state that more clearly.

The problem is that external diff programs that run in the terminal instead of a separate window (like vimdiff or in fact the plain diff) use stdin and stdout to perform drawing and receiving user input; they don't work when we redirect stdin/stdout to newly created handles (as execPipeIgnoreError does).

Now to the fix: I agree that using execInteractive is the right thing, as this is what we use when we execute an editor, too. I don't think we need a new flag, though. It should be possible to use execInteractive in all cases without anything breaking. For instance, I am using darcs with DARCS_EDITOR set to a GUI editor and that works just fine, so meld and co should also work fine with execInteractive.
msg21688 (view) Author: bf Date: 2020-01-21.13:31:37
[damn, I forgot that using the text edit widget on the tracker messes up
the formatting, sending again via email]

Hi raichoo

thanks for the patch(es). It took me a while to figure out what the
problem is that you are fixing here, it would have helped to state that
more clearly.

The problem is that external diff programs that run in the terminal
instead of a separate window (like vimdiff or in fact the plain diff)
use stdin and stdout to perform drawing and receiving user input; they
don't work when we redirect stdin/stdout to newly created handles (as
execPipeIgnoreError does).

Now to the fix: I agree that using execInteractive is the right thing,
as this is what we use when we execute an editor, too. I don't think we
need a new flag, though. It should be possible to use execInteractive in
all cases without anything breaking. For instance, I am using darcs with
DARCS_EDITOR set to a GUI editor and that works just fine, so I don't
see a reason why e.g. meld should not.
msg21689 (view) Author: raichoo Date: 2020-01-23.16:15:38
Hi Ben, thanks for the feedback. I was actually kind of unsure how to elaborate on the issue, sorry for causing you the extra work.

I've tried it with just using `execInteractive` but that broke regular `diff` on my system (running FreeBSD 12.1-STABLE here, if that matters). That's the reason why I reintroduced the flag in the first place. I'm not quite sure how this would work without the flag to be quite honest.
msg21690 (view) Author: bf Date: 2020-01-23.17:20:30
> It should be possible to use execInteractive in all cases without
> anything breaking.
I am seeing now that this would break getDiffDoc which is exported,
although it is not used anywhere else in darcs. This was deliberate:

patch 57cfa5be6b80cace5b7a735fdf15252a8041c384
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Wed Dec  2 07:29:36 CET 2015
  * resolve issue2481: expose API for 'darcs diff' command
  This is wanted for a new darcsden feature

However, getDiffDoc is not used in darcsden as of today. So I wonder if
we still need to export it.
msg21691 (view) Author: bf Date: 2020-01-23.17:43:24
What breaks the regular diff is that you pass an empty string as an
extra argument. Apparently diff doesn't like that whereas vimdiff just
ignores it.

In my version I fixed that by changing the type of execInteractive to

  String -> Maybe String -> IO ExitCode

i.e. allow the argument to be optional, and then pass Nothing instead of
an empty argument.

I would have sent a patch with my solution, but then I stumbled over the
problem with getDiffDoc no longer working as expected...
msg21692 (view) Author: raichoo Date: 2020-01-24.10:45:08
Something along those lines? I know this needs better naming (rather than just adding a single quote to the original functions name), this is just for testing things. It seems to work so far, or do you see any issues with this change?
Attachments
msg21695 (view) Author: bf Date: 2020-01-24.15:17:36
Yes, this is largely what I meant.

However, this won't compile on Windows because you forgot to adapt the
#else part of execInteractive'.

Regarding names:

 * I wouldn't bother with adding a new function execInteractive', just
change the type of execInteractive and adapt the only other call site
(Darcs.UI.External.runEditor), perhaps using a local function definition
there to avoid repetition of Just.

 * The cmd' in the diff command I would name cmdline, indicating that it
may consist of a command plus arguments.

The fact that we are now returning 'empty' makes it quite clear that
getDiffDoc no longer works as expected: the output of the external diff
will be seen on the console but it will not be part of the Doc returned
from getDiffDoc.

Since neither Ganesh nor Simon have commented, I feel entitled to
unilaterally decide that we can remove getDiffDoc and fold it back into
doDiff. This will also allow us to fix a second issue with your (last)
patch: we now see the output from the external diff command /before/ the
patch descriptions are printed, whereas the intention of the code
obviously is to first print the patch descriptions and then the diff (as
we do with whatsnew). So we need to move the call to rundiff down i.e.
after we output the result of 'changelog (getDiffInfo opts morepatches)'.
msg21698 (view) Author: raichoo Date: 2020-01-24.17:36:30
Thanks for your feedback :) Here's what I came up with.
Attachments
msg21702 (view) Author: bf Date: 2020-01-25.12:57:17
Thanks, this one looks pretty good.

Screening it now, consider as reviewed.
msg21769 (view) Author: bf Date: 2020-01-30.18:02:54
I already reviewed this one.
History
Date User Action Args
2020-01-17 18:07:00raichoocreate
2020-01-19 18:51:11raichoosetfiles: + fix-diff_command-for-tools-like-vimdiff.dpatch
messages: + msg21685
2020-01-20 17:59:35raichoosetfiles: + add-gui-flag-to-_diff_.dpatch
messages: + msg21686
2020-01-21 13:19:24bfsetmessages: + msg21687
2020-01-21 13:31:37bfsetmessages: + msg21688
2020-01-23 16:15:38raichoosetmessages: + msg21689
2020-01-23 17:20:31bfsetmessages: + msg21690
2020-01-23 17:43:24bfsetmessages: + msg21691
2020-01-24 10:45:08raichoosetfiles: + make-diff-use-execinteractive.dpatch
messages: + msg21692
2020-01-24 15:17:37bfsetmessages: + msg21695
2020-01-24 17:36:30raichoosetfiles: + make-diff-use-execinteractive.dpatch
messages: + msg21698
2020-01-25 12:57:17bfsetstatus: needs-screening -> needs-review
messages: + msg21702
2020-01-30 17:59:57bfsettitle: Added interactive flag to diff -> make diff use execInteractive
2020-01-30 18:02:54bfsetstatus: needs-review -> accepted
messages: + msg21769