Re: [PATCH v3 1/2] bundle: lost objects when removing duplicate pendings

["Đoàn Trần Công Danh" <congdanhqx@gmail.com>]

On 2021-01-07 08:50:24-0500, Jiang Xin <worldhello.net@gmail.com> wrote:
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> 
> `git rev-list` will list one commit for the following command:
> 
>     $ git rev-list 'main^!'
>     <tip-commit-of-main-branch>
> 
> But providing the same rev-list args to `git bundle`, fail to create
> a bundle file.
> 
>     $ git bundle create - 'main^!'
>     # v2 git bundle
>     -<OID> <one-line-message>
> 
>     fatal: Refusing to create empty bundle.
> 
> This is because when removing duplicate objects in function
> `object_array_remove_duplicates()`, one unique pending object which has
> the same name is deleted by mistake.  The revision arg 'main^!' in the
> above example is parsed by `handle_revision_arg()`, and at lease two
> different objects will be appended to `revs.pending`, one points to the
> parent commit of the "main" branch, and the other points to the tip
> commit of the "main" branch.  These two objects have the same name
> "main".  Only one object is left with the name "main" after calling the
> function `object_array_remove_duplicates()`.
> 
> And what's worse, when adding boundary commits into pending list, we use
> one-line commit message as names, and the arbitory names may surprise
> git-bundle.
> 
> Only comparing objects themselves (".item") is also not good enough,
> because user may want to create a bundle with two identical objects but
> with different reference names, such as: "HEAD" and "refs/heads/main".
> 
> Add new function `contains_object()` which compare both the address and
> the name of the object.
> 
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
>  object.c               |  10 +-
>  t/t6020-bundle-misc.sh | 488 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 494 insertions(+), 4 deletions(-)
>  create mode 100755 t/t6020-bundle-misc.sh
> 
> diff --git a/object.c b/object.c
> index 68f80b0b3d..98017bed8e 100644
> --- a/object.c
> +++ b/object.c
> @@ -412,15 +412,16 @@ void object_array_clear(struct object_array *array)
>  }
>  
>  /*
> - * Return true iff array already contains an entry with name.
> + * Return true if array already contains an entry.
>   */
> -static int contains_name(struct object_array *array, const char *name)
> +static int contains_object(struct object_array *array,
> +			   const struct object *item, const char *name)
>  {
>  	unsigned nr = array->nr, i;
>  	struct object_array_entry *object = array->objects;
>  
>  	for (i = 0; i < nr; i++, object++)
> -		if (!strcmp(object->name, name))
> +		if (item == object->item && !strcmp(object->name, name))

I think the comparison of `item == object->item` is a bit too fragile.
Yes, we all know `item` must be an entry of array.
However, it's separated from its usage may lead to misuse in the
future. Perhaps using `oidcmp` or adding a comment to indicate that
`item` must be an entry of `array`.

>  			return 1;
>  	return 0;
>  }
> @@ -432,7 +433,8 @@ void object_array_remove_duplicates(struct object_array *array)
>  
>  	array->nr = 0;
>  	for (src = 0; src < nr; src++) {
> -		if (!contains_name(array, objects[src].name)) {
> +		if (!contains_object(array, objects[src].item,
> +				     objects[src].name)) {
>  			if (src != array->nr)
>  				objects[array->nr] = objects[src];
>  			array->nr++;
> diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh
> new file mode 100755
> index 0000000000..d15e67c8f7
> --- /dev/null
> +++ b/t/t6020-bundle-misc.sh
> @@ -0,0 +1,488 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2021 Jiang Xin
> +#
> +
> +test_description='Test git-bundle'
> +
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +
> +. ./test-lib.sh
> +
> +test_bundle_object_count () {
> +	bundle=$1 &&
> +	pack=${bundle%.bdl}.pack &&
> +	convert_bundle_to_pack <"$bundle" >"$pack" &&
> +	git index-pack "$pack" &&
> +	git verify-pack -v "$pack" >verify.out &&
> +	count=$(grep "^$OID_REGEX " verify.out | wc -l) &&

I think we can use 'grep -c' instead of `grep .. | wc -l`.

Or

	grep "^$OID_REGEX " verify.out >verify.filtered &&
	test_line_count = $2 verify.filtered

The same comment applied to test_thin_bundle_object_count

> +	test $2 = $count && return 0
> +	echo object count for $bundle is $count, not $2
> +	return 1
> +}
> +
> +
> +test_thin_bundle_object_count () {
> +	bundle=$1 &&
> +	pack=${bundle%.bdl}.pack &&
> +	convert_bundle_to_pack <"$bundle" |
> +		test_must_fail git index-pack --stdin "$pack" &&
> +	rm -f "$pack" &&
> +	convert_bundle_to_pack <"$bundle" |
> +		git index-pack --stdin --fix-thin "$pack" &&
> +	git verify-pack -v "$pack" >verify.out &&
> +	count=$(grep "^$OID_REGEX " verify.out | wc -l) &&
> +	test $2 = $count && return 0
> +	echo object count for $bundle is $count, not $2
> +	return 1
> +}
> +
> +convert_bundle_to_pack () {
> +	while read x && test -n "$x"
> +	do
> +		:;
> +	done
> +	cat

I'm not sure what you would expect in this case,
but in my experience, I replace this whole block with

	sed '1,/^$/d'

also works.

IOW, I would apply this on top of your change:

----8<-----
diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh
index 1f60fe180e..3a428454d7 100755
--- a/t/t6020-bundle-misc.sh
+++ b/t/t6020-bundle-misc.sh
@@ -16,10 +16,8 @@ test_bundle_object_count () {
 	convert_bundle_to_pack <"$bundle" >"$pack" &&
 	git index-pack "$pack" &&
 	git verify-pack -v "$pack" >verify.out &&
-	count=$(grep "^$OID_REGEX " verify.out | wc -l) &&
-	test $2 = $count && return 0
-	echo object count for $bundle is $count, not $2
-	return 1
+	grep "^$OID_REGEX " verify.out >verify.filtered &&
+	test_line_count = $2 verify.filtered
 }
 
 
@@ -32,18 +30,12 @@ test_thin_bundle_object_count () {
 	convert_bundle_to_pack <"$bundle" |
 		git index-pack --stdin --fix-thin "$pack" &&
 	git verify-pack -v "$pack" >verify.out &&
-	count=$(grep "^$OID_REGEX " verify.out | wc -l) &&
-	test $2 = $count && return 0
-	echo object count for $bundle is $count, not $2
-	return 1
+	grep "^$OID_REGEX " verify.out >verify.filtered &&
+	test_line_count = $2 verify.filtered
 }
 
 convert_bundle_to_pack () {
-	while read x && test -n "$x"
-	do
-		:;
-	done
-	cat
+	sed '1,/^$/d'
 }
 
 # Format the output of git commands to make a user-friendly and stable
----->8-----

For the below change, I haven't checked but I think test_commit should work, no?

-- Danh

> +}
> +
> +# Format the output of git commands to make a user-friendly and stable
> +# text.  We can easily prepare the expect text without having to worry
> +# about future changes of the commit ID and spaces of the output.
> +make_user_friendly_and_stable_output () {
> +	sed \
> +		-e "s/ *\$//" \
> +		-e "s/$A/<COMMIT-A>/g" \
> +		-e "s/$B/<COMMIT-B>/g" \
> +		-e "s/$C/<COMMIT-C>/g" \
> +		-e "s/$D/<COMMIT-D>/g" \
> +		-e "s/$E/<COMMIT-E>/g" \
> +		-e "s/$F/<COMMIT-F>/g" \
> +		-e "s/$G/<COMMIT-G>/g" \
> +		-e "s/$H/<COMMIT-H>/g" \
> +		-e "s/$I/<COMMIT-I>/g" \
> +		-e "s/$J/<COMMIT-J>/g" \
> +		-e "s/$K/<COMMIT-K>/g" \
> +		-e "s/$L/<COMMIT-L>/g" \
> +		-e "s/$M/<COMMIT-M>/g" \
> +		-e "s/$N/<COMMIT-N>/g" \
> +		-e "s/$O/<COMMIT-O>/g" \
> +		-e "s/$P/<COMMIT-P>/g" \
> +		-e "s/$(echo $A | cut -c1-7)[0-9a-f]*/<OID-A>/g" \
> +		-e "s/$(echo $B | cut -c1-7)[0-9a-f]*/<OID-B>/g" \
> +		-e "s/$(echo $C | cut -c1-7)[0-9a-f]*/<OID-C>/g" \
> +		-e "s/$(echo $D | cut -c1-7)[0-9a-f]*/<OID-D>/g" \
> +		-e "s/$(echo $E | cut -c1-7)[0-9a-f]*/<OID-E>/g" \
> +		-e "s/$(echo $F | cut -c1-7)[0-9a-f]*/<OID-F>/g" \
> +		-e "s/$(echo $G | cut -c1-7)[0-9a-f]*/<OID-G>/g" \
> +		-e "s/$(echo $H | cut -c1-7)[0-9a-f]*/<OID-H>/g" \
> +		-e "s/$(echo $I | cut -c1-7)[0-9a-f]*/<OID-I>/g" \
> +		-e "s/$(echo $J | cut -c1-7)[0-9a-f]*/<OID-J>/g" \
> +		-e "s/$(echo $K | cut -c1-7)[0-9a-f]*/<OID-K>/g" \
> +		-e "s/$(echo $L | cut -c1-7)[0-9a-f]*/<OID-L>/g" \
> +		-e "s/$(echo $M | cut -c1-7)[0-9a-f]*/<OID-M>/g" \
> +		-e "s/$(echo $N | cut -c1-7)[0-9a-f]*/<OID-N>/g" \
> +		-e "s/$(echo $O | cut -c1-7)[0-9a-f]*/<OID-O>/g" \
> +		-e "s/$(echo $P | cut -c1-7)[0-9a-f]*/<OID-P>/g" \
> +		-e "s/$TAG1/<TAG-1>/g" \
> +		-e "s/$TAG2/<TAG-2>/g" \
> +		-e "s/$TAG3/<TAG-3>/g" \
> +		-e "s/$(echo $TAG1 | cut -c1-7)[0-9a-f]*/<OID-TAG-1>/g" \
> +		-e "s/$(echo $TAG2 | cut -c1-7)[0-9a-f]*/<OID-TAG-2>/g" \
> +		-e "s/$(echo $TAG3 | cut -c1-7)[0-9a-f]*/<OID-TAG-3>/g" \
> +		-e "s/$ZERO_OID/<ZERO-OID>/g"
> +}
> +
> +#            (C)   (D, pull/1/head, topic/1)
> +#             o --- o
> +#            /       \                              (L)
> +#           /         \        o (H, topic/2)             (M, tag:v2)
> +#          /    (F)    \      /                                 (N, tag:v3)
> +#         /      o --------- o (G, pull/2/head)      o --- o --- o (release)
> +#        /      /        \    \                      /       \
> +#  o --- o --- o -------- o -- o ------------------ o ------- o --- o (main)
> +# (A)   (B)  (E, tag:v1) (I)  (J)                  (K)       (O)   (P)
> +#
> +test_expect_success 'setup' '
> +	# commit A & B
> +	cat >main.txt <<-EOF &&
> +		Commit A
> +		EOF
> +	git add main.txt &&
> +	test_tick &&
> +	git commit -m "Commit A" &&
> +
> +	cat >main.txt <<-EOF &&
> +		Commit B
> +		EOF
> +	git add main.txt &&
> +	test_tick &&
> +	git commit -m "Commit B" &&
> +	A=$(git rev-parse HEAD~) &&
> +	B=$(git rev-parse HEAD) &&
> +
> +	# branch topic/1
> +	git checkout -b topic/1 &&
> +	cat >topic-1.txt <<-EOF &&
> +		Commit C
> +		EOF
> +	git add topic-1.txt &&
> +	test_tick &&
> +	git commit -m "Commit C" &&
> +
> +	cat >topic-1.txt <<-EOF &&
> +		Commit D
> +		EOF
> +	git add -u &&
> +	test_tick &&
> +	git commit -m "Commit D" &&
> +	git update-ref refs/pull/1/head HEAD &&
> +	C=$(git rev-parse topic/1~) &&
> +	D=$(git rev-parse topic/1) &&
> +
> +	# commit E
> +	git checkout main &&
> +	cat >main.txt <<-EOF &&
> +		Commit E
> +		EOF
> +	git add main.txt &&
> +	test_tick &&
> +	git commit -m "Commit E" &&
> +	E=$(git rev-parse HEAD) &&
> +	test_tick &&
> +	git tag -m "v1" v1 HEAD &&
> +	TAG1=$(git rev-parse refs/tags/v1) &&
> +
> +	# branch topic/2
> +	git checkout -b topic/2 &&
> +	cat >topic-2.txt <<-EOF &&
> +		Commit F
> +		EOF
> +	git add topic-2.txt &&
> +	test_tick &&
> +	git commit -m "Commit F" &&
> +
> +	cat >topic-2.txt <<-EOF &&
> +		Commit G
> +		EOF
> +	git add -u &&
> +	test_tick &&
> +	git commit -m "Commit G" &&
> +	git update-ref refs/pull/2/head HEAD &&
> +
> +	cat >topic-2.txt <<-EOF &&
> +		Commit H
> +		EOF
> +	git add -u &&
> +	test_tick &&
> +	git commit -m "Commit H" &&
> +	F=$(git rev-parse topic/2~2) &&
> +	G=$(git rev-parse topic/2~) &&
> +	H=$(git rev-parse topic/2) &&
> +
> +	# merge commit I & J
> +	git checkout main &&
> +	test_tick &&
> +	git merge --no-ff --no-edit topic/1 &&
> +	test_tick &&
> +	git merge --no-ff --no-edit refs/pull/2/head &&
> +	I=$(git rev-parse HEAD~) &&
> +	J=$(git rev-parse HEAD) &&
> +
> +	# commit K
> +	git checkout main &&
> +	cat >main.txt <<-EOF &&
> +		Commit K
> +		EOF
> +	git add main.txt &&
> +	test_tick &&
> +	git commit -m "Commit K" &&
> +	K=$(git rev-parse HEAD) &&
> +
> +	# branch release
> +	git checkout -b release &&
> +	cat >release.txt <<-EOF &&
> +		Commit L
> +		EOF
> +	git add release.txt &&
> +	test_tick &&
> +	git commit -m "Commit L" &&
> +
> +	cat >release.txt <<-EOF &&
> +		Commit M
> +		EOF
> +	git add -u &&
> +	test_tick &&
> +	git commit -m "Commit M" &&
> +	test_tick &&
> +	git tag -m "v2" v2 HEAD &&
> +
> +	cat >release.txt <<-EOF &&
> +		Commit N
> +		EOF
> +	git add -u &&
> +	test_tick &&
> +	git commit -m "Commit N" &&
> +	test_tick &&
> +	git tag -m "v3" v3 HEAD &&
> +	L=$(git rev-parse HEAD~2) &&
> +	M=$(git rev-parse HEAD~) &&
> +	N=$(git rev-parse HEAD) &&
> +	TAG2=$(git rev-parse refs/tags/v2) &&
> +	TAG3=$(git rev-parse refs/tags/v3) &&
> +
> +	# merge commit O
> +	git checkout main &&
> +	test_tick &&
> +	git merge --no-ff --no-edit tags/v2 &&
> +	O=$(git rev-parse HEAD) &&
> +
> +	# commit P
> +	git checkout main &&
> +	cat >main.txt <<-EOF &&
> +		Commit P
> +		EOF
> +	git add main.txt &&
> +	test_tick &&
> +	git commit -m "Commit P" &&
> +	P=$(git rev-parse HEAD)
> +'
> +
> +test_expect_success 'create bundle from special rev: main^!' '
> +	git bundle create special-rev.bdl "main^!" &&
> +
> +	git bundle list-heads special-rev.bdl |
> +		make_user_friendly_and_stable_output >actual &&
> +	cat >expect <<-EOF &&
> +		<COMMIT-P> refs/heads/main
> +		EOF
> +	test_i18ncmp expect actual &&
> +
> +	git bundle verify special-rev.bdl |
> +		make_user_friendly_and_stable_output >actual &&
> +	cat >expect <<-EOF &&
> +		The bundle contains this ref:
> +		<COMMIT-P> refs/heads/main
> +		The bundle requires this ref:
> +		<COMMIT-O>
> +		EOF
> +	test_i18ncmp expect actual &&
> +
> +	test_bundle_object_count special-rev.bdl 3
> +'
> +
> +test_expect_success 'create bundle with --max-count option' '
> +	git bundle create max-count.bdl --max-count 1 \
> +		main \
> +		"^release" \
> +		refs/tags/v1 \
> +		refs/pull/1/head \
> +		refs/pull/2/head &&
> +
> +	git bundle list-heads max-count.bdl |
> +		make_user_friendly_and_stable_output >actual &&
> +	cat >expect <<-EOF &&
> +		<COMMIT-P> refs/heads/main
> +		<TAG-1> refs/tags/v1
> +		EOF
> +	test_i18ncmp expect actual &&
> +
> +	git bundle verify max-count.bdl |
> +		make_user_friendly_and_stable_output >actual &&
> +	cat >expect <<-EOF &&
> +		The bundle contains these 2 refs:
> +		<COMMIT-P> refs/heads/main
> +		<TAG-1> refs/tags/v1
> +		The bundle requires this ref:
> +		<COMMIT-O>
> +		EOF
> +	test_i18ncmp expect actual &&
> +
> +	test_bundle_object_count max-count.bdl 4
> +'
> +
> +test_expect_success 'create bundle with --since option' '
> +	git bundle create since.bdl \
> +		--since "Thu Apr 7 15:26:13 2005 -0700" \
> +		--all &&
> +
> +	git bundle list-heads since.bdl |
> +		make_user_friendly_and_stable_output >actual &&
> +	cat >expect <<-EOF &&
> +		<COMMIT-P> refs/heads/main
> +		<COMMIT-N> refs/heads/release
> +		<TAG-2> refs/tags/v2
> +		<TAG-3> refs/tags/v3
> +		<COMMIT-P> HEAD
> +		EOF
> +	test_i18ncmp expect actual &&
> +
> +	git bundle verify since.bdl |
> +		make_user_friendly_and_stable_output >actual &&
> +	cat >expect <<-EOF &&
> +		The bundle contains these 5 refs:
> +		<COMMIT-P> refs/heads/main
> +		<COMMIT-N> refs/heads/release
> +		<TAG-2> refs/tags/v2
> +		<TAG-3> refs/tags/v3
> +		<COMMIT-P> HEAD
> +		The bundle requires these 2 refs:
> +		<COMMIT-L>
> +		<COMMIT-K>
> +		EOF
> +	test_i18ncmp expect actual &&
> +
> +	test_thin_bundle_object_count since.bdl 16
> +'
> +
> +test_expect_success 'create bundle 1 - no prerequisites' '
> +	git bundle create 1.bdl topic/1 topic/2 &&
> +
> +	cat >expect <<-EOF &&
> +		The bundle contains these 2 refs:
> +		<COMMIT-D> refs/heads/topic/1
> +		<COMMIT-H> refs/heads/topic/2
> +		The bundle records a complete history.
> +		EOF
> +
> +	# verify bundle, which has no prerequisites
> +	git bundle verify 1.bdl |
> +		make_user_friendly_and_stable_output >actual &&
> +	test_i18ncmp expect actual &&
> +
> +	test_bundle_object_count 1.bdl 24
> +'
> +
> +test_expect_success 'create bundle 2 - has prerequisites' '
> +	git bundle create 2.bdl \
> +		--ignore-missing \
> +		^topic/deleted \
> +		^$D \
> +		^topic/2 \
> +		release &&
> +
> +	cat >expect <<-EOF &&
> +		The bundle contains this ref:
> +		<COMMIT-N> refs/heads/release
> +		The bundle requires these 3 refs:
> +		<COMMIT-D>
> +		<COMMIT-E>
> +		<COMMIT-G>
> +		EOF
> +
> +	git bundle verify 2.bdl |
> +		make_user_friendly_and_stable_output >actual &&
> +	test_i18ncmp expect actual &&
> +
> +	test_bundle_object_count 2.bdl 16
> +'
> +
> +test_expect_success 'fail to verify bundle without prerequisites' '
> +	git init --bare test1.git &&
> +
> +	cat >expect <<-EOF &&
> +		error: Repository lacks these prerequisite commits:
> +		error: <COMMIT-D>
> +		error: <COMMIT-E>
> +		error: <COMMIT-G>
> +		EOF
> +
> +	test_must_fail git -C test1.git bundle verify ../2.bdl 2>&1 |
> +		make_user_friendly_and_stable_output >actual &&
> +	test_i18ncmp expect actual
> +'
> +
> +test_expect_success 'create bundle 3 - two refs, same object' '
> +	git bundle create --version=3 3.bdl \
> +		^release \
> +		^topic/1 \
> +		^topic/2 \
> +		main \
> +		HEAD &&
> +
> +	cat >expect <<-EOF &&
> +		The bundle contains these 2 refs:
> +		<COMMIT-P> refs/heads/main
> +		<COMMIT-P> HEAD
> +		The bundle requires these 2 refs:
> +		<COMMIT-M>
> +		<COMMIT-K>
> +		EOF
> +
> +	git bundle verify 3.bdl |
> +		make_user_friendly_and_stable_output >actual &&
> +	test_i18ncmp expect actual &&
> +
> +	test_bundle_object_count 3.bdl 4
> +'
> +
> +test_expect_success 'create bundle 4 - with tags' '
> +	git bundle create 4.bdl \
> +		^main \
> +		^release \
> +		^topic/1 \
> +		^topic/2 \
> +		--all &&
> +
> +	cat >expect <<-EOF &&
> +		The bundle contains these 3 refs:
> +		<TAG-1> refs/tags/v1
> +		<TAG-2> refs/tags/v2
> +		<TAG-3> refs/tags/v3
> +		The bundle records a complete history.
> +		EOF
> +
> +	git bundle verify 4.bdl |
> +		make_user_friendly_and_stable_output >actual &&
> +	test_i18ncmp expect actual &&
> +
> +	test_bundle_object_count 4.bdl 3
> +'
> +
> +test_expect_success 'clone from bundle' '
> +	git clone --mirror 1.bdl mirror.git &&
> +	git -C mirror.git show-ref |
> +		make_user_friendly_and_stable_output >actual &&
> +	cat >expect <<-EOF &&
> +		<COMMIT-D> refs/heads/topic/1
> +		<COMMIT-H> refs/heads/topic/2
> +		EOF
> +	test_cmp expect actual &&
> +
> +	git -C mirror.git fetch ../2.bdl "+refs/*:refs/*" &&
> +	git -C mirror.git show-ref |
> +		make_user_friendly_and_stable_output >actual &&
> +	cat >expect <<-EOF &&
> +		<COMMIT-N> refs/heads/release
> +		<COMMIT-D> refs/heads/topic/1
> +		<COMMIT-H> refs/heads/topic/2
> +		EOF
> +	test_cmp expect actual &&
> +
> +	git -C mirror.git fetch ../3.bdl "+refs/*:refs/*" &&
> +	git -C mirror.git show-ref |
> +		make_user_friendly_and_stable_output >actual &&
> +	cat >expect <<-EOF &&
> +		<COMMIT-P> refs/heads/main
> +		<COMMIT-N> refs/heads/release
> +		<COMMIT-D> refs/heads/topic/1
> +		<COMMIT-H> refs/heads/topic/2
> +		EOF
> +	test_cmp expect actual &&
> +
> +	git -C mirror.git fetch ../4.bdl "+refs/*:refs/*" &&
> +	git -C mirror.git show-ref |
> +		make_user_friendly_and_stable_output >actual &&
> +	cat >expect <<-EOF &&
> +		<COMMIT-P> refs/heads/main
> +		<COMMIT-N> refs/heads/release
> +		<COMMIT-D> refs/heads/topic/1
> +		<COMMIT-H> refs/heads/topic/2
> +		<TAG-1> refs/tags/v1
> +		<TAG-2> refs/tags/v2
> +		<TAG-3> refs/tags/v3
> +		EOF
> +	test_cmp expect actual
> +'
> +
> +test_done
> -- 
> 2.30.0.2.g06d2f50715
> 

-- 
Danh