threads / rfc / 8410

RFC patch, 2 partsFixing the rebase safeguard

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

## tl;dr

5 messages between Jun 3, 2007 and Jun 4, 2007. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Yann Dirson· Jun 3, 2007, 13:41 UTC · lore

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/>
Yann Dirson· Jun 3, 2007, 13:41 UTC · re: Yann Dirson · lore

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

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(-)
Show changes to 5 files +26 −18

stgit/commands/common.py, stgit/commands/pull.py, stgit/commands/rebase.py, stgit/git.py, stgit/stack.py

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))
 
David Kågedal· Jun 4, 2007, 13:42 UTC · re: Yann Dirson · lore

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

Yann Dirson <ydirson@altern.org> writes:
Show 17 quoted lines
> --- 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
Yann Dirson· Jun 3, 2007, 13:41 UTC · re: Yann Dirson · lore

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

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(-)
Show changes to t/t2200-rebase.sh +16 −0
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
Catalin Marinas· Jun 4, 2007, 22:41 UTC · re: Yann Dirson · lore

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

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

← back to recent threads