Re:Re: [PATCH v6 1/3] bundle-uri: verify oid before writing refs

>"Xing Xin via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> This commit fixes the bug by removing `REF_SKIP_OID_VERIFICATION` flag
>
>"Fix the bug by ...".
>
>> when writing bundle refs. When `refs.c:refs_update_ref` is called to to
>
>"to to"

>>  test_expect_success 'create bundle' '
>>  	git init clone-from &&
>> -	git -C clone-from checkout -b topic &&
>> -	test_commit -C clone-from A &&
>> -	test_commit -C clone-from B &&
>> -	git -C clone-from bundle create B.bundle topic
>> +	(
>> +		cd clone-from &&
>> +		git checkout -b topic &&
>> +
>> +		test_commit A &&
>> +		git bundle create A.bundle topic &&
>> +
>> +		test_commit B &&
>> +		git bundle create B.bundle topic &&
>> +
>> +		# Create a bundle with reference pointing to non-existent object.
>> +		sed "s/$(git rev-parse A)/$(git rev-parse B)/" <A.bundle >bad-header.bundle
>
>I suspect that this would be terribly unportable.  The early part of
>a bundle file may be text and sed may be able to grok but are you
>sure everybody's implementation of sed would not barf (or even
>worse, corrupt) the pack stream data that follows?
>
>The code used in t/lib-bundle.sh:convert_bundle_to_pack() has been
>in use since 8315588b (bundle: fix wrong check of read_header()'s
>return value & add tests, 2007-03-06), so munging the bundle with
>a code similar to it may have a better portability story.
>
>Add something like:
>
>        corrupt_bundle_header () {
>                sed -e '/^$/q' "$@"
>                cat
>        }
>
>to t/lib-bundle.sh, which can take an arbitrary sequence of command
>line parameters to drive "sed", and can be used like so:
>
>	corrupt_bundle_header \
>		-e "s/^$(git rev-parse A) /$(git rev-parse B) /" \
>		<A.bndl >B.bndl
>
>perhaps?

>> @@ -33,6 +42,16 @@ test_expect_success 'clone with path bundle' '
>>  	test_cmp expect actual
>>  '
>>  
>> +test_expect_success 'clone with bundle that has bad header' '
>> +	git clone --bundle-uri="clone-from/bad-header.bundle" \
>> +		clone-from clone-bad-header 2>err &&
>> +	# Write bundle ref fails, but clone can still proceed.
>> +	commit_b=$(git -C clone-from rev-parse B) &&
>> +	test_grep "trying to write ref '\''refs/bundles/topic'\'' with nonexistent object $commit_b" err &&
>> +	git -C clone-bad-header for-each-ref --format="%(refname)" >refs &&
>> +	! grep "refs/bundles/" refs
>> +'
>> +
>
>So this is the test the proposed log message discussed.  The
>description gave a false impression that the "broken header" test
>that has not much to do with the bug being fixed was the only added
>test---we probably want to correct that impression.

>> @@ -259,6 +278,132 @@ test_expect_success 'clone bundle list (file, any mode, all failures)' '
>>  	! grep "refs/bundles/" refs
>>  '
>>  
>> +#########################################################################
>> +# Clone negotiation related tests begin here
>
>Drop this divider and comment.  The HTTP tests you see below has a
>much better reason to be separated like that in order to warn test
>writers (they shouldn't add their random new tests after that point,
>because everything after that one is skipped when HTTPD tests are
>disabled---see the beginning of t/lib-httpd.sh which is included
>after that divider line), but everything here you added is not
>special.  Everybody should run these tests.

