git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jiang Xin <worldhello.net@gmail.com>
Date
Jan 9, 2021, 13:32 UTC
Message-ID
<CANYiYbGcXORT-kryEngy17_J1g3FDKbty9wVXj1U5OWHu8yM8g@mail.gmail.com>
In-Reply-To
<xmqqv9c6g8r4.fsf@gitster.c.googlers.com>
Junio C Hamano <gitster@pobox.com> 于2021年1月9日周六 上午10:11写道：
Show 44 quoted lines
>
> 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.

Show 24 quoted lines
> 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.
Show 13 quoted lines
> > +     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.
Show 53 quoted lines
> > +     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.

Show 6 quoted lines
> > +                     ;;
> > +             esac
> > +             shift
> > +     done &&
>
> At this point, if $var is still empty, the caller is buggy, and ...
See the above note.
Show 42 quoted lines
> > +     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.
Show 63 quoted lines
> > +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.
>
Previous: Junio C HamanoNext: Junio C Hamano
Message 14 of 60 in “bundle: arguments can be read from stdin”
  1. bundle: arguments can be read from stdinJiang Xin, Jan 3, 2021
  2. Junio C HamanoJan 4, 2021
  3. 1/2 bundle: lost objects when removing duplicate pendingsJiang Xin, Jan 5, 2021
  4. 2/2 bundle: arguments can be read from stdinJiang Xin, Jan 5, 2021
  5. 2/2 bundle: arguments can be read from stdinJiang Xin, Jan 7, 2021
  6. 1/2 bundle: lost objects when removing duplicate pendingsJiang Xin, Jan 7, 2021
  7. Đoàn Trần Công DanhJan 7, 2021
  8. Jiang XinJan 8, 2021
  9. 0/2 Improvements for git-bundleJiang Xin, Jan 8, 2021
  10. 2/2 bundle: arguments can be read from stdinJiang Xin, Jan 8, 2021
  11. Junio C HamanoJan 9, 2021
  12. 1/2 bundle: lost objects when removing duplicate pendingsJiang Xin, Jan 8, 2021
  13. Junio C HamanoJan 9, 2021
  14. Jiang XinJan 9, 2021
  15. Junio C HamanoJan 9, 2021
  16. 0/3 improvements for git-bundleJiang Xin, Jan 10, 2021
  17. 1/3 test: add helper functions for git-bundleJiang Xin, Jan 10, 2021
  18. Junio C HamanoJan 11, 2021
  19. 0/3 improvements for git-bundleJiang Xin, Jan 12, 2021
  20. 1/3 test: add helper functions for git-bundleJiang Xin, Jan 12, 2021
  21. 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, May 26, 2021
  22. Jiang XinMay 27, 2021
  23. Ævar Arnfjörð BjarmasonMay 27, 2021
  24. Jeff KingMay 27, 2021
  25. Felipe ContrerasMay 27, 2021
  26. Jiang XinJun 1, 2021
  27. Jiang XinJun 1, 2021
  28. Ævar Arnfjörð BjarmasonJun 1, 2021
  29. Jiang XinJun 1, 2021
  30. 1/2 t6020: fix bash incompatible issueJiang Xin, Jun 1, 2021
  31. 2/2 t6020: do not mangle trailing spaces in outputJiang Xin, Jun 1, 2021
  32. Ævar Arnfjörð BjarmasonJun 5, 2021
  33. 3/4 sideband: append suffix for message whose CR in next pktlineJiang Xin, Jun 12, 2021
  34. Ævar Arnfjörð BjarmasonJun 13, 2021
  35. Junio C HamanoJun 14, 2021
  36. Jiang XinJun 14, 2021
  37. Junio C HamanoJun 15, 2021
  38. Jiang XinJun 15, 2021
  39. Nicolas PitreJun 15, 2021
  40. Jiang XinJun 15, 2021
  41. Nicolas PitreJun 15, 2021
  42. Junio C HamanoJun 15, 2021
  43. Jiang XinJun 15, 2021
  44. Nicolas PitreJun 15, 2021
  45. 0/4 Fixed t6020 bash compatible issue and fixed wrong sideband suffix issueJiang Xin, Jun 12, 2021
  46. Junio C HamanoJun 14, 2021
  47. Jiang XinJun 15, 2021
  48. t6020: fix incompatible parameter expansionJiang Xin, Jun 17, 2021
  49. Ævar Arnfjörð BjarmasonJun 21, 2021
  50. 1/4 t6020: fix bash incompatible issueJiang Xin, Jun 12, 2021
  51. 2/4 test: refactor create_commits_in() for t5411 and t5548Jiang Xin, Jun 12, 2021
  52. 4/4 test: compare raw output, not mangle tabs and spacesJiang Xin, Jun 12, 2021
  53. 3/3 bundle: arguments can be read from stdinJiang Xin, Jan 12, 2021
  54. 2/3 bundle: lost objects when removing duplicate pendingsJiang Xin, Jan 12, 2021
  55. 2/3 bundle: lost objects when removing duplicate pendingsJiang Xin, Jan 10, 2021
  56. Junio C HamanoJan 11, 2021
  57. 3/3 bundle: arguments can be read from stdinJiang Xin, Jan 10, 2021
  58. Jiang XinJan 9, 2021
  59. Junio C HamanoJan 9, 2021
  60. 0/2 improvements for git-bundleJiang Xin, Jan 7, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.