[StGIT RFC PATCH 0/2] Fixing the rebase safeguard

[StGIT RFC PATCH 0/2] Fixing the rebase safeguard

[Yann Dirson]

This is an attempt at using a reachability test to replace the use of
orig-base, to decide when it is safe to accept a rebase.

This implementation passes the testcase posted earlier by Karl (resent
here as 2nd patch in this series), BUT fails the 4th test of t2100.
That is, it fails to deal with the case of a rewinding commit occuring
in the upstream branch, and being first git-fetch'd before being
stg-pull'd in fetch-rebase mode.  In this case, the former upstream
commit will really be lost.

In this case, however, it is exactly what we want, but I'm still
undecided about how to deal with this best.  Possibly insane ideas for
now include:

- parsing the remote.*.fetch lines to detect the leading + (just
  kidding ;)
- using ORIG_HEAD, but then, how do we decide when it is valid to do
  so ?

I'll now switch to some real-life activity to see if that helps to
find a better solution.

-- 
Yann Dirson    <ydirson@altern.org> |
Debian-related: <dirson@debian.org> |   Support Debian GNU/Linux:
                                    |  Freedom, Power, Stability, Gratis
     http://ydirson.free.fr/        | Check <http://www.debian.org/>

[PATCH 1/2] Changed rebasing safety check to look for reachability of stack base (gna bug #9181).

[Yann Dirson]

Signed-off-by: Yann Dirson <ydirson@altern.org>
---

 stgit/commands/common.py |   23 +++++++++++++++--------
 stgit/commands/pull.py   |   12 ++++--------
 stgit/commands/rebase.py |    2 +-
 stgit/git.py             |    6 ++++++
 stgit/stack.py           |    1 -
 5 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/stgit/commands/common.py b/stgit/commands/common.py
index 22c78ae..a3df2a2 100644
--- a/stgit/commands/common.py
+++ b/stgit/commands/common.py
@@ -318,13 +318,22 @@ def address_or_alias(addr_str):
                  for addr in addr_str.split(',')]
     return ', '.join([addr for addr in addr_list if addr])
 
-def prepare_rebase(real_rebase, force=None):
+def prepare_rebase(force=None):
     if not force:
-        # Be sure we won't loose results of stg-(un)commit by error.
-        # Do not require an existing orig-base for compatibility with 0.12 and earlier.
-        origbase = crt_series._get_field('orig-base')
-        if origbase and crt_series.get_base() != origbase:
-            raise CmdException, 'Rebasing would possibly lose data'
+        # Be sure we won't loose results of stg-commit by error.
+        # Note: checking for refs/bases should not be necessary with
+        # repo format version 2, but better safe than sorry.
+        branchname = crt_series.get_branch()
+        # references for anything but the current stack
+        refs = [ref for ref in git.all_refs()
+                if ref != 'refs/heads/'+branchname
+                and ref != 'refs/bases/'+branchname
+                and not re.match('^refs/patches/%s/'%branchname, ref)]
+        stray_commits = git._output_lines(['git-rev-list',
+                                           crt_series.get_base(),
+                                           '--not'] + refs)
+        if len(stray_commits) != 0:
+            raise CmdException, 'Rebasing would make the following commits below the stack base unreachable: %s' % stray_commits
 
     # pop all patches
     applied = crt_series.get_applied()
@@ -343,8 +352,6 @@ def rebase(target):
     out.done()
 
 def post_rebase(applied, nopush, merged):
-    # memorize that we rebased to here
-    crt_series._set_field('orig-base', git.get_head())
     # push the patches back
     if not nopush:
         push_patches(applied, merged)
diff --git a/stgit/commands/pull.py b/stgit/commands/pull.py
index beaa7b5..bacd5fc 100644
--- a/stgit/commands/pull.py
+++ b/stgit/commands/pull.py
@@ -77,16 +77,12 @@ def func(parser, options, args):
     check_conflicts()
     check_head_top_equal()
 
-    if policy == 'pull':
-        must_rebase = 0
-    elif policy == 'fetch-rebase':
-        must_rebase = 1
-    elif policy == 'rebase':
-        must_rebase = 1
-    else:
+    if (policy != 'pull') \
+           and (policy != 'fetch-rebase') \
+           and (policy != 'rebase'):
         raise GitConfigException, 'Unsupported pull-policy "%s"' % policy
 
-    applied = prepare_rebase(real_rebase=must_rebase, force=options.force)
+    applied = prepare_rebase(force=options.force)
 
     # pull the remote changes
     if policy == 'pull':
diff --git a/stgit/commands/rebase.py b/stgit/commands/rebase.py
index d132b60..13933d6 100644
--- a/stgit/commands/rebase.py
+++ b/stgit/commands/rebase.py
@@ -52,7 +52,7 @@ def func(parser, options, args):
     check_conflicts()
     check_head_top_equal()
 
-    applied = prepare_rebase(real_rebase=True, force=options.force)
+    applied = prepare_rebase(force=options.force)
     rebase(args[0])
     post_rebase(applied, options.nopush, options.merged)
 
diff --git a/stgit/git.py b/stgit/git.py
index 4bc41aa..bec691b 100644
--- a/stgit/git.py
+++ b/stgit/git.py
@@ -1070,3 +1070,9 @@ def fetch_head():
 
     # here we are sure to have a single fetch_head
     return fetch_head
+
+def all_refs():
+    """Return a list of all refs in the current repository.
+    """
+
+    return [line.split()[1] for line in _output_lines(['git-show-ref'])]
diff --git a/stgit/stack.py b/stgit/stack.py
index 7a06458..45fd98a 100644
--- a/stgit/stack.py
+++ b/stgit/stack.py
@@ -588,7 +588,6 @@ class Series(StgitObject):
         self.create_empty_field('applied')
         self.create_empty_field('unapplied')
         os.makedirs(self.__refs_dir)
-        self._set_field('orig-base', git.get_head())
 
         config.set(format_version_key(self.get_branch()), str(FORMAT_VERSION))
 

Re: [PATCH 1/2] Changed rebasing safety check to look for reachability of stack base (gna bug #9181).

[David Kågedal]

Yann Dirson <ydirson@altern.org> writes:

> --- a/stgit/commands/pull.py
> +++ b/stgit/commands/pull.py
> @@ -77,16 +77,12 @@ def func(parser, options, args):
>      check_conflicts()
>      check_head_top_equal()
>  
> -    if policy == 'pull':
> -        must_rebase = 0
> -    elif policy == 'fetch-rebase':
> -        must_rebase = 1
> -    elif policy == 'rebase':
> -        must_rebase = 1
> -    else:
> +    if (policy != 'pull') \
> +           and (policy != 'fetch-rebase') \
> +           and (policy != 'rebase'):
>          raise GitConfigException, 'Unsupported pull-policy "%s"' % policy

Minor nit: I think this is much more clearly written as

    if policy not in ('pull', 'fetch-rebase', 'rebase'):

-- 
David Kågedal

[PATCH 2/2] Test "stg rebase" after "stg commit"

[Yann Dirson]

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>

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

 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 RFC PATCH 0/2] Fixing the rebase safeguard

[Catalin Marinas]

On 03/06/07, Yann Dirson <ydirson@altern.org> wrote:
> This is an attempt at using a reachability test to replace the use of
> orig-base, to decide when it is safe to accept a rebase.
[...]
> - using ORIG_HEAD, but then, how do we decide when it is valid to do
>   so ?

I think using the ORIG_HEAD was the initial behaviour of StGIT. I
didn't have any problems as I am only following one tree (Linus'
mainline kernel) but other users might have different scenarios.

-- 
Catalin