Re: [PATCH v3 2/2] merge-base: teach "--fork-point" mode

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 28, 2013, 14:47 UTC
Message-ID: <xmqqwqkxh2q5.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CANiSa6h7x=CcwwAqvoaOm4-y+6MCugV5BE0OXnfGHvf+iJ66MA@mail.gmail.com>

Martin von Zweigbergk <martinvonz@gmail.com> writes:

Show 12 quoted lines

>> +       bases = get_merge_bases_many(derived, revs.nr, revs.commit, 0);
>> + ...
>> +       if (revs.nr <= i)
>> +               return 1; /* not found */
>> +
>> +       printf("%s\n", sha1_to_hex(bases->item->object.sha1));
>> +       free_commit_list(bases);
>> +       return 0;
>
> Should free_commit_list also be called in the two "return 1" cases
> above? I suppose the process will exit soon after this, but that seems
> to be true for all three cases.

You are right that the above is inconsistent. Because the code intends to be called only once in the lifetime of the program, it calls get_merge_bases_many() with cleanup set to 0, so in that sense, not freeing them anywhere may make it even more clear that this function expects to be shortly followed by a process exit.

Show 31 quoted lines

>> diff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh
>> index f80bba8..4f09db0 100755
>> --- a/t/t6010-merge-base.sh
>> +++ b/t/t6010-merge-base.sh
>> @@ -230,4 +230,31 @@ test_expect_success 'criss-cross merge-base for octopus-step' '
>>         test_cmp expected.sorted actual.sorted
>>  '
>>
>> +test_expect_success 'using reflog to find the fork point' '
>> +       git reset --hard &&
>> +       git checkout -b base $E &&
>> +
>> +       (
>> +               for count in 1 2 3 4 5
>> +               do
>> +                       git commit --allow-empty -m "Base commit #$count" &&
>> +                       git rev-parse HEAD >expect$count &&
>> +                       git checkout -B derived &&
>> +                       git commit --allow-empty -m "Derived #$count" &&
>> +                       git rev-parse HEAD >derived$count &&
>> +                       git checkout base || exit 1
>
> I think this creates a history like
>
> ---E---B1--B2--B3--B4--B5 (base)
>         \   \   \   \   \
>          D1  D2  D3  D4  D5 (derived)
>
> So I think the following test would pass even if you drop the
> --fork-point. Did you mean to create a fan-shaped history by resetting
> base to $E on every iteration above?

Just showing that I didn't think things deeply ;-) I do agree that a fan-shaped history would show what we want to do a lot better.

Thanks.

>> +                       git merge-base --fork-point base $(cat derived$count) >actual &&
>> +                       test_cmp expect$count actual || exit 1