use of IFS= breaks GfW

Question

use of IFS= breaks GfW

r2evans opened this issue 4 years ago · 11 comments

This is a formalization of troubleshooting made while investigating #60.

The issue is with IFS= and windows' propensity to use C: at the start of paths.

For instance, consider this:

echo -e "executable is [$0]"
echo "dirname is [$(dirname $0)/../lib]"

If added to git-issue.sh before (< 33) or after (> 51) the block of code that overrides IFS, I see

executable is [C:/Users/r2/AppData/Roaming/git-issue/bin/git-issue]
dirname is [C:/Users/r2/AppData/Roaming/git-issue/bin/../lib]

but between lines 33 and 51 (around your LIB_PATH code where IFS=":" temporarily), I see

executable is [C:/Users/r2/AppData/Roaming/git-issue/bin/git-issue]
dirname0 is [.
/Users/r2/AppData/Roaming/git-issue/bin]

The easy looping of LIB_PATH by using IFS is broken by that executable.

(I thought having this as a separate issue would do better to track the problem, vice discussing too long within the PR.)

Answer 1 · 2020-02-04T21:21:41.000Z

Two things that appear to work:

Infer: if the dirname of the executable has as its second character a colon, then transform to GfW's fancy c-drive-mounting (e.g., c:/Users/r2 --> /c/Users/r2).

diff --git a/git-issue.sh b/git-issue.sh
index 5965aa1..0398486 100755
--- a/git-issue.sh
+++ b/git-issue.sh
@@ -29,6 +29,12 @@
 # SC2034 : USER_AGENT appears unused. Verify use (or export if used externally)
 USER_AGENT=https://github.com/dspinellis/git-issue/tree/afda065
 
+DIRN=$(dirname $0)
+if [ "${DIRN:1:1}" = ":" ]; then
+  # Git-for-windows, IFS will break this later
+  DIRN="/${DIRN:0:1}${DIRN:2}"
+fi
+
 # Determine our script library path
 my_IFS=$IFS
 IFS=:
@@ -36,7 +42,7 @@ IFS=:
 # Set library path
 # shellcheck disable=SC2086
 # Rationale: Word splitting not an issue
-LIB_PATH="$(dirname $0)/../lib:$LD_LIBRARY_PATH:/usr/lib:/usr/local/lib"
+LIB_PATH="${DIRN}/../lib:$LD_LIBRARY_PATH:/usr/lib:/usr/local/lib"
 if [ "x$GIT_ISSUE_LIB_PATH" != x ] ; then
   LIB_PATH="$GIT_ISSUE_LIB_PATH"
 fi

Substring replacement: replace any colons, and re-add them as used:

diff --git a/git-issue.sh b/git-issue.sh
index 5965aa1..9f31387 100755
--- a/git-issue.sh
+++ b/git-issue.sh
@@ -29,6 +29,9 @@
 # SC2034 : USER_AGENT appears unused. Verify use (or export if used externally)
 USER_AGENT=https://github.com/dspinellis/git-issue/tree/afda065
 
+DIRN=$(dirname $0)
+DIRN=${DIRN/:/|}
+
 # Determine our script library path
 my_IFS=$IFS
 IFS=:
@@ -36,13 +39,13 @@ IFS=:
 # Set library path
 # shellcheck disable=SC2086
 # Rationale: Word splitting not an issue
-LIB_PATH="$(dirname $0)/../lib:$LD_LIBRARY_PATH:/usr/lib:/usr/local/lib"
+LIB_PATH="${DIRN}/../lib:$LD_LIBRARY_PATH:/usr/lib:/usr/local/lib"
 if [ "x$GIT_ISSUE_LIB_PATH" != x ] ; then
   LIB_PATH="$GIT_ISSUE_LIB_PATH"
 fi
 for i in ${LIB_PATH} ; do
-  if [ -d "${i}/git-issue" ] ; then
-    MY_LIB="${i}/git-issue"
+  if [ -d "${i/|/:}/git-issue" ] ; then
+    MY_LIB="${i/|/:}/git-issue"
     break
   fi
 done

Personally I prefer the first because of its specificity, it is less likely to mess things up on other platforms.

If you think that it is not sufficient to find a colon in the second character, then we can try to use another canary ...

the env var $OSTYPE, as it is "msys" on my GfW and either "linux-gnu" (normal shell) or unset (called from git, I think) on my ubuntu 16.04 box;

I don't know if there is another non-GfW system out there that would advertise "msys" (that may not be impacted by this problem).

