From: Jiang Xin <worldhello.net@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Git List" <git@vger.kernel.org>,
"Đoàn Trần Công Danh" <congdanhqx@gmail.com>,
"Jiang Xin" <zhiyou.jx@alibaba-inc.com>
Subject: Re: [PATCH v4 1/2] bundle: lost objects when removing duplicate pendings
Date: Sat, 9 Jan 2021 21:32:18 +0800 [thread overview]
Message-ID: <CANYiYbGcXORT-kryEngy17_J1g3FDKbty9wVXj1U5OWHu8yM8g@mail.gmail.com> (raw)
In-Reply-To: <xmqqv9c6g8r4.fsf@gitster.c.googlers.com>
Junio C Hamano <gitster@pobox.com> 于2021年1月9日周六 上午10:11写道:
>
> Jiang Xin <worldhello.net@gmail.com> writes:
>
> > diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh
> > new file mode 100755
> > index 0000000000..c4447ca88f
> > --- /dev/null
> > +++ b/t/t6020-bundle-misc.sh
> > @@ -0,0 +1,477 @@
> > +#!/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
> > +
> > +# Check count of objects in a bundle file.
> > +# We can use "--thin" opiton to check thin pack, which must be fixed by
> > +# command `git-index-pack --fix-thin --stdin`.
> > +test_bundle_object_count () {
> > + thin= &&
> > + if test "$1" = "--thin"
> > + then
> > + thin=yes
> > + shift
> > + fi &&
> > + if test $# -ne 2
> > + then
> > + echo >&2 "args should be: <bundle> <count>"
> > + return 1
> > + fi
> > + bundle=$1 &&
> > + pack=${bundle%.bdl}.pack &&
> > + convert_bundle_to_pack <"$bundle" >"$pack" &&
> > + if test -n "$thin"
> > + then
> > + test_must_fail git index-pack "$pack" &&
>
> This is overly strict, isn't it?
I want to make sure that the created bundle file which contains a thin
pack is not changed, but now I think this check is not necessary,
testing the count of objects is enough.
> Imagine a case where the objects newer revisions introduce have *no*
> resemblance to the objects in the prerequisites' trees---the
> resulting pack will have no object that is expressed as a delta
> against anything outside the pack, and the above "index-pack" would
> succeed.
>
> Besides, "git pack-objects --thin" is *not* obligated to create a
> pack that lacks one or more objects. The "--thin" option merely
> *allows* pack-objects to omit base objects if it is convenient to do
> so.
>
> > + mv "$pack" "$pack"-thin &&
> > + cat "$pack"-thin |
> > + git index-pack --stdin --fix-thin "$pack"
>
> This side is good, but do not cat a single file into a pipe.
> The whole "then" clause would become
>
> then
> mv "$pack" "$pack-thin" &&
> git index-pack --stdin --fix-thin "$pack" <"$pack-thin"
> else
>
> I would think.
That's better.
> > + else
> > + git index-pack "$pack"
> > + fi &&
> > + git verify-pack -v "$pack" >verify.out
> > + if test $? -ne 0
> > + then
> > + echo >&2 "error: fail to convert $bundle to $pack"
> > + return 1
> > + fi
>
> At this point, we are not testing the bundle subcommand, but is
> testing "git index-pack --fix-thin" that we run ourselves. Is it
> essential to ensure $pack is sane here? I doubt it.
If not check the return error code of `git index-pack`, the report
message will be "error: object count for $bundle is , not $2". I want
to give a specific error message for developer.
> > + count=$(grep -c "^$OID_REGEX " verify.out) &&
>
> And if there is no need to run verify-pack, then we can do
> count=$(git show-index "${pack%pack}idx" | wc -l) instead, perhaps?
Will do.
> > + test $2 = $count && return 0
> > + echo >&2 "error: object count for $bundle is $count, not $2"
> > + return 1
> > +}
> > +
> > +# Display the pack data contained in the bundle file, bypassing the
> > +# header that contains the signature, prerequisites and references.
> > +convert_bundle_to_pack () {
> > + while read x && test -n "$x"
> > + do
> > + :;
> > + done
> > + cat
> > +}
>
> This looks somewhat familiar. Perhaps extract out necessary helpers
> including this one into t/test-bundle-lib or something similar in a
> preparatory step before this patch?
>
> > +# Create a commit or tag and set the variable with the object ID.
> > +test_commit_setvar () {
> > + notick= &&
> > + signoff= &&
> > + indir= &&
> > + merge= &&
> > + tag= &&
> > + var= &&
> > + while test $# != 0
> > + do
> > + case "$1" in
> > + --merge)
> > + merge=yes
> > + ;;
> > + --tag)
> > + tag=yes
> > + ;;
> > + --notick)
> > + notick=yes
> > + ;;
> > + --signoff)
> > + signoff="$1"
> > + ;;
> > + -C)
> > + indir="$2"
> > + shift
> > + ;;
> > + -*)
> > + echo >&2 "error: unknown option $1"
> > + return 1
> > + ;;
> > + *)
> > + test -n "$var" && break
> > + var=$1
The loop ends only if $var has been assigned a value, or no other
args. Will report error if no other args later.
> > + ;;
> > + esac
> > + shift
> > + done &&
>
> At this point, if $var is still empty, the caller is buggy, and ...
See the above note.
> > + indir=${indir:+"$indir"/} &&
> > + if test $# -eq 0
> > + then
> > + echo >&2 "no args provided"
> > + return 1
> > + fi &&
> > + if test -z "$notick"
> > + then
> > + test_tick
> > + fi &&
> > + if test -n "$merge"
> > + then
> > + git ${indir:+ -C "$indir"} merge --no-edit --no-ff \
> > + ${2:+-m "$2"} "$1" &&
> > + oid=$(git ${indir:+ -C "$indir"} rev-parse HEAD)
> > + elif test -n "$tag"
> > + then
> > + git ${indir:+ -C "$indir"} tag -m "$1" "$1" &&
> > + oid=$(git ${indir:+ -C "$indir"} rev-parse "$1")
> > + else
> > + file=${2:-"$1.t"} &&
> > + echo "${3-$1}" > "$indir$file" &&
> > + git ${indir:+ -C "$indir"} add "$file" &&
> > + git ${indir:+ -C "$indir"} commit $signoff -m "$1" &&
> > + oid=$(git ${indir:+ -C "$indir"} rev-parse HEAD)
> > + fi &&
> > + eval $var=$oid
> > +}
>
> ... it will cause a failure in 'eval' we have here. Not good.
>
> > +# 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.
>
> Hmph. This relies on 7 hexdigits being sufficient to uniquely
> identify all objects involved in the test? It should be OK in
> practice.
>
> Is there a point in having both <COMMIT-A> and <OID-A>? I would
> have expected that all these "full object name" conversions are
> unneeded.
Will do.
> > +make_user_friendly_and_stable_output () {
> > + sed \
> > + -e "s/$A/<COMMIT-A>/" \
> > + -e "s/$B/<COMMIT-B>/" \
> > + -e "s/$C/<COMMIT-C>/" \
> > + -e "s/$D/<COMMIT-D>/" \
> > + -e "s/$E/<COMMIT-E>/" \
> > + -e "s/$F/<COMMIT-F>/" \
> > + -e "s/$G/<COMMIT-G>/" \
> > + -e "s/$H/<COMMIT-H>/" \
> > + -e "s/$I/<COMMIT-I>/" \
> > + -e "s/$J/<COMMIT-J>/" \
> > + -e "s/$K/<COMMIT-K>/" \
> > + -e "s/$L/<COMMIT-L>/" \
> > + -e "s/$M/<COMMIT-M>/" \
> > + -e "s/$N/<COMMIT-N>/" \
> > + -e "s/$O/<COMMIT-O>/" \
> > + -e "s/$P/<COMMIT-P>/" \
> > + -e "s/$TAG1/<TAG-1>/" \
> > + -e "s/$TAG2/<TAG-2>/" \
> > + -e "s/$TAG3/<TAG-3>/" \
> > + -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/$(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/ *\$//"
> > +}
> > ...
> > +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
>
> We prefer to indent these more like so:
>
> cat >expect <<-\EOF &&
> <COMMIT-P> refs/heads/main
> EOF
>
> i.e. the indent of the line with <<EOF on it and the indent of the
> line with the matching EOF are the same. Also, quote EOF to signal
> that the body of the here text should be taken as-is without $var
> substitution.
>
next prev parent reply other threads:[~2021-01-09 13:34 UTC|newest]
Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-03 9:54 [PATCH] bundle: arguments can be read from stdin Jiang Xin
2021-01-04 23:41 ` Junio C Hamano
2021-01-05 16:30 ` [PATCH v2 1/2] bundle: lost objects when removing duplicate pendings Jiang Xin
2021-01-05 16:30 ` [PATCH v2 2/2] bundle: arguments can be read from stdin Jiang Xin
2021-01-07 13:50 ` [PATCH v3 0/2] improvements for git-bundle Jiang Xin
2021-01-07 13:50 ` [PATCH v3 1/2] bundle: lost objects when removing duplicate pendings Jiang Xin
2021-01-07 15:37 ` Đoàn Trần Công Danh
2021-01-08 13:14 ` Jiang Xin
2021-01-08 14:45 ` [PATCH v4 0/2] Improvements for git-bundle Jiang Xin
2021-01-08 14:45 ` [PATCH v4 1/2] bundle: lost objects when removing duplicate pendings Jiang Xin
2021-01-09 2:10 ` Junio C Hamano
2021-01-09 13:32 ` Jiang Xin [this message]
2021-01-09 22:02 ` Junio C Hamano
2021-01-10 14:30 ` [PATCH v5 0/3] improvements for git-bundle Jiang Xin
2021-01-10 14:30 ` [PATCH v5 1/3] test: add helper functions " Jiang Xin
2021-01-11 20:09 ` Junio C Hamano
2021-01-12 2:27 ` [PATCH v6 0/3] improvements " Jiang Xin
2021-01-12 2:27 ` [PATCH v6 1/3] test: add helper functions " Jiang Xin
2021-05-26 18:49 ` Runaway sed memory use in test on older sed+glibc (was "Re: [PATCH v6 1/3] test: add helper functions for git-bundle") Ævar Arnfjörð Bjarmason
2021-05-27 11:52 ` Jiang Xin
2021-05-27 12:19 ` Ævar Arnfjörð Bjarmason
2021-05-27 13:48 ` Jeff King
2021-05-27 19:19 ` Felipe Contreras
2021-06-01 9:45 ` Jiang Xin
2021-06-01 9:42 ` Jiang Xin
2021-06-01 11:50 ` Ævar Arnfjörð Bjarmason
2021-06-01 13:20 ` Jiang Xin
2021-06-01 14:49 ` [PATCH 1/2] t6020: fix bash incompatible issue Jiang Xin
2021-06-01 14:49 ` [PATCH 2/2] t6020: do not mangle trailing spaces in output Jiang Xin
2021-06-05 17:02 ` Ævar Arnfjörð Bjarmason
2021-06-12 5:07 ` [PATCH v2 0/4] Fixed t6020 bash compatible issue and fixed wrong sideband suffix issue Jiang Xin
2021-06-14 4:10 ` Junio C Hamano
2021-06-15 3:11 ` Jiang Xin
2021-06-17 3:14 ` [PATCH v3] t6020: fix incompatible parameter expansion Jiang Xin
2021-06-21 8:41 ` Ævar Arnfjörð Bjarmason
2021-06-12 5:07 ` [PATCH v2 1/4] t6020: fix bash incompatible issue Jiang Xin
2021-06-12 5:07 ` [PATCH v2 2/4] test: refactor create_commits_in() for t5411 and t5548 Jiang Xin
2021-06-12 5:07 ` [PATCH v2 3/4] sideband: append suffix for message whose CR in next pktline Jiang Xin
2021-06-13 7:47 ` Ævar Arnfjörð Bjarmason
2021-06-14 3:50 ` Junio C Hamano
2021-06-14 11:51 ` Jiang Xin
2021-06-15 1:17 ` Junio C Hamano
2021-06-15 1:47 ` Jiang Xin
2021-06-15 2:11 ` Nicolas Pitre
2021-06-15 3:04 ` Jiang Xin
2021-06-15 3:26 ` Nicolas Pitre
2021-06-15 4:46 ` Junio C Hamano
2021-06-15 7:17 ` Jiang Xin
2021-06-15 14:46 ` Nicolas Pitre
2021-06-12 5:07 ` [PATCH v2 4/4] test: compare raw output, not mangle tabs and spaces Jiang Xin
2021-01-12 2:27 ` [PATCH v6 2/3] bundle: lost objects when removing duplicate pendings Jiang Xin
2021-01-12 2:27 ` [PATCH v6 3/3] bundle: arguments can be read from stdin Jiang Xin
2021-01-10 14:30 ` [PATCH v5 2/3] bundle: lost objects when removing duplicate pendings Jiang Xin
2021-01-11 20:12 ` Junio C Hamano
2021-01-10 14:30 ` [PATCH v5 3/3] bundle: arguments can be read from stdin Jiang Xin
2021-01-09 15:09 ` [PATCH v4 1/2] bundle: lost objects when removing duplicate pendings Jiang Xin
2021-01-09 22:02 ` Junio C Hamano
2021-01-08 14:45 ` [PATCH v4 2/2] bundle: arguments can be read from stdin Jiang Xin
2021-01-09 2:18 ` Junio C Hamano
2021-01-07 13:50 ` [PATCH v3 " Jiang Xin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CANYiYbGcXORT-kryEngy17_J1g3FDKbty9wVXj1U5OWHu8yM8g@mail.gmail.com \
--to=worldhello.net@gmail.com \
--cc=congdanhqx@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=zhiyou.jx@alibaba-inc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).