"Xing Xin via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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"
> write the corresponding bundle refs, it triggers
> `refs.c:ref_transaction_commit`. This, in turn, invokes
> `refs.c:ref_transaction_prepare`, which calls `transaction_prepare` of
> the refs storage backend. For files backend, this function is
> `files-backend.c:files_transaction_prepare`, and for reftable backend,
> it is `reftable-backend.c:reftable_be_transaction_prepare`. Both
> functions eventually call `object.c:parse_object`, which can invoke
> `packfile.c:reprepare_packed_git` to refresh `packed_git`.
Nice. That does sound like the right fix. The one who did
something to _require_ us to reprepare causes the packed_git
list refreshed.
> 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
