[StGIT PATCH] Test "stg rebase" after "stg commit"

[StGIT PATCH] Test "stg rebase" after "stg commit"

[Karl Hasselström]

Two new tests for "stg rebase":

  1. Try to rebase to a commit that is ahead of HEAD. This should
     work, and does.

  2. Try to commit a patch, and then rebase. This doesn't work,
     because "stg rebase" aborts if orig-base != base, and "stg
     commit" doesn't update orig-base. (It does work if "stg rebase"
     is given the --force flag.)

Signed-off-by: Karl Hasselström <kha@treskal.com>
---

(2) shows a bug in "stg rebase"'s safety logic. I'm not sure how to
fix it, because I don't know how it's supposed to work in the first
place. (An obvious fix would be to update it whenever the base
changes, but that'll take some work, and I'm not convinced it can't be
done with les work. Yes, I'm lazy.) Yann, could you explain?

 t/t2200-rebase.sh |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/t/t2200-rebase.sh b/t/t2200-rebase.sh
index 52462dd..b48e513 100755
--- a/t/t2200-rebase.sh
+++ b/t/t2200-rebase.sh
@@ -30,4 +30,20 @@ test_expect_success \
 	test `stg id base@stack` = `git rev-parse master~1`
 	'
 
+test_expect_success \
+	'Rebase to next commit' \
+	'
+	stg rebase master &&
+	test $(stg id base@stack) = $(git rev-parse master)
+	'
+
+test_expect_success \
+	'Commit the patch and rebase again' \
+	'
+	stg commit &&
+	git tag committed-here &&
+	stg rebase master &&
+	test $(stg id base@stack) = $(git rev-parse master)
+	'
+
 test_done

Re: [StGIT PATCH] Test "stg rebase" after "stg commit"

[Karl Hasselström]

On 2007-05-04 10:13:21 +0200, Karl Hasselström wrote:

>   2. Try to commit a patch, and then rebase. This doesn't work,
>      because "stg rebase" aborts if orig-base != base, and "stg
>      commit" doesn't update orig-base. (It does work if "stg rebase"
>      is given the --force flag.)
>
> (2) shows a bug in "stg rebase"'s safety logic. I'm not sure how to
> fix it, because I don't know how it's supposed to work in the first
> place. (An obvious fix would be to update it whenever the base
> changes, but that'll take some work, and I'm not convinced it can't
> be done with les work. Yes, I'm lazy.) Yann, could you explain?

Another reason why this is impractical is that it's not only stgit
that's allowed to change the base. For example, doing "stg pop -a &&
git reset --hard foobar && stg rebase qwrtflsptk" will also trigger
the alarm.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: [StGIT PATCH] Test "stg rebase" after "stg commit"

[Yann Dirson]

On Sun, May 06, 2007 at 02:21:16PM +0200, Karl Hasselström wrote:
> On 2007-05-04 10:13:21 +0200, Karl Hasselström wrote:
> 
> >   2. Try to commit a patch, and then rebase. This doesn't work,
> >      because "stg rebase" aborts if orig-base != base, and "stg
> >      commit" doesn't update orig-base. (It does work if "stg rebase"
> >      is given the --force flag.)
> >
> > (2) shows a bug in "stg rebase"'s safety logic. I'm not sure how to
> > fix it, because I don't know how it's supposed to work in the first
> > place. (An obvious fix would be to update it whenever the base
> > changes, but that'll take some work, and I'm not convinced it can't
> > be done with les work. Yes, I'm lazy.) Yann, could you explain?

The idea of the safety logic is that by default stgit should not loose
a commit without the user knowing.  That is, if you used "stg commit",
the patch you committed is most probably only reachable through the
head of your stack, and rebasing may make it unreachable (unless you
have explicitely added a new ref to it, in which case you know what
you're doing and you know you can "rebase --force").

> Another reason why this is impractical is that it's not only stgit
> that's allowed to change the base. For example, doing "stg pop -a &&
> git reset --hard foobar && stg rebase qwrtflsptk" will also trigger
> the alarm.

Sure, but "pop -a && git reset" is exactly a "rebase -n".  If you use
stgit in this case instead of the plumbing, the rebase safety will not
trigger.  If you use the plumbing, you're out the scope of stgit which
cannot guess what you really want, and you should use --force because
you're supposed to know better.

But well, that's the general idea, and this safety mechanism is quite
young, it is surely possible to make it better.  But IMHO the base
principle when dealing with this is that we should make every effort
so the user does not unkwingly loose a commit (which was the case
without the safety).  OTOH, it is sure that the safety must not be
overly zealous, or we'll just end up teaching users to --force without
thinking.

That said, I have not looked at your testcase yet, I'll try to do this
soon.

Best regards,
-- 
Yann.

Re: [StGIT PATCH] Test "stg rebase" after "stg commit"

[Yann Dirson]

On Sun, May 06, 2007 at 03:15:54PM +0200, Yann Dirson wrote:
> That said, I have not looked at your testcase yet, I'll try to do this
> soon.

Oh, timeslot allocated :)

Well, this case clearly falls in the category of "actions outside
stgit that make it possible to rebase without a loss".  But then it is
also clear that the action of tagging makes the committed patch
reachable, and thus the rebase loss-less.

The safety check could be possibly be rewritten as "check if current
base is reachable without using any refs from current series".

Best regards,
-- 
Yann.

Re: [StGIT PATCH] Test "stg rebase" after "stg commit"

[Karl Hasselström]

On 2007-05-06 15:39:09 +0200, Yann Dirson wrote:

> Well, this case clearly falls in the category of "actions outside
> stgit that make it possible to rebase without a loss". But then it
> is also clear that the action of tagging makes the committed patch
> reachable, and thus the rebase loss-less.
>
> The safety check could be possibly be rewritten as "check if current
> base is reachable without using any refs from current series".

Yes, I like that idea _much_ better. That's what we _should_ be
testing for, given that the objective is to keep all commits
reachable.

So, how can we do that? gitk displays, when you view a commit, the
heads through which that commit is reachable. How does it compute
that? Hmm, it seems like this type of construct works for selecting
only those commits that are only reachable through a given ref:

  gitk origin/pu --not $(git show-ref | grep -v refs/remotes/origin/pu| cut -f 1 -d ' ')

Of course, one could use git log instead of gitk if it turns out to be
too hard to write an x-windows parser for stgit. :-)

