Re: [PATCH v3 2/2] merge-base: teach "--fork-point" mode
John Keeping writes: > OK - given this reasoning I agree that --fork-point makes sense. > > I think this would have been clearer if the short description said > something like: > > Find the point at which a branch forked from another branch; this > does not just look for the common ancestor of the two commits but > also takes into account the reflog of to see if the branch > forked from an earlier incarnation. That's much easier to read. Will squash the following in (I do want to make sure that it is clear that does not have to be at the tip, and also does not have to be a branch---it could be any ref). Thanks. --fork-point:: - Given a commit that is derived from possibly an earlier - incarnation of a ref, find an appropriate fork-point of the - derived history to rebase it on top of an updated base - history (see discussion on this mode below). + Find the point at which a branch (or any history that leads + to ) forked from another branch (or any reference) + . This does not just look for the common ancestor of + the two commits, but also takes into account the reflog of +to see if the history leading to forked from + an earlier incarnation of the branch (see discussion + on this mode below). OPTIONS --- -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] merge-base: teach "--fork-point" mode
On Mon, Oct 28, 2013 at 12:13:22PM -0700, Junio C Hamano wrote: > John Keeping writes: > > > The --reflog name has the advantage that it makes clear that this is > > looking at something more than the commit graph and I don't think > > --fork-point does imply that. > > I think I understand what you are saying, but that "more than the > commit graph" part in your reasoning is exactly one of the two > reasons why I do not think that it is a good idea to call it with > "reflog" in its name. The next round of update to the feature may > find a better way to find the fork point than looking at the reflog. > What the feature is meant to do, i.e. "find the fork point" can stay > the same (i.e. people can use it in their script), while the way how > the implementation achieves it (i.e. by looking at reflog) can > evolve over time, and by not hardcoding "how" in the name, the users > will benefit from the updated behaviour, without having to ask for a > better heuristics by using a different option by updating all of > their scripts. OK - given this reasoning I agree that --fork-point makes sense. I think this would have been clearer if the short description said something like: Find the point at which a branch forked from another branch; this does not just look for the common ancestor of the two commits but also takes into account the reflog of to see if the branch forked from an earlier incarnation. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] merge-base: teach "--fork-point" mode
John Keeping writes: > The --reflog name has the advantage that it makes clear that this is > looking at something more than the commit graph and I don't think > --fork-point does imply that. I think I understand what you are saying, but that "more than the commit graph" part in your reasoning is exactly one of the two reasons why I do not think that it is a good idea to call it with "reflog" in its name. The next round of update to the feature may find a better way to find the fork point than looking at the reflog. What the feature is meant to do, i.e. "find the fork point" can stay the same (i.e. people can use it in their script), while the way how the implementation achieves it (i.e. by looking at reflog) can evolve over time, and by not hardcoding "how" in the name, the users will benefit from the updated behaviour, without having to ask for a better heuristics by using a different option by updating all of their scripts. The other reason (of my two reasons) is because I do not think "finding the fork-point" will stay to be the _only_ feature that uses reflog in merge-base. When a next cool feature to compute something completely different from fork-point happens to be implemented by taking advantage of reflog data, what would we call it? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] merge-base: teach "--fork-point" mode
Martin von Zweigbergk 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] merge-base: teach "--fork-point" mode
On Fri, Oct 25, 2013 at 02:38:08PM -0700, Junio C Hamano wrote: > It also comes with a documentation update. The option is not called > --reflog but --fork-point; naming a feature after what it does > (i.e. it finds the fork point) is a lot more sensible than naming > it after how it happens to do what it does (i.e. it does so by > peeking into the reflog). I think the new name is likely to confuse normal users - when talking about a branch, you can talk about where it forked from and in that case it normally means the merge-base of that branch and master. The --reflog name has the advantage that it makes clear that this is looking at something more than the commit graph and I don't think --fork-point does imply that. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] merge-base: teach "--fork-point" mode
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 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/
> + 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.
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] merge-base: teach "--fork-point" mode
On Fri, Oct 25, 2013 at 5:38 PM, Junio C Hamano wrote: > diff --git a/Documentation/git-merge-base.txt > b/Documentation/git-merge-base.txt > index 87842e3..b383766 100644 > --- a/Documentation/git-merge-base.txt > +++ b/Documentation/git-merge-base.txt > @@ -137,6 +143,31 @@ In modern git, you can say this in a more direct way: > > instead. > > +Discussion on fork-point mode > +- > + > +After working on the `topic` branch created with `git checkout -b > +topic origin/master`, the history of remote-tracking branch > +`origin/master` may have been rewound and rebuilt, leading to a > +history of this shape: > + > +o---B1 > + / > + ---o---o---B2--o---o---o---B (origin/master) > + \ > +B3 > + \ > + Derived (topic) > + > +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 > +when `origin/master` was at B3. This mode uses the reflog of > +`origin/master` to finds B3 as the fork point, so that the `topic` s/finds/find/ > +can be rebased on top of the updated `origin/master` by: > + > +$ fork_point=$(git merge-base --fork-point origin/master topic) > +$ git rebase --onto origin/master $fork_point topic > + > > See also > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
