use of IFS= breaks GfW

[r2evans]

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.)

[r2evans]

Two things that appear to work:

1. 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

2. 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.

[dspinellis]

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.

[dspinellis]

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

[r2evans]

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:

1. change git-issue.sh so that we can later replace LIBPREFIX;
2. 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.

[r2evans]

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.

[r2evans]

... 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.

[dspinellis]

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

[r2evans]

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).

[dspinellis]

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?

[r2evans]

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").

[dspinellis]

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.