{"thread":{"id":"53988","subject":"[PATCH 0/3] mergetool-lib: Don't use deprecated variable to detect GNOME","startedAt":"2020-08-05T16:44:17Z","lastAt":"2020-08-06T17:29:18Z","messageCount":17,"participants":["Marco Trevisan via GitGitGadget","Marco Trevisan (Treviño) via GitGitGadget","Eric Sunshine","Junio C Hamano","Johannes Sixt","Marco Trevisan (Treviño)"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"402912","messageId":"pull.693.git.1596634463.gitgitgadget@gmail.com","threadId":"53988","inReplyTo":null,"subject":"[PATCH 0/3] mergetool-lib: Don't use deprecated variable to detect GNOME","fromName":"Marco Trevisan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-05T13:34:20Z","receivedAt":"2020-08-05T16:44:17Z","isPatch":true,"sender":{"key":"name:Marco Trevisan","avatar":null},"body":"To list merge tool candidates we used to use a private GNOME env variable (\nGNOME_DESKTOP_SESSION_ID) that has been deprecated for long time ago and\nremoved as part of GNOME 3.30.0 release [1].\n\nSo replace this using XDG_CURRENT_DESKTOP instead, and cleanup the code to\navoid duplication and supporting KDE's kdiff3 better.\n\n[1] https://gitlab.gnome.org/GNOME/gnome-session/-/commit/00e0e6226371d53f65\n\nMarco Trevisan (Treviño) (3):\n mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME\n mergetool-lib: keep a list of cross desktop merge tools\n mergetool-lib: give kdiff3 prioirty in KDE environments\n\n git-mergetool--lib.sh | 23 ++++++++++++++++++++---\n 1 file changed, 20 insertions(+), 3 deletions(-)\n\n\nbase-commit: 85b4e0a6dc8407de6f69808d9ee6debdf167ced3\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-693%2F3v1n0%2Fdesktop-envs-fixes-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-693/3v1n0/desktop-envs-fixes-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/693\n-- \ngitgitgadget\n"},{"id":"402945","messageId":"66a026ae678341fe7e93a89e22f76e24282cebaa.1596634463.git.gitgitgadget@gmail.com","threadId":"53988","inReplyTo":"pull.693.git.1596634463.gitgitgadget@gmail.com","subject":"[PATCH 1/3] mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME","fromName":"Marco Trevisan (Treviño) via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-05T13:34:21Z","receivedAt":"2020-08-05T19:50:21Z","isPatch":true,"sender":{"key":"mail@3v1n0.net","avatar":"https://avatars.githubusercontent.com/u/345675?v=4"},"body":"From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= \n\nTo list merge tool candidates we used to use a private GNOME env\nvariable (GNOME_DESKTOP_SESSION_ID) that has been deprecated for long time ago\nand removed as part of GNOME 3.30.0 release [1].\n\nSo, git should instead check the XDG_CURRENT_DESKTOP env variable, that\nis supported by all the desktop environments.\n\nSince the variable is actually a colon-separated list of names that the current\ndesktop is known as, we need to go through all the values to ensure\nwe're using GNOME.\n\n[1] https://gitlab.gnome.org/GNOME/gnome-session/-/commit/00e0e6226371d53f65\n\nSigned-off-by: Marco Trevisan (Treviño) \n---\n git-mergetool--lib.sh | 15 ++++++++++++++-\n 1 file changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 204a5acd66..be28fe375f 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -266,6 +266,19 @@ run_merge_cmd () {\n \tfi\n }\n \n+is_desktop () {\n+\tIFS=':'\n+\tfor desktop in ${XDG_CURRENT_DESKTOP}\n+\tdo\n+\t\tif test \"$desktop\" = \"$1\"\n+\t\tthen\n+\t\t\treturn 0\n+\t\tfi\n+\tdone\n+\n+\treturn 1\n+}\n+\n list_merge_tool_candidates () {\n \tif merge_mode\n \tthen\n@@ -275,7 +288,7 @@ list_merge_tool_candidates () {\n \tfi\n \tif test -n \"$DISPLAY\"\n \tthen\n-\t\tif test -n \"$GNOME_DESKTOP_SESSION_ID\"\n+\t\tif is_desktop \"GNOME\"\n \t\tthen\n \t\t\ttools=\"meld opendiff kdiff3 tkdiff xxdiff $tools\"\n \t\telse\n-- \ngitgitgadget\n\n"},{"id":"402946","messageId":"fc0d2b103ec080fa38e5d51bf2205b7360c1b601.1596634463.git.gitgitgadget@gmail.com","threadId":"53988","inReplyTo":"pull.693.git.1596634463.gitgitgadget@gmail.com","subject":"[PATCH 2/3] mergetool-lib: keep a list of cross desktop merge tools","fromName":"Marco Trevisan (Treviño) via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-05T13:34:22Z","receivedAt":"2020-08-05T19:52:41Z","isPatch":true,"sender":{"key":"mail@3v1n0.net","avatar":"https://avatars.githubusercontent.com/u/345675?v=4"},"body":"From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= \n\nInstead of repeating the same tools multiple times, let's just keep them\nin another variable and list them depending the current desktop\n\nSigned-off-by: Marco Trevisan (Treviño) \n---\n git-mergetool--lib.sh | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex be28fe375f..243cd2b06b 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -288,11 +288,12 @@ list_merge_tool_candidates () {\n \tfi\n \tif test -n \"$DISPLAY\"\n \tthen\n+\t\tcross_desktop_tools=\"opendiff kdiff3 tkdiff xxdiff\"\n \t\tif is_desktop \"GNOME\"\n \t\tthen\n-\t\t\ttools=\"meld opendiff kdiff3 tkdiff xxdiff $tools\"\n+\t\t\ttools=\"meld $cross_desktop_tools $tools\"\n \t\telse\n-\t\t\ttools=\"opendiff kdiff3 tkdiff xxdiff meld $tools\"\n+\t\t\ttools=\"$cross_desktop_tools meld $tools\"\n \t\tfi\n \t\ttools=\"$tools gvimdiff diffuse diffmerge ecmerge\"\n \t\ttools=\"$tools p4merge araxis bc codecompare\"\n-- \ngitgitgadget\n\n"},{"id":"402949","messageId":"37090d232221415b227c165bd44f6711d21f376b.1596634463.git.gitgitgadget@gmail.com","threadId":"53988","inReplyTo":"pull.693.git.1596634463.gitgitgadget@gmail.com","subject":"[PATCH 3/3] mergetool-lib: give kdiff3 prioirty in KDE environments","fromName":"Marco Trevisan (Treviño) via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-05T13:34:23Z","receivedAt":"2020-08-05T20:01:53Z","isPatch":true,"sender":{"key":"mail@3v1n0.net","avatar":"https://avatars.githubusercontent.com/u/345675?v=4"},"body":"From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= \n\nSigned-off-by: Marco Trevisan (Treviño) \n---\n git-mergetool--lib.sh | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 243cd2b06b..90de6ee884 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -288,12 +288,15 @@ list_merge_tool_candidates () {\n \tfi\n \tif test -n \"$DISPLAY\"\n \tthen\n-\t\tcross_desktop_tools=\"opendiff kdiff3 tkdiff xxdiff\"\n+\t\tcross_desktop_tools=\"opendiff tkdiff xxdiff\"\n \t\tif is_desktop \"GNOME\"\n \t\tthen\n-\t\t\ttools=\"meld $cross_desktop_tools $tools\"\n+\t\t\ttools=\"meld $cross_desktop_tools kdiff3 $tools\"\n+\t\telif is_desktop \"KDE\"\n+\t\tthen\n+\t\t\ttools=\"kdiff3 $cross_desktop_tools meld $tools\"\n \t\telse\n-\t\t\ttools=\"$cross_desktop_tools meld $tools\"\n+\t\t\ttools=\"$cross_desktop_tools kdiff3 meld $tools\"\n \t\tfi\n \t\ttools=\"$tools gvimdiff diffuse diffmerge ecmerge\"\n \t\ttools=\"$tools p4merge araxis bc codecompare\"\n-- \ngitgitgadget\n"},{"id":"402964","messageId":"CAPig+cR_04fL85nH1tJnbWYbwk3Bn5fRqzr2JReB2xXu9iUGAQ@mail.gmail.com","threadId":"53988","inReplyTo":"66a026ae678341fe7e93a89e22f76e24282cebaa.1596634463.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-05T20:58:04Z","receivedAt":"2020-08-05T20:58:19Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Aug 5, 2020 at 3:51 PM Marco Trevisan (Treviño) via\nGitGitGadget wrote:\n> To list merge tool candidates we used to use a private GNOME env\n> variable (GNOME_DESKTOP_SESSION_ID) that has been deprecated for long time ago\n> and removed as part of GNOME 3.30.0 release [1].\n>\n> So, git should instead check the XDG_CURRENT_DESKTOP env variable, that\n> is supported by all the desktop environments.\n>\n> Since the variable is actually a colon-separated list of names that the current\n> desktop is known as, we need to go through all the values to ensure\n> we're using GNOME.\n>\n> Signed-off-by: Marco Trevisan (Treviño) \n> ---\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> @@ -266,6 +266,19 @@ run_merge_cmd () {\n> +is_desktop () {\n> + IFS=':'\n\nWe usually want to restore the value of IFS after we're done with it.\nFor instance:\n\n OLDIFS=$IFS\n IFS=:\n\nand then restore it before returning:\n\n IFS=$OLDIFS\n\n> + for desktop in ${XDG_CURRENT_DESKTOP}\n> + do\n> + if test \"$desktop\" = \"$1\"\n> + then\n> + return 0\n> + fi\n> + done\n> +\n> + return 1\n> +}\n\nRather than looping and mucking with IFS, even easier would be:\n\nis_desktop () {\n case \":$XDG_CURRENT_DESKTOP:\" in\n *:$1:*) return 0 ;;\n *) return 1 ;;\n esac\n}\n\nBut perhaps that's too magical for people?\n\n> @@ -275,7 +288,7 @@ list_merge_tool_candidates () {\n> - if test -n \"$GNOME_DESKTOP_SESSION_ID\"\n> + if is_desktop \"GNOME\"\n\nWhy do we need to retire the $GNOME_DESKTOP_SESSION_ID check here,\nthus penalizing people who might still be on an old version of GNOME?\nIt doesn't seem like it would be a maintenance burden to continue\nchecking it while also taking advantage of $XDG_CURRENT_DESKTOP:\n\n if test -n \"$GNOME_DESKTOP_SESSION_ID\" || is_desktop GNOME\n"},{"id":"402969","messageId":"CAPig+cRX3_ArTYQwc1jWHznBxLf+j0McYSo=nPq4C319J=DBvg@mail.gmail.com","threadId":"53988","inReplyTo":"fc0d2b103ec080fa38e5d51bf2205b7360c1b601.1596634463.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] mergetool-lib: keep a list of cross desktop merge tools","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-05T21:08:10Z","receivedAt":"2020-08-05T21:08:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Aug 5, 2020 at 3:53 PM Marco Trevisan (Treviño) via\nGitGitGadget wrote:\n> Instead of repeating the same tools multiple times, let's just keep them\n> in another variable and list them depending the current desktop\n>\n> Signed-off-by: Marco Trevisan (Treviño) \n> ---\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> @@ -288,11 +288,12 @@ list_merge_tool_candidates () {\n> if test -n \"$DISPLAY\"\n> then\n> + cross_desktop_tools=\"opendiff kdiff3 tkdiff xxdiff\"\n> if is_desktop \"GNOME\"\n> then\n> - tools=\"meld opendiff kdiff3 tkdiff xxdiff $tools\"\n> + tools=\"meld $cross_desktop_tools $tools\"\n> else\n> - tools=\"opendiff kdiff3 tkdiff xxdiff meld $tools\"\n> + tools=\"$cross_desktop_tools meld $tools\"\n> fi\n\nI have mixed feelings about this change. On the one hand, I see the\nreason for doing it if the list of tools remains substantially the\nsame in each case, but it also seems like it could become a burden,\npossibly requiring factoring the list into more pieces, as new\nplatforms or tools are added.\n\nWhat I might find more compelling is creation of a table of tools and\nthe platforms for which they are suitable. It doesn't seem like it\nwould be too difficult to express such a table in shell and to extract\nthe desired rows (but that might be overkill). At any rate, I'm rather\n\"meh\" on this change, though I don't oppose it strongly.\n"},{"id":"402970","messageId":"CAPig+cSn7pGzr+PYBMAgbp9V0z+OVUGgW=SDWV33HbqjMiiKhA@mail.gmail.com","threadId":"53988","inReplyTo":"37090d232221415b227c165bd44f6711d21f376b.1596634463.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] mergetool-lib: give kdiff3 prioirty in KDE environments","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-05T21:15:57Z","receivedAt":"2020-08-05T21:16:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Aug 5, 2020 at 4:02 PM Marco Trevisan (Treviño) via\nGitGitGadget wrote:\n> mergetool-lib: give kdiff3 prioirty in KDE environments\n\ns/prioirty/priority/\n\n> Signed-off-by: Marco Trevisan (Treviño) \n> ---\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> @@ -288,12 +288,15 @@ list_merge_tool_candidates () {\n> - cross_desktop_tools=\"opendiff kdiff3 tkdiff xxdiff\"\n> + cross_desktop_tools=\"opendiff tkdiff xxdiff\"\n> if is_desktop \"GNOME\"\n> then\n> - tools=\"meld $cross_desktop_tools $tools\"\n> + tools=\"meld $cross_desktop_tools kdiff3 $tools\"\n> + elif is_desktop \"KDE\"\n> + then\n> + tools=\"kdiff3 $cross_desktop_tools meld $tools\"\n> else\n> - tools=\"$cross_desktop_tools meld $tools\"\n> + tools=\"$cross_desktop_tools kdiff3 meld $tools\"\n> fi\n\nWouldn't this change the behavior for people running old KDE which\ndoesn't have XDG_CURRENT_DESKTOP, giving \"kdiff3\" much lower priority\nthan it had before?\n\nThis change also illustrates why I wasn't convinced that patch 2/3 was\nnecessarily a good idea. When you touch 'cross_desktop_tools' here,\nyou end up having to touch all the other 'tools=' lines anyhow, so the\nintroduction of 'cross_desktop_tools' didn't buy much in terms of\nreduced maintenance cost.\n"},{"id":"402994","messageId":"xmqqlfisn3jk.fsf@gitster.c.googlers.com","threadId":"53988","inReplyTo":"CAPig+cR_04fL85nH1tJnbWYbwk3Bn5fRqzr2JReB2xXu9iUGAQ@mail.gmail.com","subject":"Re: [PATCH 1/3] mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-05T22:14:39Z","receivedAt":"2020-08-05T22:14:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Rather than looping and mucking with IFS, even easier would be:\n>\n> is_desktop () {\n> case \":$XDG_CURRENT_DESKTOP:\" in\n> *:$1:*) return 0 ;;\n> *) return 1 ;;\n> esac\n> }\n>\n> But perhaps that's too magical for people?\n\nNah, that is exactly how 'case' in shell is supposed to be used.\n\nThanks.\n"},{"id":"403029","messageId":"4ef90c30-6c06-a355-69eb-9c8e10c6bef6@kdbg.org","threadId":"53988","inReplyTo":"CAPig+cRX3_ArTYQwc1jWHznBxLf+j0McYSo=nPq4C319J=DBvg@mail.gmail.com","subject":"Re: [PATCH 2/3] mergetool-lib: keep a list of cross desktop merge tools","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2020-08-06T08:16:49Z","receivedAt":"2020-08-06T11:05:23Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 05.08.20 um 23:08 schrieb Eric Sunshine:\n> On Wed, Aug 5, 2020 at 3:53 PM Marco Trevisan (Treviño) via\n> GitGitGadget wrote:\n>> Instead of repeating the same tools multiple times, let's just keep them\n>> in another variable and list them depending the current desktop\n>>\n>> Signed-off-by: Marco Trevisan (Treviño) \n>> ---\n>> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n>> @@ -288,11 +288,12 @@ list_merge_tool_candidates () {\n>> if test -n \"$DISPLAY\"\n>> then\n>> + cross_desktop_tools=\"opendiff kdiff3 tkdiff xxdiff\"\n>> if is_desktop \"GNOME\"\n>> then\n>> - tools=\"meld opendiff kdiff3 tkdiff xxdiff $tools\"\n>> + tools=\"meld $cross_desktop_tools $tools\"\n>> else\n>> - tools=\"opendiff kdiff3 tkdiff xxdiff meld $tools\"\n>> + tools=\"$cross_desktop_tools meld $tools\"\n>> fi\n> \n> I have mixed feelings about this change. On the one hand, I see the\n> reason for doing it if the list of tools remains substantially the\n> same in each case, but it also seems like it could become a burden,\n> possibly requiring factoring the list into more pieces, as new\n> platforms or tools are added.\n> \n> What I might find more compelling is creation of a table of tools and\n> the platforms for which they are suitable. It doesn't seem like it\n> would be too difficult to express such a table in shell and to extract\n> the desired rows (but that might be overkill). At any rate, I'm rather\n> \"meh\" on this change, though I don't oppose it strongly.\n\nThere may be some confusion caused by the name of the new variable. A\nbetter name would perhaps be $tools_with_desktop_dependent_preference.\n(That's a tongue-in-cheek suggestion, BTW.)\n\nOn a side note, the refactoring that happened in 21d0ba7ebb0c\n(\"difftool/mergetool: refactor commands to use git-mergetool--lib\",\n2009-04-08) did not preserve the original order of diff tools. The\nspirit was to prefer meld over kompare and kdiff3 with GNOME, and the\nother way round with KDE. But since that refactoring, kompare is always\nfirst in the list.\n\n-- Hannes\n"},{"id":"403036","messageId":"CAPig+cTgkD02VfvGnECRdkiZDS1jLEXXPveY2h61DY83n5rMnw@mail.gmail.com","threadId":"53988","inReplyTo":"CAALaSc=U65aqgK95zWG2=qSNguHLYkve4Hu=30WxyVOFYPUX+w@mail.gmail.com","subject":"Re: [PATCH 3/3] mergetool-lib: give kdiff3 prioirty in KDE environments","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-06T16:17:07Z","receivedAt":"2020-08-06T16:43:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 6, 2020 at 8:37 AM Marco Trevisan (Treviño) wrote:\n> Il giorno mer 5 ago 2020 alle ore 23:16 Eric Sunshine\n> ha scritto:\n> > Wouldn't this change the behavior for people running old KDE which\n> > doesn't have XDG_CURRENT_DESKTOP, giving \"kdiff3\" much lower priority\n> > than it had before?\n>\n> Yeah, true.. So to avoid this we can just also check for\n> KDE_FULL_SESSION, that has been introduced by KDE 3.2, and this should\n> be enough I think.\n\nI'm not a user of git-mergetool or KDE, so I can't speak as an\nend-user. My comment was made merely as a reviewer of the code.\n\nIf it is easy to avoid the behavior change by also checking\nKDE_FULL_SESSION in addition to the new check of XDG_CURRENT_DESKTOP\nwithout it being a maintenance burden, then that sounds like a good\nchoice. On the other hand, if there wasn't a good way to avoid\nchanging behavior for users of older KDE, then explaining that in the\ncommit message would be expected.\n"},{"id":"403051","messageId":"CAALaSckL5sKynGDRoqN7vd6cuXAvGwaDSyB4tVGXxZWGkNG_gw@mail.gmail.com","threadId":"53988","inReplyTo":"CAPig+cR_04fL85nH1tJnbWYbwk3Bn5fRqzr2JReB2xXu9iUGAQ@mail.gmail.com","subject":"Re: [PATCH 1/3] mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME","fromName":"Marco Trevisan (Treviño)","fromEmail":"mail@3v1n0.net","sentAt":"2020-08-06T12:12:49Z","receivedAt":"2020-08-06T17:03:15Z","isPatch":true,"sender":{"key":"mail@3v1n0.net","avatar":"https://avatars.githubusercontent.com/u/345675?v=4"},"body":"Hi,\n\nIl giorno mer 5 ago 2020 alle ore 22:58 Eric Sunshine\n ha scritto:\n>\n> On Wed, Aug 5, 2020 at 3:51 PM Marco Trevisan (Treviño) via\n> GitGitGadget wrote:\n> > To list merge tool candidates we used to use a private GNOME env\n> > variable (GNOME_DESKTOP_SESSION_ID) that has been deprecated for long time ago\n> > and removed as part of GNOME 3.30.0 release [1].\n> >\n> > So, git should instead check the XDG_CURRENT_DESKTOP env variable, that\n> > is supported by all the desktop environments.\n> >\n> > Since the variable is actually a colon-separated list of names that the current\n> > desktop is known as, we need to go through all the values to ensure\n> > we're using GNOME.\n> >\n> > Signed-off-by: Marco Trevisan (Treviño) \n> > ---\n> > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> > @@ -266,6 +266,19 @@ run_merge_cmd () {\n> > +is_desktop () {\n> > + IFS=':'\n>\n> We usually want to restore the value of IFS after we're done with it.\n> For instance:\n>\n> OLDIFS=$IFS\n> IFS=:\n>\n> and then restore it before returning:\n>\n> IFS=$OLDIFS\n\nYeah, that's true and always a good practice, but it was never\nhappening in this file, so I followed the style.\n\n> > + for desktop in ${XDG_CURRENT_DESKTOP}\n> > + do\n> > + if test \"$desktop\" = \"$1\"\n> > + then\n> > + return 0\n> > + fi\n> > + done\n> > +\n> > + return 1\n> > +}\n>\n> Rather than looping and mucking with IFS, even easier would be:\n>\n> is_desktop () {\n> case \":$XDG_CURRENT_DESKTOP:\" in\n> *:$1:*) return 0 ;;\n> *) return 1 ;;\n> esac\n> }\n>\n> But perhaps that's too magical for people?\n\nOk, fair enough, this is fine as well.\n\n> > @@ -275,7 +288,7 @@ list_merge_tool_candidates () {\n> > - if test -n \"$GNOME_DESKTOP_SESSION_ID\"\n> > + if is_desktop \"GNOME\"\n>\n> Why do we need to retire the $GNOME_DESKTOP_SESSION_ID check here,\n> thus penalizing people who might still be on an old version of GNOME?\n> It doesn't seem like it would be a maintenance burden to continue\n> checking it while also taking advantage of $XDG_CURRENT_DESKTOP:\n>\n> if test -n \"$GNOME_DESKTOP_SESSION_ID\" || is_desktop GNOME\n\nOk, keeping this as well, it's even true that most people won't be in\ngnome 2 these days, but not a problem to keep it around.\n\n-- \nTreviño's World - Life and Linux\nhttp://www.3v1n0.net\n"},{"id":"403053","messageId":"26b25b0d652e3a5b834eb004151f4a322c450403.1596727673.git.gitgitgadget@gmail.com","threadId":"53988","inReplyTo":"pull.693.v2.git.1596727673.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME","fromName":"Marco Trevisan (Treviño) via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-06T15:27:52Z","receivedAt":"2020-08-06T17:10:20Z","isPatch":true,"sender":{"key":"mail@3v1n0.net","avatar":"https://avatars.githubusercontent.com/u/345675?v=4"},"body":"From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= \n\nTo list merge tool candidates we used to use a private GNOME env\nvariable (GNOME_DESKTOP_SESSION_ID) that has been deprecated for long time ago\nand removed as part of GNOME 3.30.0 release [1].\n\nSo, git should instead primarily check the XDG_CURRENT_DESKTOP env variable,\nthat is now supported by all the desktop environments.\n\nSince the variable is actually a colon-separated list of names that the current\ndesktop is known as, we need to check if the value is set if we're using GNOME.\n\n[1] https://gitlab.gnome.org/GNOME/gnome-session/-/commit/00e0e6226371d53f65\n\nSigned-off-by: Marco Trevisan (Treviño) \n---\n git-mergetool--lib.sh | 13 ++++++++++++-\n 1 file changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 204a5acd66..f9d8f309c8 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -266,6 +266,17 @@ run_merge_cmd () {\n \tfi\n }\n \n+is_desktop () {\n+\tcase \":$XDG_CURRENT_DESKTOP:\" in\n+\t*:$1:*)\n+\t\treturn 0\n+\t\t;;\n+\t*)\n+\t\treturn 1\n+\t\t;;\n+\tesac\n+}\n+\n list_merge_tool_candidates () {\n \tif merge_mode\n \tthen\n@@ -275,7 +286,7 @@ list_merge_tool_candidates () {\n \tfi\n \tif test -n \"$DISPLAY\"\n \tthen\n-\t\tif test -n \"$GNOME_DESKTOP_SESSION_ID\"\n+\t\tif is_desktop \"GNOME\" || test -n \"$GNOME_DESKTOP_SESSION_ID\"\n \t\tthen\n \t\t\ttools=\"meld opendiff kdiff3 tkdiff xxdiff $tools\"\n \t\telse\n-- \ngitgitgadget\n\n"},{"id":"403055","messageId":"pull.693.v2.git.1596727673.gitgitgadget@gmail.com","threadId":"53988","inReplyTo":"pull.693.git.1596634463.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] mergetool-lib: Don't use deprecated variable to detect GNOME","fromName":"Marco Trevisan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-06T15:27:51Z","receivedAt":"2020-08-06T17:11:50Z","isPatch":true,"sender":{"key":"name:Marco Trevisan","avatar":null},"body":"To list merge tool candidates we used to use a private GNOME env variable (\nGNOME_DESKTOP_SESSION_ID) that has been deprecated for long time ago and\nremoved as part of GNOME 3.30.0 release [1].\n\nSo replace this using XDG_CURRENT_DESKTOP instead, and cleanup the code to\navoid duplication and supporting KDE's kdiff3 better.\n\n[1] https://gitlab.gnome.org/GNOME/gnome-session/-/commit/00e0e6226371d53f65\n\nMarco Trevisan (Treviño) (2):\n mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME\n mergetool-lib: give kdiff3 priority in KDE environments\n\n git-mergetool--lib.sh | 16 +++++++++++++++-\n 1 file changed, 15 insertions(+), 1 deletion(-)\n\n\nbase-commit: 85b4e0a6dc8407de6f69808d9ee6debdf167ced3\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-693%2F3v1n0%2Fdesktop-envs-fixes-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-693/3v1n0/desktop-envs-fixes-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/693\n\nRange-diff vs v1:\n\n 1: 66a026ae67 ! 1: 26b25b0d65 mergetool-lib: use $XDG_CURRENT_DESKTOP to check GNOME\n @@ Commit message\n variable (GNOME_DESKTOP_SESSION_ID) that has been deprecated for long time ago\n and removed as part of GNOME 3.30.0 release [1].\n \n - So, git should instead check the XDG_CURRENT_DESKTOP env variable, that\n - is supported by all the desktop environments.\n + So, git should instead primarily check the XDG_CURRENT_DESKTOP env variable,\n + that is now supported by all the desktop environments.\n \n Since the variable is actually a colon-separated list of names that the current\n - desktop is known as, we need to go through all the values to ensure\n - we're using GNOME.\n + desktop is known as, we need to check if the value is set if we're using GNOME.\n \n [1] https://gitlab.gnome.org/GNOME/gnome-session/-/commit/00e0e6226371d53f65\n \n @@ git-mergetool--lib.sh: run_merge_cmd () {\n }\n \n +is_desktop () {\n -+\tIFS=':'\n -+\tfor desktop in ${XDG_CURRENT_DESKTOP}\n -+\tdo\n -+\t\tif test \"$desktop\" = \"$1\"\n -+\t\tthen\n -+\t\t\treturn 0\n -+\t\tfi\n -+\tdone\n -+\n -+\treturn 1\n ++\tcase \":$XDG_CURRENT_DESKTOP:\" in\n ++\t*:$1:*)\n ++\t\treturn 0\n ++\t\t;;\n ++\t*)\n ++\t\treturn 1\n ++\t\t;;\n ++\tesac\n +}\n +\n list_merge_tool_candidates () {\n @@ git-mergetool--lib.sh: list_merge_tool_candidates () {\n \tif test -n \"$DISPLAY\"\n \tthen\n -\t\tif test -n \"$GNOME_DESKTOP_SESSION_ID\"\n -+\t\tif is_desktop \"GNOME\"\n ++\t\tif is_desktop \"GNOME\" || test -n \"$GNOME_DESKTOP_SESSION_ID\"\n \t\tthen\n \t\t\ttools=\"meld opendiff kdiff3 tkdiff xxdiff $tools\"\n \t\telse\n 2: fc0d2b103e < -: ---------- mergetool-lib: keep a list of cross desktop merge tools\n 3: 37090d2322 ! 2: c18a5edf50 mergetool-lib: give kdiff3 prioirty in KDE environments\n @@ Metadata\n Author: Marco Trevisan (Treviño) \n \n ## Commit message ##\n - mergetool-lib: give kdiff3 prioirty in KDE environments\n + mergetool-lib: give kdiff3 priority in KDE environments\n \n Signed-off-by: Marco Trevisan (Treviño) \n \n ## git-mergetool--lib.sh ##\n @@ git-mergetool--lib.sh: list_merge_tool_candidates () {\n - \tfi\n - \tif test -n \"$DISPLAY\"\n - \tthen\n --\t\tcross_desktop_tools=\"opendiff kdiff3 tkdiff xxdiff\"\n -+\t\tcross_desktop_tools=\"opendiff tkdiff xxdiff\"\n - \t\tif is_desktop \"GNOME\"\n + \t\tif is_desktop \"GNOME\" || test -n \"$GNOME_DESKTOP_SESSION_ID\"\n \t\tthen\n --\t\t\ttools=\"meld $cross_desktop_tools $tools\"\n -+\t\t\ttools=\"meld $cross_desktop_tools kdiff3 $tools\"\n -+\t\telif is_desktop \"KDE\"\n + \t\t\ttools=\"meld opendiff kdiff3 tkdiff xxdiff $tools\"\n ++\t\telif is_desktop \"KDE\" || test x\"$KDE_FULL_SESSION\" = x\"true\"\n +\t\tthen\n -+\t\t\ttools=\"kdiff3 $cross_desktop_tools meld $tools\"\n ++\t\t\ttools=\"kdiff3 opendiff tkdiff xxdiff meld $tools\"\n \t\telse\n --\t\t\ttools=\"$cross_desktop_tools meld $tools\"\n -+\t\t\ttools=\"$cross_desktop_tools kdiff3 meld $tools\"\n + \t\t\ttools=\"opendiff kdiff3 tkdiff xxdiff meld $tools\"\n \t\tfi\n - \t\ttools=\"$tools gvimdiff diffuse diffmerge ecmerge\"\n - \t\ttools=\"$tools p4merge araxis bc codecompare\"\n\n-- \ngitgitgadget\n"},{"id":"403057","messageId":"c18a5edf505cf8421f4465c9f6e26bce8d94d107.1596727673.git.gitgitgadget@gmail.com","threadId":"53988","inReplyTo":"pull.693.v2.git.1596727673.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] mergetool-lib: give kdiff3 priority in KDE environments","fromName":"Marco Trevisan (Treviño) via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-06T15:27:53Z","receivedAt":"2020-08-06T17:22:49Z","isPatch":true,"sender":{"key":"mail@3v1n0.net","avatar":"https://avatars.githubusercontent.com/u/345675?v=4"},"body":"From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= \n\nSigned-off-by: Marco Trevisan (Treviño) \n---\n git-mergetool--lib.sh | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex f9d8f309c8..ac6695ea26 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -289,6 +289,9 @@ list_merge_tool_candidates () {\n \t\tif is_desktop \"GNOME\" || test -n \"$GNOME_DESKTOP_SESSION_ID\"\n \t\tthen\n \t\t\ttools=\"meld opendiff kdiff3 tkdiff xxdiff $tools\"\n+\t\telif is_desktop \"KDE\" || test x\"$KDE_FULL_SESSION\" = x\"true\"\n+\t\tthen\n+\t\t\ttools=\"kdiff3 opendiff tkdiff xxdiff meld $tools\"\n \t\telse\n \t\t\ttools=\"opendiff kdiff3 tkdiff xxdiff meld $tools\"\n \t\tfi\n-- \ngitgitgadget\n"},{"id":"403060","messageId":"CAALaScmKT44snh-5zTqe=Y2gjXjpo3fxRrbzotsXnr=+3Ap9ew@mail.gmail.com","threadId":"53988","inReplyTo":"CAPig+cRX3_ArTYQwc1jWHznBxLf+j0McYSo=nPq4C319J=DBvg@mail.gmail.com","subject":"Re: [PATCH 2/3] mergetool-lib: keep a list of cross desktop merge tools","fromName":"Marco Trevisan (Treviño)","fromEmail":"mail@3v1n0.net","sentAt":"2020-08-06T12:28:45Z","receivedAt":"2020-08-06T17:23:20Z","isPatch":true,"sender":{"key":"mail@3v1n0.net","avatar":"https://avatars.githubusercontent.com/u/345675?v=4"},"body":"Il giorno mer 5 ago 2020 alle ore 23:08 Eric Sunshine\n ha scritto:\n>\n> On Wed, Aug 5, 2020 at 3:53 PM Marco Trevisan (Treviño) via\n> GitGitGadget wrote:\n> > Instead of repeating the same tools multiple times, let's just keep them\n> > in another variable and list them depending the current desktop\n> >\n> > Signed-off-by: Marco Trevisan (Treviño) \n> > ---\n> > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> > @@ -288,11 +288,12 @@ list_merge_tool_candidates () {\n> > if test -n \"$DISPLAY\"\n> > then\n> > + cross_desktop_tools=\"opendiff kdiff3 tkdiff xxdiff\"\n> > if is_desktop \"GNOME\"\n> > then\n> > - tools=\"meld opendiff kdiff3 tkdiff xxdiff $tools\"\n> > + tools=\"meld $cross_desktop_tools $tools\"\n> > else\n> > - tools=\"opendiff kdiff3 tkdiff xxdiff meld $tools\"\n> > + tools=\"$cross_desktop_tools meld $tools\"\n> > fi\n>\n> I have mixed feelings about this change. On the one hand, I see the\n> reason for doing it if the list of tools remains substantially the\n> same in each case, but it also seems like it could become a burden,\n> possibly requiring factoring the list into more pieces, as new\n> platforms or tools are added.\n\nI kind of agree on that, so it was mostly a proposal but I can withdraw it.\n\n> What I might find more compelling is creation of a table of tools and\n> the platforms for which they are suitable. It doesn't seem like it\n> would be too difficult to express such a table in shell and to extract\n> the desired rows (but that might be overkill). At any rate, I'm rather\n> \"meh\" on this change, though I don't oppose it strongly.\n\nYeah, I was thinking the same, but also could be a bit complicated to\nmaintain especially when it comes using something that needs to be\nsupported by pure sh.\n\n-- \nTreviño's World - Life and Linux\nhttp://www.3v1n0.net\n"},{"id":"403061","messageId":"CAALaSc=U65aqgK95zWG2=qSNguHLYkve4Hu=30WxyVOFYPUX+w@mail.gmail.com","threadId":"53988","inReplyTo":"CAPig+cSn7pGzr+PYBMAgbp9V0z+OVUGgW=SDWV33HbqjMiiKhA@mail.gmail.com","subject":"Re: [PATCH 3/3] mergetool-lib: give kdiff3 prioirty in KDE environments","fromName":"Marco Trevisan (Treviño)","fromEmail":"mail@3v1n0.net","sentAt":"2020-08-06T12:36:28Z","receivedAt":"2020-08-06T17:24:13Z","isPatch":true,"sender":{"key":"mail@3v1n0.net","avatar":"https://avatars.githubusercontent.com/u/345675?v=4"},"body":"Il giorno mer 5 ago 2020 alle ore 23:16 Eric Sunshine\n ha scritto:\n>\n> On Wed, Aug 5, 2020 at 4:02 PM Marco Trevisan (Treviño) via\n> GitGitGadget wrote:\n> > mergetool-lib: give kdiff3 prioirty in KDE environments\n>\n> s/prioirty/priority/\n>\n> > Signed-off-by: Marco Trevisan (Treviño) \n> > ---\n> > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> > @@ -288,12 +288,15 @@ list_merge_tool_candidates () {\n> > - cross_desktop_tools=\"opendiff kdiff3 tkdiff xxdiff\"\n> > + cross_desktop_tools=\"opendiff tkdiff xxdiff\"\n> > if is_desktop \"GNOME\"\n> > then\n> > - tools=\"meld $cross_desktop_tools $tools\"\n> > + tools=\"meld $cross_desktop_tools kdiff3 $tools\"\n> > + elif is_desktop \"KDE\"\n> > + then\n> > + tools=\"kdiff3 $cross_desktop_tools meld $tools\"\n> > else\n> > - tools=\"$cross_desktop_tools meld $tools\"\n> > + tools=\"$cross_desktop_tools kdiff3 meld $tools\"\n> > fi\n>\n> Wouldn't this change the behavior for people running old KDE which\n> doesn't have XDG_CURRENT_DESKTOP, giving \"kdiff3\" much lower priority\n> than it had before?\n\nYeah, true.. So to avoid this we can just also check for\nKDE_FULL_SESSION, that has been introduced by KDE 3.2, and this should\nbe enough I think.\n\n> This change also illustrates why I wasn't convinced that patch 2/3 was\n> necessarily a good idea. When you touch 'cross_desktop_tools' here,\n> you end up having to touch all the other 'tools=' lines anyhow, so the\n> introduction of 'cross_desktop_tools' didn't buy much in terms of\n> reduced maintenance cost.\n\nYeah, I had the same feeling, TBH.\n\n-- \nTreviño's World - Life and Linux\nhttp://www.3v1n0.net\n"},{"id":"403064","messageId":"CAALaScnJfaJg5moOL5DAFb2yDpkT03emVAZnBZs3+uo8rGbOLA@mail.gmail.com","threadId":"53988","inReplyTo":"4ef90c30-6c06-a355-69eb-9c8e10c6bef6@kdbg.org","subject":"Re: [PATCH 2/3] mergetool-lib: keep a list of cross desktop merge tools","fromName":"Marco Trevisan (Treviño)","fromEmail":"mail@3v1n0.net","sentAt":"2020-08-06T12:19:39Z","receivedAt":"2020-08-06T17:29:18Z","isPatch":true,"sender":{"key":"mail@3v1n0.net","avatar":"https://avatars.githubusercontent.com/u/345675?v=4"},"body":"Hi,\n\nIl giorno gio 6 ago 2020 alle ore 10:16 Johannes Sixt ha scritto:\n>\n> On a side note, the refactoring that happened in 21d0ba7ebb0c\n> (\"difftool/mergetool: refactor commands to use git-mergetool--lib\",\n> 2009-04-08) did not preserve the original order of diff tools. The\n> spirit was to prefer meld over kompare and kdiff3 with GNOME, and the\n> other way round with KDE. But since that refactoring, kompare is always\n> first in the list.\n\nWell, actually when DISPLAY is set so basically always (as even under\nWAYLAND that's defined for XWayland apps) meld is still preferred.\n\nI'm not sure it makes sense to have tortoisemerge or kompare if no\ndisplay is set though, but I didn't want to change the whole thing.\n\n-- \nTreviño's World - Life and Linux\nhttp://www.3v1n0.net\n"}]}