git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Tao Klerks <tao@klerks.biz>
Date
Feb 17, 2023, 10:59 UTC
Message-ID
<CAPMMpoi8mbqSAYMbhgYRj0UTjxHnGy50Z3HKP6fOaDj7AcQ=mA@mail.gmail.com>
In-Reply-To
<pull.1381.v3.git.1666076086910.gitgitgadget@gmail.com>

On Tue, Oct 18, 2022 at 8:54 AM Tao Klerks via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 56 quoted lines
>
> 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

Previous: Tao Klerks via GitGitGadgetNext: Tao Klerks via GitGitGadget
Message 18 of 23 in “mergetool: new config guiDefault supports auto-toggling gui by DISPLAY”
  1. mergetool: new config guiDefault supports auto-toggling gui by DISPLAYTao Klerks via GitGitGadget, Oct 12, 2022
  2. Tao KlerksOct 12, 2022
  3. Junio C HamanoOct 12, 2022
  4. Tao KlerksOct 13, 2022
  5. Junio C HamanoOct 13, 2022
  6. Tao KlerksOct 14, 2022
  7. Junio C HamanoOct 14, 2022
  8. Tao KlerksOct 14, 2022
  9. Junio C HamanoOct 14, 2022
  10. Tao KlerksOct 16, 2022
  11. RFC: mergetool: new config guiDefault supports auto-toggling gui by DISPLAYTao Klerks via GitGitGadget, Oct 14, 2022
  12. Eric SunshineOct 14, 2022
  13. Tao KlerksOct 14, 2022
  14. Junio C HamanoOct 14, 2022
  15. Tao KlerksOct 16, 2022
  16. Junio C HamanoOct 17, 2022
  17. RFC: mergetool: new config guiDefault supports auto-toggling gui by DISPLAYTao Klerks via GitGitGadget, Oct 18, 2022
  18. Tao KlerksFeb 17, 2023
  19. mergetool: new config guiDefault supports auto-toggling gui by DISPLAYTao Klerks via GitGitGadget, Mar 18, 2023
  20. David AguilarApr 4, 2023
  21. Tao KlerksApr 4, 2023
  22. Junio C HamanoApr 4, 2023
  23. David AguilarApr 6, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.