On Thu, Dec 1, 2016 at 2:42 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Austin English <austinengl...@gmail.com> 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.

GPG: 14FB D7EA A041 937B

Reply via email to