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

[Adam Dinwoodie <adam@dinwoodie.org>]

On Wed, Jun 15, 2022 at 01:00:07PM -0700, Junio C Hamano wrote:
> Adam Dinwoodie <adam@dinwoodie.org> writes:
> 
> >> diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
> >> index d6027189e2..3992d08158 100755
> >> --- a/t/t5003-archive-zip.sh
> >> +++ b/t/t5003-archive-zip.sh
> >> @@ -207,13 +207,21 @@ check_zip with_untracked
> >>  check_added with_untracked untracked untracked
> >>  
> >>  test_expect_success UNZIP 'git archive --format=zip --add-virtual-file' '
> >> +	if test_have_prereq FUNNYNAMES
> >> +	then
> >> +		PATHNAME="pathname with : colon"
> >> +	else
> >> +		PATHNAME="pathname without colon"
> >> +	fi &&
> >>  	git archive --format=zip >with_file_with_content.zip \
> >> +		--add-virtual-file=\""$PATHNAME"\": \
> >>  		--add-virtual-file=hello:world $EMPTY_TREE &&
> >>  	test_when_finished "rm -rf tmp-unpack" &&
> >>  	mkdir tmp-unpack && (
> >>  		cd tmp-unpack &&
> >>  		"$GIT_UNZIP" ../with_file_with_content.zip &&
> >>  		test_path_is_file hello &&
> >> +		test_path_is_file "$PATHNAME" &&
> >>  		test world = $(cat hello)
> >>  	)
> >>  '
> >
> > This test is currently failing on Cygwin: it looks like it's exposing a
> > bug in Cygwin that means files with colons in their name aren't
> > correctly extracted from zip archives.  I'm going to report that to the
> > Cygwin mailing list, but I wanted to note it for the record here, too.
> 
> Does this mean that our code to set FUNNYNAMES prerequiste is
> slightly broken?  IOW, should we check with a path with a colon in
> it, as well as whatever we use currently for FUNNYNAMES?
> 
> Something like the attached patch?  
> 
> Or does Cygwin otherwise work perfectly well with a path with a
> colon in it, but only $GIT_UNZIP command has problem with it?  If
> that is the case, then please disregard the attached.

The latter: Cygwin works perfectly with paths containing colons, except
that Cygwin's `unzip` is seemingly buggy and doesn't work.  The file
systems Cygwin runs on don't support colons in paths, but Cygwin hides
that problem by rewriting ASCII colons to some high Unicode code point
on the filesystem, meaning Cygwin-native applications see a regular
colon, while Windows-native applications see an unusual but perfectly
valid Unicode character.

I tested the same patch to FUNNYNAMES myself before reporting, and the
test fails exactly the same way.  If we wanted to catch this, I think
we'd need a test that explicitly attempted to unzip an archive
containing a path with a colon.

(The code to set FUNNYNAMES *is* slightly broken, per the discussions
around 6d340dfaef ("t9902: split test to run on appropriate systems",
2022-04-08), and my to-do list still features tidying up and
resubmitting the patch Ævar wrote in that discussion thread.  But it
wouldn't help here because this issue is specific to Cygwin's `unzip`,
rather than a general limitation of running on Cygwin.)