mergetool-lib: Don't use deprecated variable to detect GNOME #693
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3v1n0
force-pushed
the
desktop-envs-fixes
branch
from
b5d132f to
98e7e7b
Compare
|
There is an issue in commit 5032cfc: |
|
There is an issue in commit f7e09d8: |
|
There is an issue in commit 98e7e7b: |
3v1n0
force-pushed
the
desktop-envs-fixes
branch
from
98e7e7b to
37090d2
Compare
|
/submit |
|
Submitted as pull.693.git.1596634463.gitgitgadget@gmail.com |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -266,6 +266,19 @@ run_merge_cmd () { | |||
| fi | |||
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Wed, Aug 5, 2020 at 3:51 PM Marco Trevisan (Treviño) via
GitGitGadget <gitgitgadget@gmail.com> wrote:
> To list merge tool candidates we used to use a private GNOME env
> variable (GNOME_DESKTOP_SESSION_ID) that has been deprecated for long time ago
> and removed as part of GNOME 3.30.0 release [1].
>
> So, git should instead check the XDG_CURRENT_DESKTOP env variable, that
> is supported by all the desktop environments.
>
> Since the variable is actually a colon-separated list of names that the current
> desktop is known as, we need to go through all the values to ensure
> we're using GNOME.
>
> Signed-off-by: Marco Trevisan (Treviño) <mail@3v1n0.net>
> ---
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> @@ -266,6 +266,19 @@ run_merge_cmd () {
> +is_desktop () {
> + IFS=':'
We usually want to restore the value of IFS after we're done with it.
For instance:
OLDIFS=$IFS
IFS=:
and then restore it before returning:
IFS=$OLDIFS
> + for desktop in ${XDG_CURRENT_DESKTOP}
> + do
> + if test "$desktop" = "$1"
> + then
> + return 0
> + fi
> + done
> +
> + return 1
> +}
Rather than looping and mucking with IFS, even easier would be:
is_desktop () {
case ":$XDG_CURRENT_DESKTOP:" in
*:$1:*) return 0 ;;
*) return 1 ;;
esac
}
But perhaps that's too magical for people?
> @@ -275,7 +288,7 @@ list_merge_tool_candidates () {
> - if test -n "$GNOME_DESKTOP_SESSION_ID"
> + if is_desktop "GNOME"
Why do we need to retire the $GNOME_DESKTOP_SESSION_ID check here,
thus penalizing people who might still be on an old version of GNOME?
It doesn't seem like it would be a maintenance burden to continue
checking it while also taking advantage of $XDG_CURRENT_DESKTOP:
if test -n "$GNOME_DESKTOP_SESSION_ID" || is_desktop GNOME
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Eric Sunshine <sunshine@sunshineco.com> writes:
> Rather than looping and mucking with IFS, even easier would be:
>
> is_desktop () {
> case ":$XDG_CURRENT_DESKTOP:" in
> *:$1:*) return 0 ;;
> *) return 1 ;;
> esac
> }
>
> But perhaps that's too magical for people?
Nah, that is exactly how 'case' in shell is supposed to be used.
Thanks.
There was a problem hiding this comment.
On the Git mailing list, Marco Trevisan (Treviño) wrote (reply to this):
Hi,
Il giorno mer 5 ago 2020 alle ore 22:58 Eric Sunshine
<sunshine@sunshineco.com> ha scritto:
>
> On Wed, Aug 5, 2020 at 3:51 PM Marco Trevisan (Treviño) via
> GitGitGadget <gitgitgadget@gmail.com> wrote:
> > To list merge tool candidates we used to use a private GNOME env
> > variable (GNOME_DESKTOP_SESSION_ID) that has been deprecated for long time ago
> > and removed as part of GNOME 3.30.0 release [1].
> >
> > So, git should instead check the XDG_CURRENT_DESKTOP env variable, that
> > is supported by all the desktop environments.
> >
> > Since the variable is actually a colon-separated list of names that the current
> > desktop is known as, we need to go through all the values to ensure
> > we're using GNOME.
> >
> > Signed-off-by: Marco Trevisan (Treviño) <mail@3v1n0.net>
> > ---
> > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> > @@ -266,6 +266,19 @@ run_merge_cmd () {
> > +is_desktop () {
> > + IFS=':'
>
> We usually want to restore the value of IFS after we're done with it.
> For instance:
>
> OLDIFS=$IFS
> IFS=:
>
> and then restore it before returning:
>
> IFS=$OLDIFS
Yeah, that's true and always a good practice, but it was never
happening in this file, so I followed the style.
> > + for desktop in ${XDG_CURRENT_DESKTOP}
> > + do
> > + if test "$desktop" = "$1"
> > + then
> > + return 0
> > + fi
> > + done
> > +
> > + return 1
> > +}
>
> Rather than looping and mucking with IFS, even easier would be:
>
> is_desktop () {
> case ":$XDG_CURRENT_DESKTOP:" in
> *:$1:*) return 0 ;;
> *) return 1 ;;
> esac
> }
>
> But perhaps that's too magical for people?
Ok, fair enough, this is fine as well.
> > @@ -275,7 +288,7 @@ list_merge_tool_candidates () {
> > - if test -n "$GNOME_DESKTOP_SESSION_ID"
> > + if is_desktop "GNOME"
>
> Why do we need to retire the $GNOME_DESKTOP_SESSION_ID check here,
> thus penalizing people who might still be on an old version of GNOME?
> It doesn't seem like it would be a maintenance burden to continue
> checking it while also taking advantage of $XDG_CURRENT_DESKTOP:
>
> if test -n "$GNOME_DESKTOP_SESSION_ID" || is_desktop GNOME
Ok, keeping this as well, it's even true that most people won't be in
gnome 2 these days, but not a problem to keep it around.
--
Treviño's World - Life and Linux
http://www.3v1n0.net
| @@ -266,6 +266,19 @@ run_merge_cmd () { | |||
| fi | |||
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
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 was a problem hiding this comment.
On the Git mailing list, Johannes Sixt wrote (reply to this):
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
There was a problem hiding this comment.
On the Git mailing list, Marco Trevisan (Treviño) wrote (reply to this):
Il giorno mer 5 ago 2020 alle ore 23:08 Eric Sunshine
<sunshine@sunshineco.com> ha scritto:
>
> 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.
I kind of agree on that, so it was mostly a proposal but I can withdraw it.
> 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.
Yeah, I was thinking the same, but also could be a bit complicated to
maintain especially when it comes using something that needs to be
supported by pure sh.
--
Treviño's World - Life and Linux
http://www.3v1n0.net
There was a problem hiding this comment.
On the Git mailing list, Marco Trevisan (Treviño) wrote (reply to this):
Hi,
Il giorno gio 6 ago 2020 alle ore 10:16 Johannes Sixt <j6t@kdbg.org> ha scritto:
>
> 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.
Well, actually when DISPLAY is set so basically always (as even under
WAYLAND that's defined for XWayland apps) meld is still preferred.
I'm not sure it makes sense to have tortoisemerge or kompare if no
display is set though, but I didn't want to change the whole thing.
--
Treviño's World - Life and Linux
http://www.3v1n0.net
| @@ -266,6 +266,19 @@ run_merge_cmd () { | |||
| fi | |||
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Wed, Aug 5, 2020 at 4:02 PM Marco Trevisan (Treviño) via
GitGitGadget <gitgitgadget@gmail.com> wrote:
> mergetool-lib: give kdiff3 prioirty in KDE environments
s/prioirty/priority/
> Signed-off-by: Marco Trevisan (Treviño) <mail@3v1n0.net>
> ---
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> @@ -288,12 +288,15 @@ list_merge_tool_candidates () {
> - cross_desktop_tools="opendiff kdiff3 tkdiff xxdiff"
> + cross_desktop_tools="opendiff tkdiff xxdiff"
> if is_desktop "GNOME"
> then
> - tools="meld $cross_desktop_tools $tools"
> + tools="meld $cross_desktop_tools kdiff3 $tools"
> + elif is_desktop "KDE"
> + then
> + tools="kdiff3 $cross_desktop_tools meld $tools"
> else
> - tools="$cross_desktop_tools meld $tools"
> + tools="$cross_desktop_tools kdiff3 meld $tools"
> fi
Wouldn't this change the behavior for people running old KDE which
doesn't have XDG_CURRENT_DESKTOP, giving "kdiff3" much lower priority
than it had before?
This change also illustrates why I wasn't convinced that patch 2/3 was
necessarily a good idea. When you touch 'cross_desktop_tools' here,
you end up having to touch all the other 'tools=' lines anyhow, so the
introduction of 'cross_desktop_tools' didn't buy much in terms of
reduced maintenance cost.
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Thu, Aug 6, 2020 at 8:37 AM Marco Trevisan (Treviño) <mail@3v1n0.net> wrote:
> Il giorno mer 5 ago 2020 alle ore 23:16 Eric Sunshine
> <sunshine@sunshineco.com> ha scritto:
> > Wouldn't this change the behavior for people running old KDE which
> > doesn't have XDG_CURRENT_DESKTOP, giving "kdiff3" much lower priority
> > than it had before?
>
> Yeah, true.. So to avoid this we can just also check for
> KDE_FULL_SESSION, that has been introduced by KDE 3.2, and this should
> be enough I think.
I'm not a user of git-mergetool or KDE, so I can't speak as an
end-user. My comment was made merely as a reviewer of the code.
If it is easy to avoid the behavior change by also checking
KDE_FULL_SESSION in addition to the new check of XDG_CURRENT_DESKTOP
without it being a maintenance burden, then that sounds like a good
choice. On the other hand, if there wasn't a good way to avoid
changing behavior for users of older KDE, then explaining that in the
commit message would be expected.
There was a problem hiding this comment.
On the Git mailing list, Marco Trevisan (Treviño) wrote (reply to this):
Il giorno mer 5 ago 2020 alle ore 23:16 Eric Sunshine
<sunshine@sunshineco.com> ha scritto:
>
> On Wed, Aug 5, 2020 at 4:02 PM Marco Trevisan (Treviño) via
> GitGitGadget <gitgitgadget@gmail.com> wrote:
> > mergetool-lib: give kdiff3 prioirty in KDE environments
>
> s/prioirty/priority/
>
> > Signed-off-by: Marco Trevisan (Treviño) <mail@3v1n0.net>
> > ---
> > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> > @@ -288,12 +288,15 @@ list_merge_tool_candidates () {
> > - cross_desktop_tools="opendiff kdiff3 tkdiff xxdiff"
> > + cross_desktop_tools="opendiff tkdiff xxdiff"
> > if is_desktop "GNOME"
> > then
> > - tools="meld $cross_desktop_tools $tools"
> > + tools="meld $cross_desktop_tools kdiff3 $tools"
> > + elif is_desktop "KDE"
> > + then
> > + tools="kdiff3 $cross_desktop_tools meld $tools"
> > else
> > - tools="$cross_desktop_tools meld $tools"
> > + tools="$cross_desktop_tools kdiff3 meld $tools"
> > fi
>
> Wouldn't this change the behavior for people running old KDE which
> doesn't have XDG_CURRENT_DESKTOP, giving "kdiff3" much lower priority
> than it had before?
Yeah, true.. So to avoid this we can just also check for
KDE_FULL_SESSION, that has been introduced by KDE 3.2, and this should
be enough I think.
> This change also illustrates why I wasn't convinced that patch 2/3 was
> necessarily a good idea. When you touch 'cross_desktop_tools' here,
> you end up having to touch all the other 'tools=' lines anyhow, so the
> introduction of 'cross_desktop_tools' didn't buy much in terms of
> reduced maintenance cost.
Yeah, I had the same feeling, TBH.
--
Treviño's World - Life and Linux
http://www.3v1n0.net
37090d2 to
74b691d
Compare
To list merge tool candidates we used to use a private GNOME env variable (GNOME_DESKTOP_SESSION_ID) that has been deprecated for long time ago and removed as part of GNOME 3.30.0 release [1]. So, git should instead primarily check the XDG_CURRENT_DESKTOP env variable, that is now supported by all the desktop environments. Since the variable is actually a colon-separated list of names that the current desktop is known as, we need to check if the value is set if we're using GNOME. [1] https://gitlab.gnome.org/GNOME/gnome-session/-/commit/00e0e6226371d53f65 Signed-off-by: Marco Trevisan (Treviño) <mail@3v1n0.net>
Signed-off-by: Marco Trevisan (Treviño) <mail@3v1n0.net>
3v1n0
force-pushed
the
desktop-envs-fixes
branch
from
74b691d to
c18a5ed
Compare
|
/submit |
|
Submitted as pull.693.v2.git.1596727673.gitgitgadget@gmail.com |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
To list merge tool candidates we used to use a private GNOME env variable (
GNOME_DESKTOP_SESSION_ID) that has been deprecated for long time ago and removed as part of GNOME 3.30.0 release [1].So replace this using
XDG_CURRENT_DESKTOPinstead, and cleanup the code to avoid duplication and supporting KDE's kdiff3 better.[1] https://gitlab.gnome.org/GNOME/gnome-session/-/commit/00e0e6226371d53f65