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 j...@keeping.me.uk 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 ref 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 j...@keeping.me.uk 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 ref 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 commit does not have to be at the tip, and also ref 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 commit) forked from another branch (or any reference) + ref. This does not just look for the common ancestor of + the two commits, but also takes into account the reflog of + ref to see if the history leading to commit forked from + an earlier incarnation of the branch ref (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
Martin von Zweigbergk martinv...@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 -- 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 j...@keeping.me.uk 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
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
[PATCH v3 2/2] merge-base: teach --fork-point mode
The git pull --rebase command computes the fork point of the branch being rebased using the reflog entries of the base branch (typically a remote-tracking branch) the branch's work was based on, in order to cope with the case in which the base branch has been rewound and rebuilt. For example, if the history looked like this: o---B1 / ---o---o---B2--o---o---o---Base \ B3 \ Derived where the current tip of the base branch is at Base, but earlier fetch observed that its tip used to be B3 and then B2 and then B1 before getting to the current commit, and the branch being rebased on top of the latest base is based on commit B3, it tries to find B3 by going through the output of git rev-list --reflog base (i.e. Base, B1, B2, B3) until it finds a commit that is an ancestor of the current tip Derived. Internally, we have get_merge_bases_many() that can compute this with one-go. We would want a merge-base between Derived and a fictitious merge commit that would result by merging all the historical tips of base. When such a commit exist, we should get a single result, which exactly match one of the reflog entries of base. Teach git merge-base a new mode, --fork-point, to compute exactly that. Signed-off-by: Junio C Hamano gits...@pobox.com --- Junio C Hamano gits...@pobox.com writes: I'll send out a revamped version later today, updating not just the test but the implementation. And here is the reroll. The test is back to use exit 1 from a subshell pattern to notice a breakage in the loop. I also rolled the we can pick one more old commit from the reflog after it got expired change John noticed into the logic. 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). Documentation/git-merge-base.txt | 35 - builtin/merge-base.c | 103 +++ t/t6010-merge-base.sh| 27 ++ 3 files changed, 163 insertions(+), 2 deletions(-) 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 @@ -13,6 +13,7 @@ SYNOPSIS 'git merge-base' [-a|--all] --octopus commit... 'git merge-base' --is-ancestor commit commit 'git merge-base' --independent commit... +'git merge-base' --fork-point ref [commit] DESCRIPTION --- @@ -24,8 +25,8 @@ that does not have any better common ancestor is a 'best common ancestor', i.e. a 'merge base'. Note that there can be more than one merge base for a pair of commits. -OPERATION MODE --- +OPERATION MODES +--- As the most common special case, specifying only two commits on the command line means computing the merge base between the given two commits. @@ -56,6 +57,11 @@ from linkgit:git-show-branch[1] when used with the `--merge-base` option. and exit with status 0 if true, or with status 1 if not. Errors are signaled by a non-zero status that is not 1. +--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). OPTIONS --- @@ -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` +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 diff --git a/builtin/merge-base.c b/builtin/merge-base.c index d39c910..6a674e7 100644 --- a/builtin/merge-base.c +++ b/builtin/merge-base.c @@ -1,6 +1,9 @@ #include builtin.h #include cache.h #include commit.h +#include refs.h +#include diff.h +#include revision.h #include parse-options.h static int
Re: [PATCH v3 2/2] merge-base: teach --fork-point mode
On Fri, Oct 25, 2013 at 5:38 PM, Junio C Hamano gits...@pobox.com 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
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 gits...@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/ + 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