>> +test_expect_success 'negotiation: bundle with part of wanted commits' '
>> +	test_when_finished rm -rf trace*.txt &&
>
>Do not overly depend on glob not matching at this point when you
>establish the when-finished handler (or glob matching the files you
>want to catch and later test not adding anything you would want to
>clean).  Quote "rm -f trace*.txt" and drop "r" if you do not absolutely
>need it (and I would imagine you don't, given the .txt suffix).

>> +	GIT_TRACE_PACKET="$(pwd)/trace-packet.txt" \
>> +	git clone --no-local --bundle-uri="clone-from/A.bundle" \
>> +		clone-from nego-bundle-part &&
>> +	git -C nego-bundle-part for-each-ref --format="%(refname)" >refs &&
>> +	grep "refs/bundles/" refs >actual &&
>> +	cat >expect <<-\EOF &&
>> +	refs/bundles/topic
>> +	EOF
>
>Hmph, if the expected pattern is only a few lines without any magic,
>
>	test_write_lines >expect refs/bundles/topic &&
>
>may be easier to follow.

>> +	test_cmp expect actual &&
>> +	# Ensure that refs/bundles/topic are sent as "have".
>> +	grep "clone> have $(git -C clone-from rev-parse A)" trace-packet.txt
>> +'
>
>Using "test_grep" would make it easier to diagnose when test breaks.
>A failing "grep" will be silent (i.e., "I didn't find anything you
>told me to look for").  A failing "test_grep" will tell you "I was
>told to find this, but didn't find any in that".

>> +test_expect_success 'negotiation: bundle with all wanted commits' '
>> +	test_when_finished rm -rf trace*.txt &&
>> +	GIT_TRACE_PACKET="$(pwd)/trace-packet.txt" \
>> +	git clone --no-local --single-branch --branch=topic --no-tags \
>> +		--bundle-uri="clone-from/B.bundle" \
>> +		clone-from nego-bundle-all &&
>> +	git -C nego-bundle-all for-each-ref --format="%(refname)" >refs &&
>> +	grep "refs/bundles/" refs >actual &&
>> +	cat >expect <<-\EOF &&
>> +	refs/bundles/topic
>> +	EOF
>> +	test_cmp expect actual &&
>> +	# We already have all needed commits so no "want" needed.
>> +	! grep "clone> want " trace-packet.txt
>> +'
>> +
>> +test_expect_success 'negotiation: bundle list (no heuristic)' '
>> +	test_when_finished rm -f trace*.txt &&
>> +	cat >bundle-list <<-EOF &&
>> +	[bundle]
>> +		version = 1
>> +		mode = all
>> +
>> +	[bundle "bundle-1"]
>> +		uri = file://$(pwd)/clone-from/bundle-1.bundle
>> +
>> +	[bundle "bundle-2"]
>> +		uri = file://$(pwd)/clone-from/bundle-2.bundle
>> +	EOF
>
>OK.  This is a good use of here-doc (as opposed to test_write_lines
>I sugested earlier for different purposes).  I wondered if these
>$(pwd) and file://$(pwd) are safe (I always get confused by the need
>to sometimes use $PWD to help Windows), but I see them used in what
>Derrick wrote in this file, so they must be fine.
>
>But there may be characters in the leading part of $(pwd) that we do
>not control that needs quoting (like a double quote '"').  The value
>of bundle.*.uri may need to be quoted a bit carefully.  This is not
>a new problem this patch introduces, so you do not have to rewrite
>this part of the patch; I'll mark it with #leftoverbits here---the
>idea being somebody else who is too bored can come back, see if it
>is truly broken, and fix them after all dust settles.
>
>Abusing "git config -f bundle-list" might be safer, e.g.
>
>	$ git config -f bundle.list bundle.bundle-1.uri \
>		'file:///home/abc"def/t/trash dir/clone-from/b1.bndl'
>	$ cat bundle.list
>        [bundle "bundle-1"]
>                uri = file:///home/abc\"def/t/trash dir/clone-from/b1.bndl
>
>as you do not know what other garbage character is in $(pwd) part.
>
>> +	# We already have all needed commits so no "want" needed.
>> +	! grep "clone> want " trace-packet.txt
>
>Just FYI, to negate test_grep, use
>
>	test_grep ! "clone > want " trace-packet.txt
>
>not	
>
>	! test_grep "clone > want " trace-packet.txt ;# WRONG