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

[Junio C Hamano <gitster@pobox.com>]

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

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

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