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

"Xing Xin via GitGitGadget" <gitgitgadget@gmail.com> writes:

"Fix the bug by ...".

"to to"

Nice. That does sound like the right fix. The one who did something to _require_ us to reprepare causes the packed_git list refreshed.

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?

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.

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.

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).

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.

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".

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.

Just FYI, to negate test_grep, use

	test_grep ! "clone > want " trace-packet.txt

not	

	! test_grep "clone > want " trace-packet.txt ;# WRONG