However, I'm not sure even this is necessary; reflogs are enabled by
default nowadays. But if it's cheap enough, we might as well.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: [StGIT PATCH] Test "stg rebase" after "stg commit"

[Yann Dirson]

On Sun, May 06, 2007 at 04:22:33PM +0200, Karl Hasselström wrote:
> On 2007-05-06 15:39:09 +0200, Yann Dirson wrote:
> 
> > Well, this case clearly falls in the category of "actions outside
> > stgit that make it possible to rebase without a loss". But then it
> > is also clear that the action of tagging makes the committed patch
> > reachable, and thus the rebase loss-less.
> >
> > The safety check could be possibly be rewritten as "check if current
> > base is reachable without using any refs from current series".
> 
> Yes, I like that idea _much_ better. That's what we _should_ be
> testing for, given that the objective is to keep all commits
> reachable.
> 
> So, how can we do that? gitk displays, when you view a commit, the
> heads through which that commit is reachable. How does it compute
> that? Hmm, it seems like this type of construct works for selecting
> only those commits that are only reachable through a given ref:
> 
>   gitk origin/pu --not $(git show-ref | grep -v refs/remotes/origin/pu| cut -f 1 -d ' ')
> 
> Of course, one could use git log instead of gitk if it turns out to be
> too hard to write an x-windows parser for stgit. :-)

Indeed, gitk calls git-rev-list with those arguments, so the check can
be easily rewritten :)

Best regards,
-- 
Yann