| Message ID | 66a026ae678341fe7e93a89e22f76e24282cebaa.1596634463.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mergetool-lib: Don't use deprecated variable to detect GNOME | expand |
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
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.
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.
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index 204a5acd66..be28fe375f 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -266,6 +266,19 @@ run_merge_cmd () { fi } +is_desktop () { + IFS=':' + for desktop in ${XDG_CURRENT_DESKTOP} + do + if test "$desktop" = "$1" + then + return 0 + fi + done + + return 1 +} + list_merge_tool_candidates () { if merge_mode then @@ -275,7 +288,7 @@ list_merge_tool_candidates () { fi if test -n "$DISPLAY" then - if test -n "$GNOME_DESKTOP_SESSION_ID" + if is_desktop "GNOME" then tools="meld opendiff kdiff3 tkdiff xxdiff $tools" else