-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use of IFS= breaks GfW #64
Comments
Two things that appear to work:
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 ...
|
Our comments crossed; sorry. As I wrote in #63, let's avoid the broken guesswork and simply dynamically set |
Edit: ok, I think I see. Scratch this comment. |
I tested with this 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' $@
+ |
... just a thought, though ... if a user sets |
There are specific variables that install scripts are encouraged to respect. |
Is it important to you to support these extra 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 |
I think I wasn't careful when writing the installation rules, which appear to be non-standard. |
Do you mean starting 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 This changes the location for 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"). |
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 |
This is a formalization of troubleshooting made while investigating #60.
The issue is with
IFS=
and windows' propensity to useC:
at the start of paths.For instance, consider this:
If added to
git-issue.sh
before (< 33) or after (> 51) the block of code that overridesIFS
, I seebut between lines 33 and 51 (around your
LIB_PATH
code whereIFS=":"
temporarily), I seeThe easy looping of
LIB_PATH
by usingIFS
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.)
The text was updated successfully, but these errors were encountered: