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

From: Jiang Xin <worldhello.net@gmail.com>
Date: Jan 8, 2021, 13:14 UTC
Message-ID: <CANYiYbG84GuwnU5QEg0NoMfYS0fWzfDbz-yruA2fr2yQfyfMhg@mail.gmail.com>
In-Reply-To: <X/cqrTgilKAW9P9G@danh.dev>

Đoàn Trần Công Danh <congdanhqx@gmail.com> 于2021年1月7日周四 下午11:37写道：

Show 17 quoted lines

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

You can find the same usage on comparing address of objects in other places:

+ https://github.com/git/git/blob/v2.30.0/bundle.c#L447
+ https://github.com/git/git/blob/v2.30.0/commit.c#L954

Both `item` and `object->item` point to address of a shared object in parsed_object_pool of the repository, we can compare two objects by checking address of them safely.

Show 9 quoted lines

> > +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`.

This function is borrowed from `t5510-fetch.sh`, and will change like this. Thanks.

Show 12 quoted lines

> > +
> > +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'

This function is used to convert bundle file to pack file by strip the header, which has a signature, prerequisites, references. This function is also borrowed from "t5510-fetch.sh".

Show 18 quoted lines

> For the below change, I haven't checked but I think test_commit should work, no?
> > +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) &&

I should refactor these code, but I forgot.  After examine the
`test_commit` function, I have to write a new helper, because it does
not meet my needs.
1. It should not create a tag every time.
2. The tag it created is not an annotated tag.
3. I need to store the object id to a variable.

So I write a new helper `test_commit_setvar()` in next reroll. And make this testcase much simpler.

test_expect_success 'setup' '
        # branch main: commit A & B
        test_commit_setvar A "Commit A" main.txt &&
        test_commit_setvar B "Commit B" main.txt &&

        # branch topic/1: commit C & D, refs/pull/1/head
        git checkout -b topic/1 &&
        test_commit_setvar C "Commit C" topic-1.txt &&
        test_commit_setvar D "Commit D" topic-1.txt &&
        git update-ref refs/pull/1/head HEAD &&

        # branch topic/1: commit E, tag v1
        git checkout main &&
        test_commit_setvar E "Commit E" main.txt &&
        test_commit_setvar TAG1 --tag v1 &&

        # branch topic/2: commit F & G, refs/pull/2/head
        git checkout -b topic/2 &&
        test_commit_setvar F "Commit F" topic-2.txt &&
        test_commit_setvar G "Commit G" topic-2.txt &&
        git update-ref refs/pull/2/head HEAD &&
        test_commit_setvar H "Commit H" topic-2.txt &&

        # branch main: merge commit I & J
        git checkout main &&
        test_tick &&
        test_commit_setvar I --merge topic/1 "Merge commit I" &&
        test_commit_setvar J --merge refs/pull/2/head "Merge commit J" &&

        # branch main: commit K
        git checkout main &&
        test_commit_setvar K "Commit K" main.txt &&

        # branch release:
        git checkout -b release &&
        test_commit_setvar L "Commit L" release.txt &&
        test_commit_setvar M "Commit M" release.txt &&
        test_commit_setvar TAG2 --tag v2 &&
        test_commit_setvar N "Commit N" release.txt &&
        test_commit_setvar TAG3 --tag v3 &&

        # branch main: merge commit O, commit P
        git checkout main &&
        test_commit_setvar O --merge tags/v2 "Merge commit O" &&
        test_commit_setvar P "Commit P" main.txt
'

-- Jiang Xin