rycus86 / githooks

Githooks: per-repo and global Git hooks with version control
MIT License
382 stars 20 forks source link

Bugfix: Quote hook alias :boom: #98

Closed gabyx closed 4 years ago

gabyx commented 4 years ago

Thx for merging

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 631


Totals Coverage Status
Change from base Build 630: 0.0%
Covered Lines: 2068
Relevant Lines: 2470

💛 - Coveralls
rycus86 commented 4 years ago

Thanks for this! Is this a problem you've seen? We have a whitespace test on Travis, I thought that should have picked it up already?

gabyx commented 4 years ago

Jeah on windows. We have these tests, but unfortunately not inside the ~ home folder... had this on windows... May be we could use

cat <<EOF | docker build --force-rm -t githooks:whitespaces-base -
FROM alpine
RUN apk add --no-cache git curl ca-certificates
RUN mkdir -p "/root/whitespace folder"
ENV HOME="/root/whitespace folder"
EOF

That seems to work and fail the whitespace tests...

rycus86 commented 4 years ago

I like your suggestion, we should try that. Travis seems to be OK with this change on Linux, so let me merge it.

gabyx commented 4 years ago

I have some more test changes ;-) in test-whitespace.sh Will open this in another PR, this will propery test this PR.

to what will ~ expand if it has whitespace inside?

gabyx commented 4 years ago

We should prohibit install into a ~ which has whitespaces. It just gives lots of troubles... or can we really handle this properly ? or we should replace every ~ with a proper $HOME ok: I see https://unix.stackexchange.com/a/151852/63957 that tilde expansion is a single word hm...