On Thu, Dec 1, 2016 at 2:42 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Austin English <austinenglish@xxxxxxxxx> writes: > >> diff --git a/Documentation/install-webdoc.sh b/Documentation/install-webdoc.sh >> index ed8b4ff..5fb2dc5 100755 >> --- a/Documentation/install-webdoc.sh >> +++ b/Documentation/install-webdoc.sh >> @@ -18,7 +18,7 @@ do >> else >> echo >&2 "# install $h $T/$h" >> rm -f "$T/$h" >> - mkdir -p $(dirname "$T/$h") >> + mkdir -p "$(dirname "$T/$h")" >> cp "$h" "$T/$h" >> fi >> done > > We know $h is safe without quoting (see what the for loop iterates > over a list and binding each element of it to this variable), but T > is the parameter given to this script, which comes from these > > install-html: html > '$(SHELL_PATH_SQ)' ./install-webdoc.sh $(DESTDIR)$(htmldir) > > install-webdoc : html > '$(SHELL_PATH_SQ)' ./install-webdoc.sh $(WEBDOC_DEST) > > in the Makefile. So quoting the result of $(dirname "$T/$h") is > just as necessary as quoting the argument given to this dirname. > > But I do not think it is sufficient, if we are truly worried about > people who specify a path that contains IFS whitespace in DESTDIR, > WEBDOC_DEST, htmldir and other *dir variables used in the Makefile. > The references to these variables, when they are mentioned on the > command lines of Makefile actions, all need to be quoted. The > remainder of the Makefile tells me that we decided that we are not > worried about those people at all. > > So while I could take your patch as-is, I am not sure how much value > it adds to the overall callchain that would reach the location that > is updated by the patch. If you run > > make DESTDIR="/tmp/My Temporary Place" install > > it would still not do the right thing even with your patch, I would > suspect. > > Thanks. Hi Junio, Thanks for reply and reviewing. Your concerns are totally valid. Some context for the change. I wrote a wrapper script for checkbashisms/shellcheck that I use in my project. I decided to run it on other projects I have checked out, out of curiosity, and looked at some of the results. This was the only one in git, so I thought it was worth fixing. I did not test the full pipeline. I'll look again and send a follow up patch soon. -- -Austin GPG: 14FB D7EA A041 937B