Re: [PATCH v6 2/7] archive --add-virtual-file: allow paths containing colons

[Junio C Hamano <gitster@pobox.com>]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

>  test_expect_success UNZIP 'git archive --format=zip --add-virtual-file' '
> +	if test_have_prereq FUNNYNAMES
> +	then
> +		PATHNAME=quoted:colon
> +	else
> +		PATHNAME=quoted
> +	fi &&
>  	git archive --format=zip >with_file_with_content.zip \
> +		--add-virtual-file=\"$PATHNAME\": \

The name is better, but this still limits what can be in PATHNAME.

Write either one of these:

		--add-virtual-file="\"$PATHNAME\":" \
		--add-virtual-file=\""$PATHNAME"\": \

to signal the intention better to future readers.  We are showing an
explicit dq-pair we want to pass to the c-unquote machinery, and we
are showing that we are not being unnecessarily loose by protecting
the string from getting word split.

Either is fine, but leaving it unquoted is not.

> +		test_path_is_file $PATHNAME &&

Ditto.  There is no reason to forbid future developers from futzing
the test to include space in the PATHNAME variable.  

IOW, I want us to be better than saying

    I know there is no $IFS whitespace now because I just wrote it.
    Because I do not think there is any need to test with a string
    with whitespace in it, I will leave the variable unquoted.
    Anybody who changes the variable and breaks this assumption have
    only themselves to blame for breaking the tests.  It is not my
    fault and it is not my problem.

which is the signal our readers would get from this patch (I would,
if I were reading this commit as a third-party), especially once
they become aware of the fact that this exact issue was already
pointed out during the review discussion.

Using double-quote appropriately sends a strong signal to reviewers
and future developers that we care about details.

A valid alternative is to write the assumption out where we
currently assign to PATHNAME.

	# The PATHNAME variable is used without quote in the code
	# below for such and such reasons, so you cannot use a $IFS
	# whitespace in it.
	if test_have_prereq FUNNYNAMES
	then
		...

If the "defensive" measure that is necessary to avoid a limitation
is too onerous, such an approach may be very much more preferrable
than preparing for future changes.  "for such and such reasons" is
a good place to justify why we avoid unnecessarily complex defensive
measure and restrict future changes in the documented way.

But in _this_ particular case, the "defensive" measure necessary is
merely just to quote the shell variables properly, which nobody
sensible would say too onerous.  I couldn't come up with anything
remotely plausible to fill "for such and such reasons" myself when I
tried to justify leaving the variables unquoted.

Regardless of the quoting issue, we probably want to comment on what
value exactly is in PATHNAME before the assignment, by the way.

E.g.

	# The PATHNAME variable holds a filename encoded like a
	# string constant in C language (e.g. "\060" is digit "0")
	if test_have_prereq FUNNYNAMES
	then
		PATHNAME=quoted:colon:\\060zero
	else
		PATHNAME=quoted\\060zero
	fi

That would not just protect only one aspect (i.e. we can pass a
colon into the resulting filename) this change but the path goes
through the c-unquoting rules.

Thanks.