Re: [RFC PATCH] add t3420-rebase-topology
Martin von Zweigbergk martinv...@gmail.com writes: For consistency, it seems like git rebase -p --root should always be a no-op, while git rebase [-i/-m] --root should be no-op if the history has no merges. Also, since git rebase -i tries to fast-forward through existing commits, it seems like git rebase -i --root should ideally not create a sentinel commit, but instead stop at the first commit marked for editing. If, OTOH, --force-rebase is given, we should rewrite history from the first commit, which in the case of --root would mean creating a sentinel commit. So, in short, I have a feeling that the sentinel commit should be created if and only if both --root and --force-rebase (but not --onto) are given. [...] So I'm getting more and more convinced that the sentinel commit should only be created if --force-rebase was given. Let me know if I'm missing something. No, that sounds fairly convincing to me. Personally, the only behaviour I want to be able to get at without --force-rebase is for rebase -i --root to allow the root commit to be dropped, edited or reworded, and commits to reordered to make a different one the root, in the same way as normal interactive rebase does for later commits. The least intrusive implementation for rebase --root was the unconditional sentinel, but as you say, you don't need it (and it's a bit surprising) if the root commit on the instruction sheet is the same as the original root: in the edit/reword case, you could just checkout the existing root and then drop out in the normal way. You only really need the sentinel to deal with the possibility of a conflict if the root is replaced with a different commit. I think I agree with you it would be better only to create the sentinel on demand when it's required or if --force is given. Cheers, Chris. -- 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: [RFC PATCH] add t3420-rebase-topology
On Thu, Sep 27, 2012 at 5:20 AM, Chris Webb ch...@arachsys.com wrote: You're right that rebase --root without --onto always creates a brand new root as a result of the implementation using a sentinel commit. Clearly this is what's wanted with --interactive That's not as clear as one might first think. git rebase -i actually honors --force; if one marks a commit for edit, but then --continue without making any changes, the next commit(s) will be fast-forwarded. See the first few lines of pick_one (or pick_one_preserving_merges). but rebase --root with neither --onto nor --interactive is a slightly odd combination for which I struggle to imagine a natural use. Perhaps you're right that for consistency it should be a no-op unless --force-rebase is given? If we did this, this combination would be a no-op unconditionally as by definition we're always descended from the root of our current commit. For consistency, it seems like git rebase -p --root should always be a no-op, while git rebase [-i/-m] --root should be no-op if the history has no merges. Also, since git rebase -i tries to fast-forward through existing commits, it seems like git rebase -i --root should ideally not create a sentinel commit, but instead stop at the first commit marked for editing. If, OTOH, --force-rebase is given, we should rewrite history from the first commit, which in the case of --root would mean creating a sentinel commit. So, in short, I have a feeling that the sentinel commit should be created if and only if both --root and --force-rebase (but not --onto) are given. However, given the not-very-useful behaviour, I suspect that rebase --root is much more likely to be a mistyped version of rebase -i --root than rebase --root --force-rebase. (Unless I'm missing a reasonable use for this? History linearisation perhaps?) Yes, the not-very-useful-ness of this is the clear argument against making them no-ops. But I have to say I was slightly surprised when I tried git rebase --root for the first time and it created completely new history for me. As you say, git rebase --root is probably often a mistyped git rebase -i --root, and if that is the case, it seems nicer (in addition to being more consistent) if we don't do anything rather than rewriting the history. The history rewriting might even go unnoticed and come as an unpleasant surprise later. When working on a new project, git rebase -i --root might even be a convenient replacement for git rebase -i initial commit even when one does not want to rewrite the initial commit itself, and in such a case, the user would clearly not want a sentinel commit either. So I'm getting more and more convinced that the sentinel commit should only be created if --force-rebase was given. Let me know if I'm missing something. Martin -- 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: [RFC PATCH] add t3420-rebase-topology
Martin von Zweigbergk martinv...@gmail.com writes: On Tue, Sep 18, 2012 at 12:53 AM, Johannes Sixt j.s...@viscovery.net wrote: Why? Is it more like --root implies --force? It doesn't currently exactly imply --force, but the effect is the same. Also see my reply to Junio's email in this thread. Maybe Chris has some thoughts on this? Hi Martin and Johannes. Sorry for the slow follow-up here. You're right that rebase --root without --onto always creates a brand new root as a result of the implementation using a sentinel commit. Clearly this is what's wanted with --interactive, but rebase --root with neither --onto nor --interactive is a slightly odd combination for which I struggle to imagine a natural use. Perhaps you're right that for consistency it should be a no-op unless --force-rebase is given? If we did this, this combination would be a no-op unconditionally as by definition we're always descended from the root of our current commit. However, given the not-very-useful behaviour, I suspect that rebase --root is much more likely to be a mistyped version of rebase -i --root than rebase --root --force-rebase. (Unless I'm missing a reasonable use for this? History linearisation perhaps?) Best wishes, Chris. -- 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: [RFC PATCH] add t3420-rebase-topology
[+Chris Webb regarding git rebase --root]
First of all, thanks for a meticulous review!
On Tue, Sep 18, 2012 at 12:53 AM, Johannes Sixt j.s...@viscovery.net wrote:
Am 9/18/2012 8:31, schrieb Martin von Zweigbergk:
Since here and in the following tests the test cases and test descriptions
vary in the same way, wouldn't it make sense to factor the description out
as well?
Definitely. I just couldn't think of a good way of doing it, so thanks
for great and concrete suggestions!
(Watch your quoting, though.)
Switched to putting the test body in double quotes as you suggested in
your examples and used single quotes for strings within the test body.
+run () {
+echo '
+ reset
+ git rebase '$@' --keep-empty p h
+ test_range p.. f g h
+'
+}
+test_expect_success 'rebase --keep-empty keeps empty even if already in
upstream' $(run)
+test_expect_failure 'rebase -m --keep-empty keeps empty even if already in
upstream' $(run -m)
+test_expect_failure 'rebase -i --keep-empty keeps empty even if already in
upstream' $(run -i)
+test_expect_failure 'rebase -p --keep-empty keeps empty even if already in
upstream' $(run -p)
is in upstream is decided by the patch text. If an empty commit is
already in upstream, this adds another one with the same or a different
commit message and authorship information. Dubious, but since it is
opt-in, it should be OK.
Yes, it is a little dubious. See
http://thread.gmane.org/gmane.comp.version-control.git/203097/focus=203159
and Junio's answer, which I think makes sense.
+run () {
+echo '
+ reset
+ git rebase '$@' j w
+ test_range j.. E n H || test_range j.. n H E
+'
Chaining tests with || is dangerous: you do not know whether the first
failed because the condition is not satisfied or because of some other
failure.
Good point. Thanks.
Why is this needed in the first place? Shouldn't the history be
deterministic, provided that the commit timestamps are all distinct?
It may be deterministic, but it's not specified, I think, so I didn't
want to depend on the order. Thinking more about it, though, I think
it's good to protect the current behavior from patches that change the
order of the parents. Although it may not be incorrect to change the
order, it would at least protect against accidental changes.
It turns out that rebase -i goes through the commits in
--topo-order, while the others use default order, I think. Which
flavor should pass the test case and which should fail (and be fixed)?
I would personally prefer to say that rebase -i is correct in using
--topo-order and that the others should be fixed. Again, it's not
specified, but I would hate to have them behave differently.
+run () {
+echo '
+ reset
+ git rebase '$@' --root c
+ ! same_revision HEAD c
+ test_range c a b c
+'
+}
+test_expect_success 'rebase --root is not a no-op' $(run)
+test_expect_success 'rebase -m --root is not a no-op' $(run -m)
+test_expect_success 'rebase -i --root is not a no-op' $(run -i)
+test_expect_success 'rebase -p --root is not a no-op' $(run -p)
Why? Is it more like --root implies --force?
It doesn't currently exactly imply --force, but the effect is the
same. Also see my reply to Junio's email in this thread.
Maybe Chris has some thoughts on this?
+run () {
+echo '
+ reset
+ git rebase '$@' --root --onto e y
+ test_range e.. x y
+'
+}
+test_expect_success 'rebase --root --onto' $(run)
+test_expect_failure 'rebase -m --root --onto' $(run -m)
+test_expect_success 'rebase -i --root --onto' $(run -i)
+test_expect_success 'rebase -p --root --onto' $(run -p)
Where does this rebase start? Ah, --root stands in for the upstream
argument, hence, y is the tip to rebase. Right? Then it makes sense.
Thanks for pointing that out. I changed the order to git rebase
--onto e --root y. I hope that makes it clearer.
+test_expect_success 'rebase -p re-creates merge from upstream' '
+ reset
+ git rebase -p k w
+ same_revision HEAD^ H
+ same_revision HEAD^2 k
+'
IMO, this tests the wrong thing. You have this history:
---j---E---k
\ \
n---H---w
where E is the second parent of w. What does it mean to rebase w onto k?
IMO, it is a meaningless operation, and the outcome is irrelevant.
It would make sense to test that this history results after the upstream
at H moved forward:
---j---E---k
\ \
n---H \
\ \
z---w'
That is, w began a topic by mergeing the sidebranch E; then upstream
advanced to z, and now you rebase the topic to the new upstream.
Fair enough. Changed accordingly.
+test_expect_success 'rebase -p re-creates internal merge' '
+ reset
+ git rebase -p c w
+ test_revisions f j n E H w HEAD~4 HEAD~3 HEAD~2 HEAD^2 HEAD^ HEAD
You must also test for c; otherwise the test would succeed if rebase did
nothing at all.
This comment applies to all
Re: [RFC PATCH] add t3420-rebase-topology
On Tue, Sep 18, 2012 at 12:51 AM, Junio C Hamano gits...@pobox.com wrote:
Martin von Zweigbergk martinv...@gmail.com writes:
do you agree
that 'rebase --onto does not re-apply patches in onto' is desirable?
This depends on how you look at --onto. Recall the most typical and
the original use case of rebase:
A'--C' your rebased work
/
---o---o---o---F---B'--o---T master
^\
v1.7.12 A---B---C your work
You could view git rebase master as a short-hand for
$ git rebase --onto $(git merge-base master HEAD) master
Exactly. I frequently consider it a short-hand for that. It might
be worth pointing out that 'git pull --rebase', which might be
one of the most frequent uses of rebase, internally often does
git rebase --onto upstream upstream@{...} branch
where upstream@{...} is the most recent upstream that is an
ancestor of branch. For example, if your work is based on
origin/pu and you send the bottom-most patch (B in the figure
below) to the maintainer and and it gets applied to
pu. Running git pull --rebase would then lead to
git rebase --onto T A. You would want this to drop B.
C' your rebased work
/
---o---o---o---F---B'--o---T origin/pu
\
A origin/pu@{1}
\
B---C your work
The intended use case for --onto, however, is primarily to replay
a history to older base, i.e. [...] a moral equivalent of
$ git checkout v1.7.12
$ git cherry-pick A B C ;# or git cherry-pick master..HEAD
Yes, this is the alternative way of looking it at and exactly why
I, too, was not sure how it should behave.
You could argue that you can compute the patch equivalence between
the commits in onto..master and commits in master..HEAD and
filter out the equivalent commits
I'm not sure if you meant master..onto rather
than onto..master. Rebase (well, all flavors of rebase
but -m) currently drops patches from master..HEAD that are
also in HEAD..master. This is what the rebase --onto does not
lose patches in upstream test is about. It is also one of the
main problems that I try to fix in my long-stalled rebase-range
series. I think we should drop patches in master..HEAD that are
also in HEAD..onto (which is almost the same
as master..onto).
The replay to an updated base case (i.e. without --onto)
Or _with_ --onto as in the above example from git pull --rebase.
On the other hand, when the user replays to an older base, she has
some idea what constitutes a series that she is replaying (i.e.
$(git merge-base master HEAD)..HEAD). It smells to go against the
user's world model if the command silently filtered commits by patch
equivalence.
If it's truly about rebasing onto an older base, there can't
possibly be any patches in HEAD..onto, so assuming you agree
with my reasoning above that those are the patches we should
drop, rebasing onto older history would be safe.
Besides, the whole point of a separate onto is to allow the user
to specify a commit that does not have a straightforward ancestry
relationship with the bottom of the series (i.e. either master or
F), and computation of patch equivalence is expected to be much
higher. Given that it is unlikely to find any match, it feels
doubly wrong to always run git cherry equivalent in that case.
Yes, this was my only concern (apart from it possibly being
conceptually wrong to do, depending on what the user meant by
issuing the command).
How about 'rebase --root is not a no-op'?
---o---o---o---F---B'--o---T master
^\
v1.7.12 A---B---C your work
If git rebase F when you are at C in the above illustration
(reproduced only the relevant parts) is a no-op (and I think it
should be), git rebase --root in the illustration below ought to
be as well, no?
F---B'--o---T master
\
A---B---C your work
Yeah, that's what I thought as well at first. I think my test
case even started out as rebase --root _is_ a no-op.
When thinking about how to handle roots in general, I often
imagine a single virtual root commit (parent of all initial
commits), and that reasoning also implies that git rebase
--root should be a no-op. Then I saw that the test case
failed (or perhaps I remembered how it is implemented with the
clever fake root/initial commit) and started thinking about why
anyone would use git rebase --root if it was a no-op. I could
only think of using it to linearize history, but that doesn't
seem like a very likely use case. So it seems like weighing
purity/correctness against usefulness to me. I'm not sure which
way to go.
Thanks for quick and detailed feedback on an RFC patch.
Martin
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More
[RFC PATCH] add t3420-rebase-topology
Add more test cases to check that the topology after a rebase is as
expected. Conflicts are not considered, but patch-equivalence is.
---
Tests pass and fail as indicated by the suffix
(_success/_failure). Your input especially appreciated on whether you
agree with the intent of the test cases. For example, do you agree
that 'rebase --onto does not re-apply patches in onto' is desirable?
And if you do, then do you also agree that 'rebase --root --onto
ignores patch in onto' is desirable? How about 'rebase --root is not a
no-op'? One might think that --force would be necessary, but on the
other hand, if that was the case, the only point (AFAICT) of git
rebase --root branch without --force would be to linearize history,
so I instead made the test case confirm that --root without --onto
effectively behaves as if --force was also passed.
Feedback on the structure/setup and style is of course also
appreciated.
t/t3420-rebase-topology.sh | 348 +
1 file changed, 348 insertions(+)
create mode 100755 t/t3420-rebase-topology.sh
diff --git a/t/t3420-rebase-topology.sh b/t/t3420-rebase-topology.sh
new file mode 100755
index 000..024a2b4
--- /dev/null
+++ b/t/t3420-rebase-topology.sh
@@ -0,0 +1,348 @@
+#!/bin/sh
+
+test_description='effect of rebase on topology'
+. ./test-lib.sh
+
+
+# q---C---r
+# /
+# a---b---c---d!--e---p
+# \
+# f---g!--h
+#\
+# j---E---k
+# \ \
+# n---H---w
+#
+# x---y---B
+#
+#
+# ! = empty
+# uppercase = cherry-picked
+# p = reverted e
+#
+# TODO:
+# prune graph to what's needed
+
+empty () {
+ git commit --allow-empty -m $1
+ git tag $1
+}
+
+cherry_pick () {
+ git cherry-pick -n $1
+ git commit -m $2
+ git tag $2
+}
+
+revert () {
+ git revert -n $1
+ git commit -m $2
+ git tag $2
+}
+
+
+test_expect_success 'setup' '
+ test_commit a
+ test_commit b
+ test_commit c
+ empty d
+ test_commit e
+ revert e p
+ git checkout b
+ test_commit f
+ empty g
+ test_commit h
+ git checkout f
+ test_commit j
+ cherry_pick e E
+ test_commit k
+ git checkout j
+ test_commit n
+ cherry_pick h H
+ git merge -m w E
+ git tag w
+ git checkout b
+ test_commit q
+ cherry_pick c C
+ test_commit r
+ git checkout --orphan disjoint
+ git rm -rf .
+ test_commit x
+ test_commit y
+ cherry_pick b B
+'
+
+reset () {
+ git rebase --abort
+ git reset --hard
+}
+
+test_range () {
+ test $(git log --reverse --topo-order --format=%s $1 | xargs) = $2
+}
+
+test_revisions () {
+ expected=$1
+ shift
+ test $(git log --format=%s --no-walk=unsorted $@ | xargs) =
$expected
+}
+
+same_revision () {
+ test $(git rev-parse $1) = $(git rev-parse $2)
+}
+
+# the following 5 (?) tests copy t3400 tests, but check the history rather
than status code and/or stdout
+run () {
+echo '
+ reset
+ git rebase '$@' c j
+ same_revision HEAD~2 c
+ test_range c.. f j
+'
+}
+test_expect_success 'simple rebase' $(run)
+test_expect_success 'simple rebase -m' $(run -m)
+test_expect_success 'simple rebase -i' $(run -i)
+test_expect_success 'simple rebase -p' $(run -p)
+
+run () {
+echo '
+ reset
+ git rebase '$@' b j
+ same_revision HEAD j
+'
+}
+test_expect_success 'rebase is no-op if upstream is an ancestor' $(run)
+test_expect_success 'rebase -m is no-op if upstream is an ancestor' $(run -m)
+test_expect_success 'rebase -i is no-op if upstream is an ancestor' $(run -i)
+test_expect_success 'rebase -p is no-op if upstream is an ancestor' $(run -p)
+
+run () {
+echo '
+ reset
+ git rebase '$@' --force b j
+ ! same_revision HEAD j
+ test_range b.. f j
+'
+}
+test_expect_success 'rebase --force' $(run)
+test_expect_success 'rebase -m --force' $(run -m)
+test_expect_success 'rebase -i --force' $(run -i)
+test_expect_failure 'rebase -p --force' $(run -p)
+
+run () {
+echo '
+ reset
+ git rebase '$@' j b
+ same_revision HEAD j
+'
+}
+test_expect_success 'rebase fast-forwards if an ancestor of upstream' $(run)
+test_expect_success 'rebase -m fast-forwards if an ancestor of upstream'
$(run -m)
+test_expect_success 'rebase -i fast-forwards if an ancestor of upstream'
$(run -i)
+test_expect_success 'rebase -p fast-forwards if an ancestor of upstream'
$(run -p)
+
+run () {
+echo '
+ reset
+ git rebase '$@' p k
+ test_range p.. f j k
+'
+}
+test_expect_success 'rebase ignores patch in upstream' $(run)
+test_expect_failure 'rebase -m ignores patch in upstream' $(run -m)
+test_expect_success 'rebase -i ignores patch in upstream' $(run -i)
+test_expect_success 'rebase -p ignores patch in upstream' $(run -p)
+
+run () {
+echo '
+ reset
+
Re: [RFC PATCH] add t3420-rebase-topology
Am 9/18/2012 8:31, schrieb Martin von Zweigbergk:
Add more test cases to check that the topology after a rebase is as
expected. Conflicts are not considered, but patch-equivalence is.
---
Tests pass and fail as indicated by the suffix
(_success/_failure). Your input especially appreciated on whether you
agree with the intent of the test cases. For example, do you agree
that 'rebase --onto does not re-apply patches in onto' is desirable?
And if you do, then do you also agree that 'rebase --root --onto
ignores patch in onto' is desirable? How about 'rebase --root is not a
no-op'? One might think that --force would be necessary, but on the
other hand, if that was the case, the only point (AFAICT) of git
rebase --root branch without --force would be to linearize history,
so I instead made the test case confirm that --root without --onto
effectively behaves as if --force was also passed.
Feedback on the structure/setup and style is of course also
appreciated.
t/t3420-rebase-topology.sh | 348
+
1 file changed, 348 insertions(+)
create mode 100755 t/t3420-rebase-topology.sh
diff --git a/t/t3420-rebase-topology.sh b/t/t3420-rebase-topology.sh
new file mode 100755
index 000..024a2b4
--- /dev/null
+++ b/t/t3420-rebase-topology.sh
@@ -0,0 +1,348 @@
+#!/bin/sh
+
+test_description='effect of rebase on topology'
+. ./test-lib.sh
+
+
+# q---C---r
+# /
+# a---b---c---d!--e---p
+# \
+# f---g!--h
+#\
+# j---E---k
+# \ \
+# n---H---w
+#
+# x---y---B
+#
+#
+# ! = empty
+# uppercase = cherry-picked
+# p = reverted e
+#
+# TODO:
+# prune graph to what's needed
+
+empty () {
Is it is_empty or make_empty/empty_commit?
+ git commit --allow-empty -m $1
+ git tag $1
+}
Obviously the latter.
+
+cherry_pick () {
+ git cherry-pick -n $1
+ git commit -m $2
+ git tag $2
+}
+
+revert () {
+ git revert -n $1
+ git commit -m $2
+ git tag $2
+}
+
+
+test_expect_success 'setup' '
+ test_commit a
+ test_commit b
+ test_commit c
+ empty d
+ test_commit e
+ revert e p
+ git checkout b
+ test_commit f
+ empty g
+ test_commit h
+ git checkout f
+ test_commit j
+ cherry_pick e E
+ test_commit k
+ git checkout j
+ test_commit n
+ cherry_pick h H
+ git merge -m w E
+ git tag w
+ git checkout b
+ test_commit q
+ cherry_pick c C
+ test_commit r
+ git checkout --orphan disjoint
+ git rm -rf .
+ test_commit x
+ test_commit y
+ cherry_pick b B
+'
+
+reset () {
+ git rebase --abort
+ git reset --hard
+}
The 'rebase --abort' can fail, but there is no chain. Good. Using this
function instead of the individual commands in the tests does not break
the chain there. Good.
These following three functions should be and can be consistent with the
order of their arguments: expected actual.
+test_range () {
+ test $(git log --reverse --topo-order --format=%s $1 | xargs) = $2
expected=$1
set -- $(git log --reverse --topo-order --format=%s $2)
test expected = $*
+}
+
+test_revisions () {
+ expected=$1
+ shift
+ test $(git log --format=%s --no-walk=unsorted $@ | xargs) =
$expected
set -- $(git log --format=%s --no-walk=unsorted $@)
test expected = $*
+}
+
+same_revision () {
+ test $(git rev-parse $1) = $(git rev-parse $2)
+}
'test_same_revision'?
+
+# the following 5 (?) tests copy t3400 tests, but check the history rather
than status code and/or stdout
+run () {
+echo '
+ reset
+ git rebase '$@' c j
+ same_revision HEAD~2 c
+ test_range c.. f j
+'
+}
+test_expect_success 'simple rebase' $(run)
+test_expect_success 'simple rebase -m' $(run -m)
+test_expect_success 'simple rebase -i' $(run -i)
+test_expect_success 'simple rebase -p' $(run -p)
Since here and in the following tests the test cases and test descriptions
vary in the same way, wouldn't it make sense to factor the description out
as well?
test_run_rebase () {
test_expect_success simple rebase $*
reset
git rebase $* c j
same_revision HEAD~2 c
test_range c.. 'f j'
}
test_run_rebase ''
test_run_rebase -m
test_run_rebase -i
test_run_rebase -p
(Watch your quoting, though.)
Oh, I see: some tests expect failure. How about:
test_run_rebase () {
result=$1
shift
test_expect_$result simple rebase $* ...
}
test_run_rebase success ''
etc.
+
+run () {
+echo '
+ reset
+ git rebase '$@' b j
+ same_revision HEAD j
+'
+}
+test_expect_success 'rebase is no-op if upstream is an ancestor' $(run)
+test_expect_success 'rebase -m is no-op if upstream is an
