Re: [PATCH v3] RFC: mergetool: new config guiDefault supports auto-toggling gui by DISPLAY

[Tao Klerks <tao@klerks.biz>]

On Tue, Oct 18, 2022 at 8:54 AM Tao Klerks via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Tao Klerks <tao@klerks.biz>
>
> When no merge.tool or diff.tool is configured or manually selected, the
> selection of a default tool is sensitive to the DISPLAY variable; in a
> GUI session a gui-specific tool will be proposed if found, and
> otherwise a terminal-based one. This "GUI-optimizing" behavior is
> important because a GUI can make a huge difference to a user's ability
> to understand and correctly complete a non-trivial conflicting merge.
>
> Some time ago the merge.guitool and diff.guitool config options were
> introduced to enable users to configure both a GUI tool, and a non-GUI
> tool (with fallback if no GUI tool configured), in the same environment.
>
> Unfortunately, the --gui argument introduced to support the selection of
> the guitool is still explicit. When using configured tools, there is no
> equivalent of the no-tool-configured "propose a GUI tool if we are in a GUI
> environment" behavior.
>
> As proposed in <xmqqmtb8jsej.fsf@gitster.g>, introduce new configuration
> options, difftool.guiDefault and mergetool.guiDefault, supporting a special
> value "auto" which causes the corresponding tool or guitool to be selected
> depending on the presence of a non-empty DISPLAY value. Also support "true"
> to say "default to the guitool (unless --no-gui is passed on the
> commandline)", and "false" as the previous default behavior when these new
> configuration options are not specified.
>
> Signed-off-by: Tao Klerks <tao@klerks.biz>
> ---
>     RFC: mergetool: new config guiDefault supports auto-toggling gui by
>     DISPLAY
>
>     I'm reasonably comfortable that with this patch we do the right thing,
>     but I'm not sure about yet another remaining implementation detail:
>
>      * After implementing Junio's recommended "fail if defaulting config is
>        consulted and is invalid" flow, there now needs to be a distinction
>        between subshell exit code 1, which was used before and indicates
>        "tool not found or broken; falling back to default" and other
>        (higher) exit codes, which newly mean "something went wrong, stop!".
>        The resulting code looks awkward, I can't tell whether I'm missing a
>        code or even commenting pattern that would make it clearer.
>
>     V3:
>
>      * Simplify C code to use OPT_BOOL with an int rather than a custom
>        option-parsing function with an enum
>      * Fix doc to more extensively use backticks for config keys / values /
>        args
>      * Fix more shell script formatting issues
>      * Change error-handling in mergetool and difftool helpers to exit if
>        defaulting config is invalid
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1381%2FTaoK%2Ftao-mergetool-autogui-v3
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1381/TaoK/tao-mergetool-autogui-v3
> Pull-Request: https://github.com/gitgitgadget/git/pull/1381


Hi folks, this v3 never got any feedback - the only reason I had left
it as an RFC was that the error-handling looked a bit awkward, as I
noted above.

Should I resubmit this without the RFC prefix?

Are there any concerns about the change here to better support mixed
GUI/console-only environments?

Thanks,
Tao