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

• add-interactive-flag-to-diff-command.dpatch

This patch is now explicitly using `execInteractive`.

Attachments

• fix-diff_command-for-tools-like-vimdiff.dpatch

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

• add-gui-flag-to-_diff_.dpatch

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.

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

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.

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

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

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

• make-diff-use-execinteractive.dpatch

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

Thanks for your feedback :) Here's what I came up with.

Attachments

• make-diff-use-execinteractive.dpatch

Thanks, this one looks pretty good.

Screening it now, consider as reviewed.

I already reviewed this one.