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

From: Martin von Zweigbergk <martinvonz@gmail.com>
Date: Oct 26, 2013, 05:15 UTC
Message-ID: <CANiSa6h7x=CcwwAqvoaOm4-y+6MCugV5BE0OXnfGHvf+iJ66MA@mail.gmail.com>
In-Reply-To: <xmqq61sljakf.fsf_-_@gitster.dls.corp.google.com>

Thanks for taking care of this! Maybe John or I can finally get the changes to rebase done after this.

A few comments below. Sorry I didn't find time to review the earlier revisions.

On Fri, Oct 25, 2013 at 2:38 PM, Junio C Hamano <gitster@pobox.com> wrote:

> +
> +where `origin/master` used to point at commits B3, B2, B1 and now it
> +points at B, and your `topic` branch was stated on top of it back

s/stated/started/

Show 19 quoted lines

> +       bases = get_merge_bases_many(derived, revs.nr, revs.commit, 0);
> +
> +       /*
> +        * There should be one and only one merge base, when we found
> +        * a common ancestor among reflog entries.
> +        */
> +       if (!bases || bases->next)
> +               return 1;
> +
> +       /* And the found one must be one of the reflog entries */
> +       for (i = 0; i < revs.nr; i++)
> +               if (&bases->item->object == &revs.commit[i]->object)
> +                       break; /* found */
> +       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.

Show 21 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?

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