git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Johannes Sixt <j6t@kdbg.org>
To: "Eric Sunshine" <sunshine@sunshineco.com>,
	"Marco Trevisan (Treviño) via GitGitGadget"
	<gitgitgadget@gmail.com>
Cc: "Git List" <git@vger.kernel.org>,
	"Marco Trevisan (Treviño)" <mail@3v1n0.net>
Subject: Re: [PATCH 2/3] mergetool-lib: keep a list of cross desktop merge tools
Date: Thu, 6 Aug 2020 10:16:49 +0200	[thread overview]
Message-ID: <4ef90c30-6c06-a355-69eb-9c8e10c6bef6@kdbg.org> (raw)
In-Reply-To: <CAPig+cRX3_ArTYQwc1jWHznBxLf+j0McYSo=nPq4C319J=DBvg@mail.gmail.com>

Am 05.08.20 um 23:08 schrieb Eric Sunshine:
> On Wed, Aug 5, 2020 at 3:53 PM Marco Trevisan (Treviño) via
> GitGitGadget <gitgitgadget@gmail.com> wrote:
>> Instead of repeating the same tools multiple times, let's just keep them
>> in another variable and list them depending the current desktop
>>
>> Signed-off-by: Marco Trevisan (Treviño) <mail@3v1n0.net>
>> ---
>> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
>> @@ -288,11 +288,12 @@ list_merge_tool_candidates () {
>>         if test -n "$DISPLAY"
>>         then
>> +               cross_desktop_tools="opendiff kdiff3 tkdiff xxdiff"
>>                 if is_desktop "GNOME"
>>                 then
>> -                       tools="meld opendiff kdiff3 tkdiff xxdiff $tools"
>> +                       tools="meld $cross_desktop_tools $tools"
>>                 else
>> -                       tools="opendiff kdiff3 tkdiff xxdiff meld $tools"
>> +                       tools="$cross_desktop_tools meld $tools"
>>                 fi
> 
> I have mixed feelings about this change. On the one hand, I see the
> reason for doing it if the list of tools remains substantially the
> same in each case, but it also seems like it could become a burden,
> possibly requiring factoring the list into more pieces, as new
> platforms or tools are added.
> 
> What I might find more compelling is creation of a table of tools and
> the platforms for which they are suitable. It doesn't seem like it
> would be too difficult to express such a table in shell and to extract
> the desired rows (but that might be overkill). At any rate, I'm rather
> "meh" on this change, though I don't oppose it strongly.

There may be some confusion caused by the name of the new variable. A
better name would perhaps be $tools_with_desktop_dependent_preference.
(That's a tongue-in-cheek suggestion, BTW.)

On a side note, the refactoring that happened in 21d0ba7ebb0c
("difftool/mergetool: refactor commands to use git-mergetool--lib",
2009-04-08) did not preserve the original order of diff tools. The
spirit was to prefer meld over kompare and kdiff3 with GNOME, and the
other way round with KDE. But since that refactoring, kompare is always
first in the list.

-- Hannes

  reply	other threads:[~2020-08-06  8:17 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-05 13:34 [PATCH 0/3] mergetool-lib: Don't use deprecated variable to detect GNOME Marco Trevisan via GitGitGadget
2020-08-05 13:34 ` [PATCH 1/3] mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME Marco Trevisan (Treviño) via GitGitGadget
2020-08-05 20:58   ` Eric Sunshine
2020-08-05 22:14     ` Junio C Hamano
2020-08-06 12:12     ` Marco Trevisan (Treviño)
2020-08-05 13:34 ` [PATCH 2/3] mergetool-lib: keep a list of cross desktop merge tools Marco Trevisan (Treviño) via GitGitGadget
2020-08-05 21:08   ` Eric Sunshine
2020-08-06  8:16     ` Johannes Sixt [this message]
2020-08-06 12:19       ` Marco Trevisan (Treviño)
2020-08-06 12:28     ` Marco Trevisan (Treviño)
2020-08-05 13:34 ` [PATCH 3/3] mergetool-lib: give kdiff3 prioirty in KDE environments Marco Trevisan (Treviño) via GitGitGadget
2020-08-05 21:15   ` Eric Sunshine
2020-08-06 12:36     ` Marco Trevisan (Treviño)
2020-08-06 16:17       ` Eric Sunshine
2020-08-06 15:27 ` [PATCH v2 0/2] mergetool-lib: Don't use deprecated variable to detect GNOME Marco Trevisan via GitGitGadget
2020-08-06 15:27   ` [PATCH v2 1/2] mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME Marco Trevisan (Treviño) via GitGitGadget
2020-08-06 15:27   ` [PATCH v2 2/2] mergetool-lib: give kdiff3 priority in KDE environments Marco Trevisan (Treviño) via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4ef90c30-6c06-a355-69eb-9c8e10c6bef6@kdbg.org \
    --to=j6t@kdbg.org \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=mail@3v1n0.net \
    --cc=sunshine@sunshineco.com \
    --subject='Re: [PATCH 2/3] mergetool-lib: keep a list of cross desktop merge tools' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).