It is safe to ignore this next chunk of envvars. On the same lines, there are many envvars that appear to be only found in my GfW instance and not in my linux environments. I'll include all of them here, knowing many are too common and/or just weird. Things that could be good indicators are the presence and/or value of COMPEC, HOMEDRIVE, MACHTYPE, or OS (not used on linux?).

Output from "set"

ACLOCAL_PATH='C:\Program Files\Git\mingw64\share\aclocal;C:\Program Files\Git\usr\share\aclocal'
ALLUSERSPROFILE='C:\ProgramData'
APPDATA='C:\Users\r2\AppData\Roaming'
BASH=/usr/bin/sh
BASHOPTS=cmdhist:complete_fullquote:extquote:force_fignore:hostcomplete:interactive_comments:progcomp:promptvars:sourcepath
BASH_ALIASES=()
BASH_ARGC=()
BASH_ARGV=()
BASH_CMDS=()
BASH_LINENO=([0]="0")
BASH_SOURCE=([0]="C:/Users/r2/AppData/Roaming/git-issue/bin/git-issue")
BASH_VERSINFO=([0]="4" [1]="4" [2]="23" [3]="1" [4]="release" [5]="x86_64-pc-msys")
BASH_VERSION='4.4.23(1)-release'
COMMONPROGRAMFILES='C:\Program Files\Common Files'
COMPUTERNAME=D2SB2
COMSPEC='C:\WINDOWS\system32\cmd.exe'
CONFIG_SITE='C:/Program Files/Git/mingw64/etc/config.site'
CommonProgramW6432='C:\Program Files\Common Files'
DIRN='C|/Users/r2/AppData/Roaming/git-issue/bin'
DIRSTACK=()
DISPLAY=needs-to-be-defined
DriverData='C:\Windows\System32\Drivers\DriverData'
EUID=197609
EXEPATH='C:\Program Files\Git'
FPS_BROWSER_APP_PROFILE_STRING='Internet Explorer'
FPS_BROWSER_USER_PROFILE_STRING=Default
GIT_DIR=C:/Users/r2/Projects/git-issue/.git/worktrees/_57_gfw_install
GIT_EXEC_PATH='C:/Program Files/Git/mingw64/libexec/git-core'
GIT_PREFIX=
GROUPS=()
HOME=/c/Users/r2
HOMEDRIVE=C:
HOMEPATH='\Users\r2'
HOSTNAME=d2sb2
HOSTTYPE=x86_64
IFS='
'
INFOPATH='C:\Program Files\Git\usr\local\info;C:\Program Files\Git\usr\share\info;C:\Program Files\Git\usr\info;C:\Program Files\Git\share\info'
LANG=en_US.UTF-8
LOCALAPPDATA='C:\Users\r2\AppData\Local'
LOGONSERVER='\\D2SB2'
MACHTYPE=x86_64-pc-msys
MANPATH='C:\Program Files\Git\mingw64\local\man;C:\Program Files\Git\mingw64\share\man;C:\Program Files\Git\usr\local\man;C:\Program Files\Git\usr\share\man;C:\Program Files\Git\usr\man;C:\Program Files\Git\share\man'
MINGW_CHOST=x86_64-w64-mingw32
MINGW_PACKAGE_PREFIX=mingw-w64-x86_64
MINGW_PREFIX='C:/Program Files/Git/mingw64'
MSYSTEM=MINGW64
MSYSTEM_CARCH=x86_64
MSYSTEM_CHOST=x86_64-w64-mingw32
MSYSTEM_PREFIX='C:/Program Files/Git/mingw64'
NUMBER_OF_PROCESSORS=8
OPTERR=1
OPTIND=1
ORIGINAL_PATH='/mingw64/bin:/usr/bin:/c/Users/r2/bin:/c/Program Files/ImageMagick-7.0.8-Q16:/c/WINDOWS/system32:/c/WINDOWS:/c/WINDOWS/System32/Wbem:/c/WINDOWS/System32/WindowsPowerShell/v1.0:/c/WINDOWS/System32/OpenSSH:/c/Program Files (x86)/PDFtk/bin:/c/Program Files/Microsoft SQL Server/Client SDK/ODBC/170/Tools/Binn:/cmd:/mingw64/bin:/usr/bin:/c/Program Files/Docker/Docker/resources/bin:/c/ProgramData/DockerDesktop/version-bin:/c/Program Files/SafeNet/Authentication/SAC/x64:/c/Program Files/SafeNet/Authentication/SAC/x32:/c/Users/r2/AppData/Local/Microsoft/WindowsApps:/c/Users/r2/AppData/Local/Pandoc:/c/Users/r2/AppData/Roaming/TinyTeX/bin/win32:/c/Program Files/Microsoft SQL Server/Client SDK/ODBC/170/Tools/Binn:/c/Users/r2/AppData/Local/Julia-1.2.0/bin:/c/Program Files (x86)/gs/gs9.50/bin'
ORIGINAL_TEMP=C:/Users/r2/AppData/Local/Temp
ORIGINAL_TMP=C:/Users/r2/AppData/Local/Temp
OS=Windows_NT
OSTYPE=msys
OneDrive='C:\Users\r2\OneDrive'
PATH='/mingw64/libexec/git-core:/c/Users/r2/bin:/mingw64/bin:/usr/local/bin:/usr/bin:/usr/bin:/mingw64/bin:/usr/bin:/c/Users/r2/bin:/c/Program Files/ImageMagick-7.0.8-Q16:/c/WINDOWS/system32:/c/WINDOWS:/c/WINDOWS/System32/Wbem:/c/WINDOWS/System32/WindowsPowerShell/v1.0:/c/WINDOWS/System32/OpenSSH:/c/Program Files (x86)/PDFtk/bin:/c/Program Files/Microsoft SQL Server/Client SDK/ODBC/170/Tools/Binn:/cmd:/mingw64/bin:/usr/bin:/c/Program Files/Docker/Docker/resources/bin:/c/ProgramData/DockerDesktop/version-bin:/c/Program Files/SafeNet/Authentication/SAC/x64:/c/Program Files/SafeNet/Authentication/SAC/x32:/c/Users/r2/AppData/Local/Microsoft/WindowsApps:/c/Users/r2/AppData/Local/Pandoc:/c/Users/r2/AppData/Roaming/TinyTeX/bin/win32:/c/Program Files/Microsoft SQL Server/Client SDK/ODBC/170/Tools/Binn:/c/Users/r2/AppData/Local/Julia-1.2.0/bin:/c/Program Files (x86)/gs/gs9.50/bin:/usr/bin/vendor_perl:/usr/bin/core_perl'
PATHEXT='.COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC'
PIPESTATUS=([0]="0")
PKG_CONFIG_PATH='C:\Program Files\Git\mingw64\lib\pkgconfig;C:\Program Files\Git\mingw64\share\pkgconfig'
PLINK_PROTOCOL=ssh
POSIXLY_CORRECT=y
PPID=1
PROCESSOR_ARCHITECTURE=AMD64
PROCESSOR_IDENTIFIER='Intel64 Family 6 Model 142 Stepping 10, GenuineIntel'
PROCESSOR_LEVEL=6
PROCESSOR_REVISION=8e0a
PROGRAMFILES='C:\Program Files'
PS4='+ '
PSModulePath='C:\Program Files\WindowsPowerShell\Modules;C:\WINDOWS\system32\WindowsPowerShell\v1.0\Modules'
PUBLIC='C:\Users\Public'
PWD=/c/Users/r2/Projects/git-issue/_57_gfw_install
ProgramData='C:\ProgramData'
ProgramW6432='C:\Program Files'
SESSIONNAME=Console
SHELL=/usr/bin/bash.exe
SHELLOPTS=braceexpand:hashall:interactive-comments:posix
SHLVL=2
SYSTEMDRIVE=C:
SYSTEMROOT='C:\WINDOWS'
TEMP=/tmp
TERM=xterm
TMP=/tmp
TMPDIR=/tmp
UID=197609
USERDOMAIN=D2SB2
USERDOMAIN_ROAMINGPROFILE=D2SB2
USERNAME=r2
USERPROFILE='C:\Users\r2'
USER_AGENT=https://github.com/dspinellis/git-issue/tree/afda065
WINDIR='C:\WINDOWS'
_=

