Re: [PATCH v3 2/4] difftool-helper: conciliate difftool.tabbed and difftool.prompt settings

[Junio C Hamano <gitster@pobox.com>]

Nicholas Guriev <nicholas@guriev.su> writes:

> The commit combines paths of compared files into a separate temporary
> file and prints them before launch the tool on last invocation of the
> helper. All temporary files will be removed before exiting after that.
> At least, we try. But it may happen the files remain in case of a bug
> or a random SIGKILL.
>
> Signed-off-by: Nicholas Guriev <nicholas@guriev.su>
> ---

Isn't the introduction of prompt_before_launch etc. would be a
welcome change before step [v3 1/4]?  If it makes a pure improvement
of the existing code without adding any new/additional feature like
the "tabbed" stuff, it would be better be discussed before anything
else in the series.

Then the new "tabbed" stuff can take advantage of the cleaned up
code with the new helpers---the patch that introduces the feature
will become more concise and easier to follow, I would think.

If I am reading the patches right, [v3 1&2/4] goes the other way
around: "let's add a new feature first in an ad-hoc way.  ah, it
seems that the code added for the new feature can be simplified if
we tweak a few things, and while we are doing so, we can adjust old
feature to use the same simplification again".

Regarding what [v3 1/4] does (which if we follow thru the above
suggestion would become [v4 2/4]), I am not sure that we want to add
'if tabbed mode, do this, otherwise do that' to run_diff_cmd.  It
would become awkward to keep piling such a change on top in the
function in the longer run.  I am wondering if each mergetool can
override run_diff_cmd as a whole, when it is dot-sourced to set up
itself.  The logic you added inside is_difftool_tabbed in [v3 1/4]
would probably need to be made available to the dot-sourced backends
so that they can just call something without reimplementing the same
logic, but by doing so, diff_combo_supported would become unneeded.
Tools that do not support the "combo" mode just do not have to do
anything.

But such a change would become easier to review if we do restructure
like this [v3 2/4] does earlier in the series _before_ a new feature
gets introduced.

Thanks.