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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 25, 2021, 23:04 UTC
Message-ID: <xmqqa6swy5y6.fsf@gitster.c.googlers.com>
In-Reply-To: <20210125212132.894458-3-nicholas@guriev.su>

Nicholas Guriev <nicholas@guriev.su> writes:

Show 8 quoted lines

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