On some cursory testing, there are a handful of envvars that are consistently in both GfW and my linux git environments. I don't know that this list is perfect.

Common variables between linux and GfW

GIT_EXEC_PATH
GIT_PREFIX
HOME
IFS
LANG
OPTIND
PATH
PPID
PS4
PWD
SHELL
SHLVL
TERM
USER_AGENT

git --version | grep -qi windows, because
```
## GfW
$ git --version
git version 2.25.0.windows.1

### ubuntu-16.04
$ git --version
git version 2.25.0
```
(though this requires another call to git, even as fleeting at this one seems).
require a custom user-set env-var to enable, such as GIT_ISSUE_GFW_WORKAROUND=1; while that requires an explicit step, it may be inconvenient for many users (who just don't know how). I don't know that this will be stable in automated use of git issue if incorporated into third-party tools.

Answer 2 · 2020-02-04T21:22:47.000Z

This explains it! CygWin avoids the problem by mounting C: as /cygdrive/c. I think that @jmtd 's pull request #63 will solve the problem very neatly.

Answer 3 · 2020-02-04T21:25:03.000Z

Our comments crossed; sorry. As I wrote in #63, let's avoid the broken guesswork and simply dynamically set MYLIB based on $PREFIX/lib.

Answer 4 · 2020-02-04T21:34:33.000Z

~~Do you mean using LIBPREFIX (currently set in the Makefile) within git-issue.sh? To make sure I don't go in the wrong direction, I think this means:~~

~~change git-issue.sh so that we can later replace LIBPREFIX;~~

~~add a line to gfw-install.sh so that we can sed -e "s|LIBPREFIX|${LIBPREFIX}|g" git-issue.sh (to a temp file and rename back)~~

Edit: ok, I think I see. Scratch this comment.

Answer 5 · 2020-02-04T21:49:15.000Z

I tested with this Makefile and it worked!

diff --git a/Makefile b/Makefile
index 8b4aaae..67d48ee 100644
--- a/Makefile
+++ b/Makefile
@@ -10,7 +10,8 @@ install:
        @mkdir -p $(DESTDIR)$(MANPREFIX)
        @mkdir -p $(DESTDIR)$(BINPREFIX)
        @mkdir -p $(DESTDIR)$(LIBPREFIX)/git-issue
-       install git-issue.sh $(DESTDIR)$(BINPREFIX)/git-issue
+       sed -e "s|/usr/local|$(PREFIX)|g" git-issue.sh > foo
+       install -m 755 foo $(DESTDIR)$(BINPREFIX)/git-issue
        install lib/git-issue/import-export.sh $(DESTDIR)$(LIBPREFIX)/git-issue/import-export.sh
        install -m 644 git-issue.1 $(DESTDIR)$(MANPREFIX)/
        @mkdir -p $(DESTDIR)$(SYSCONFDIR)/bash_completion.d
@@ -20,6 +21,11 @@ install:
 sync-docs:
        ./sync-docs.sh

+gfw-install.sh: Makefile
+       c:/Rtools/bin/make -s -n PREFIX=DOLLARHOME/AppData/Roaming/git-issue > $@
+       echo "git config --global alias.issue '!'\"DOLLARHOME/AppData/Roaming/git-issue/bin/git-issue\"" >> $@
+       sed -i -e 's/DOLLARHOME/\$$HOME/g' $@
+

So my PR (#60) will need to be contingent on #63.

Answer 6 · 2020-02-04T21:55:54.000Z

... just a thought, though ... if a user sets make PREFIX=... install, then all is fine. If a user sets make PREFIX=... LIBPREFIX=... install, then things will not be fine. I think that's fine, frankly, since LIBPREFIX is likely to be just an internal variable for the maintainer's use.

Answer 7 · 2020-02-04T22:08:36.000Z

There are specific variables that install scripts are encouraged to respect.

Answer 8 · 2020-02-04T22:22:39.000Z

Is it important to you to support these extra *PREFIX variables in gfw-install.sh? For instance, do you want GfW users to be able to do the following?

PREFIX=/quux LIBPREFIX=/quux/other/lib ./gfw-install.sh

It's not hard to do, it's just a little more care when creating the install script (based on the contents of the install: section of the Makefile).

Answer 9 · 2020-02-05T07:43:40.000Z

I think I wasn't careful when writing the installation rules, which appear to be non-standard.
I propose to start from a clean slate, using and adhering to the commonly adopted make and automake variables. The ultimate targets would be $(bindir), $(libexecdir)/git-issue and $(man1dir), all declared as specified with := to allow overriding. How does that sound? Am I missing something?

Answer 10 · 2020-02-05T20:28:59.000Z

Do you mean starting Makefile with this?

prefix     := /usr/local
bindir     := $(prefix)/bin
libexecdir := $(prefix)/libexec/git-issue
sysconfdir := $(prefix)/etc/bash_completion.d
mandir     := $(prefix)/share/man
man1dir    := $(mandir)/man1

and the gfw-install.sh rule would still be make -n prefix=$HOME/AppData/Roaming/git-issue install.

This changes the location for import-export.sh, is that okay? I suspect that this dir should be baked directly as one of the options in git-issue.sh, as we discussed in a previous comment.

It is not too unreasonable to make a new PR for just this and then add GfW as its own PR (still 60). It's also easy enough to work this in to 60 (and rename it for clarity, e.g., "posix suggestions").

Answer 11 · 2020-02-05T21:09:31.000Z

Exactly! Is this some boilerplate you found somewhere? I want to make sure we get it right this time. Doing this and baking the lib location into git-issue.sh will go a long way toward fixing the the GfW issue. A separate PR is the best thing to do.