[PATCH 0/5] stgit rebasing fixes

[PATCH 0/5] stgit rebasing fixes

[Yann Dirson]

The following series stores a backup of the stack-base to detect when
it is safe to rebase or not (by "stg rebase" or "stg pull" in
fetch+rebase mode).

The final patch in the series is the one implementing this, but is not
tested enough for 0.12.1.

The previous ones, however, may be good to have in 0.12.1.  Especially
the patch that adds tracking of the orig-base parameter, since that
will make things safer for more people when the final patch gets
shipped (if no orig-base is found, rebasing is currently done without
checking, to stay backwards-compatible).

-- 
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/5] Factorize rebasing behaviour.

[Yann Dirson]

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

 stgit/commands/common.py |   23 +++++++++++++++++++++++
 stgit/commands/pull.py   |   20 ++++----------------
 stgit/commands/rebase.py |   17 +++--------------
 3 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/stgit/commands/common.py b/stgit/commands/common.py
index 432edce..a352d89 100644
--- a/stgit/commands/common.py
+++ b/stgit/commands/common.py
@@ -346,3 +346,26 @@ def make_patch_name(msg, unacceptable, default_name = 'patch',
             suffix += 1
         patchname = '%s-%d' % (patchname, suffix)
     return patchname
+
+def prepare_rebase():
+    # pop all patches
+    applied = crt_series.get_applied()
+    if len(applied) > 0:
+        print 'Popping all applied patches...',
+        sys.stdout.flush()
+        crt_series.pop_patch(applied[0])
+        print 'done'
+    return applied
+
+def rebase(target):
+    if target == git.get_head():
+        print 'Already at "%s", no need for rebasing.' % target
+        return
+    
+    print 'Rebasing to "%s"...' % target
+    git.reset(tree_id = git_id(target))
+
+def post_rebase(applied, nopush, merged):
+    # push the patches back
+    if not nopush:
+        push_patches(applied, merged)
diff --git a/stgit/commands/pull.py b/stgit/commands/pull.py
index 2d4a782..83a2725 100644
--- a/stgit/commands/pull.py
+++ b/stgit/commands/pull.py
@@ -61,27 +61,15 @@ def func(parser, options, args):
     check_conflicts()
     check_head_top_equal()
 
-    # pop all patches
-    applied = crt_series.get_applied()
-    if len(applied) > 0:
-        print 'Popping all applied patches...',
-        sys.stdout.flush()
-        crt_series.pop_patch(applied[0])
-        print 'done'
+    applied = prepare_rebase()
 
     # pull the remote changes
     print 'Pulling from "%s"...' % repository
     git.fetch(repository)
     if (config.get('stgit.pull-does-rebase') == 'yes'):
-        fetch_head = git.fetch_head()
-        if fetch_head != git.get_head():
-            print 'rebasing to "%s"...' % fetch_head
-            git.reset(tree_id = fetch_head)
-    print 'done'
-
-    # push the patches back
-    if not options.nopush:
-        push_patches(applied, options.merged)
+        rebase(git.fetch_head())
+
+    post_rebase(applied, options.nopush, options.merged)
 
     # maybe tidy up
     if config.get('stgit.keepoptimized') == 'yes':
diff --git a/stgit/commands/rebase.py b/stgit/commands/rebase.py
index 2951421..63f18ec 100644
--- a/stgit/commands/rebase.py
+++ b/stgit/commands/rebase.py
@@ -49,19 +49,8 @@ def func(parser, options, args):
     check_conflicts()
     check_head_top_equal()
 
-    # pop all patches
-    applied = crt_series.get_applied()
-    if len(applied) > 0:
-        print 'Popping all applied patches...',
-        sys.stdout.flush()
-        crt_series.pop_patch(applied[0])
-        print 'done'
-
-    print 'Rebasing to "%s"...' % args[0]
-    git.reset(tree_id = git_id(args[0]))
-
-    # push the patches back
-    if not options.nopush:
-        push_patches(applied, options.merged)
+    applied = prepare_rebase()
+    rebase(args[0])
+    post_rebase(applied, options.nopush, options.merged)
 
     print_crt_patch()

[PATCH 2/5] Move stack-base querying into Series class.

[Yann Dirson]

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

 stgit/commands/common.py |    2 +-
 stgit/stack.py           |    5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/stgit/commands/common.py b/stgit/commands/common.py
index a352d89..240d003 100644
--- a/stgit/commands/common.py
+++ b/stgit/commands/common.py
@@ -101,7 +101,7 @@ def git_id(rev):
             elif patch_id == 'log':
                 return series.get_patch(patch).get_log()
         if patch == 'base' and patch_id == None:
-            return read_string(series.get_base_file())
+            return series.get_base()
     except RevParseException:
         pass
     return git.rev_parse(rev + '^{commit}')
diff --git a/stgit/stack.py b/stgit/stack.py
index 3632aa1..dc6caa6 100644
--- a/stgit/stack.py
+++ b/stgit/stack.py
@@ -394,6 +394,9 @@ class Series(StgitObject):
         self.__begin_stack_check()
         return self.__base_file
 
+    def get_base(self):
+        return read_string(self.get_base_file())
+
     def get_protected(self):
         return os.path.isfile(os.path.join(self._dir(), 'protected'))
 
@@ -609,7 +612,7 @@ class Series(StgitObject):
         """
         try:
             # allow cloning of branches not under StGIT control
-            base = read_string(self.get_base_file())
+            base = self.get_base()
         except:
             base = git.get_head()
         Series(target_series).init(create_at = base)

[PATCH 3/5] Various cleanups for clarity.

[Yann Dirson]

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

 stgit/stack.py |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/stgit/stack.py b/stgit/stack.py
index dc6caa6..3185d64 100644
--- a/stgit/stack.py
+++ b/stgit/stack.py
@@ -518,8 +518,6 @@ class Series(StgitObject):
     def init(self, create_at=False, parent_remote=None, parent_branch=None):
         """Initialises the stgit series
         """
-        bases_dir = os.path.join(self.__base_dir, 'refs', 'bases')
-
         if os.path.exists(self.__patch_dir):
             raise StackException, self.__patch_dir + ' already exists'
         if os.path.exists(self.__refs_dir):
@@ -534,7 +532,7 @@ class Series(StgitObject):
 
         self.set_parent(parent_remote, parent_branch)
         
-        create_dirs(bases_dir)
+        create_dirs(os.path.join(self.__base_dir, 'refs', 'bases'))
 
         self.create_empty_field('applied')
         self.create_empty_field('unapplied')

[PATCH 4/5] Keep track of safe base changes.

[Yann Dirson]

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

 stgit/commands/common.py |    2 ++
 stgit/stack.py           |    1 +
 2 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/stgit/commands/common.py b/stgit/commands/common.py
index 240d003..56cb517 100644
--- a/stgit/commands/common.py
+++ b/stgit/commands/common.py
@@ -366,6 +366,8 @@ def rebase(target):
     git.reset(tree_id = git_id(target))
 
 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/stack.py b/stgit/stack.py
index 3185d64..0f5d868 100644
--- a/stgit/stack.py
+++ b/stgit/stack.py
@@ -540,6 +540,7 @@ class Series(StgitObject):
         os.makedirs(os.path.join(self._dir(), 'patches'))
         os.makedirs(self.__refs_dir)
         self.__begin_stack_check()
+        self._set_field('orig-base', git.get_head())
 
     def convert(self):
         """Either convert to use a separate patch directory, or

[PATCH 5/5] Refuse to pull/rebase when rendered unsafe by (un)commits.

[Yann Dirson]

This can be overriden by a new --force flag.

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

 stgit/commands/common.py |    9 ++++++++-
 stgit/commands/pull.py   |    8 ++++++--
 stgit/commands/rebase.py |    5 ++++-
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/stgit/commands/common.py b/stgit/commands/common.py
index 56cb517..398231a 100644
--- a/stgit/commands/common.py
+++ b/stgit/commands/common.py
@@ -347,7 +347,14 @@ def make_patch_name(msg, unacceptable, default_name = 'patch',
         patchname = '%s-%d' % (patchname, suffix)
     return patchname
 
-def prepare_rebase():
+def prepare_rebase(real_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'
+
     # pop all patches
     applied = crt_series.get_applied()
     if len(applied) > 0:
diff --git a/stgit/commands/pull.py b/stgit/commands/pull.py
index 83a2725..990244e 100644
--- a/stgit/commands/pull.py
+++ b/stgit/commands/pull.py
@@ -41,6 +41,9 @@ options = [make_option('-n', '--nopush',
                        action = 'store_true'),
            make_option('-m', '--merged',
                        help = 'check for patches merged upstream',
+                       action = 'store_true'),
+           make_option('--force',
+                       help = 'force rebase even if the stack based was moved by (un)commits',
                        action = 'store_true')]
 
 def func(parser, options, args):
@@ -61,12 +64,13 @@ def func(parser, options, args):
     check_conflicts()
     check_head_top_equal()
 
-    applied = prepare_rebase()
+    must_rebase = (config.get('stgit.pull-does-rebase') == 'yes')
+    applied = prepare_rebase(real_rebase=must_rebase, force=options.force)
 
     # pull the remote changes
     print 'Pulling from "%s"...' % repository
     git.fetch(repository)
-    if (config.get('stgit.pull-does-rebase') == 'yes'):
+    if must_rebase:
         rebase(git.fetch_head())
 
     post_rebase(applied, options.nopush, options.merged)
diff --git a/stgit/commands/rebase.py b/stgit/commands/rebase.py
index 63f18ec..d132b60 100644
--- a/stgit/commands/rebase.py
+++ b/stgit/commands/rebase.py
@@ -34,6 +34,9 @@ options = [make_option('-n', '--nopush',
                        action = 'store_true'),
            make_option('-m', '--merged',
                        help = 'check for patches merged upstream',
+                       action = 'store_true'),
+           make_option('--force',
+                       help = 'force rebase even if the stack based was moved by (un)commits',
                        action = 'store_true')]
 
 def func(parser, options, args):
@@ -49,7 +52,7 @@ def func(parser, options, args):
     check_conflicts()
     check_head_top_equal()
 
-    applied = prepare_rebase()
+    applied = prepare_rebase(real_rebase=True, force=options.force)
     rebase(args[0])
     post_rebase(applied, options.nopush, options.merged)
 

stgit "push --merged" woes

[Yann Dirson]

On Wed, Feb 21, 2007 at 10:58:13PM +0000, Catalin Marinas wrote:
> >Oh, while I'm at it, "push -m" does not seem to work as expected.
> >After pulling from your tree, which contains my changes, and rebasing
> >to your head, "push -m" of the "Factorize..." patch results in a
> >conflict, while rebasing onto this particular commit and then pushing
> >my original patch gets the merge properly detected.  Sounds like a
> >candidate for a quick 0.12.2 if we cannot kill it before 0.12.1.
> 
> Unless you deleted the empty patch, you could do a push --undo and try
> to reverse apply the diff (that's what push -m does) and see whether
> it fails or not. There might be a bug in StGIT and not be able to cope
> with new files while doing the merge check (this patch has a new test
> script). If I have time tomorrow, I'll try to put this in the push
> test-case.

Just had a similar case, trying to "push -m" my Documentation patches.
The patch creating Documentation/ fails with conflict on Makefile.

Trying to reverse-apply the diff does not work, since the Makefile has
been modified by another commit.  I verified that reverse-applying the
2 patches touching that file in my stack does work.

That seems to imply that added files are not the only ones causing
problems to the current "push -m".  They just exhibit the problem more
often.

"push -m" should probably try other techniques if that one fails.
Reverse-apply on top of every newly-pulled commit that touches the
problematic files would be a possibility, but that's expensive.
Trying a meta-diff[1] between the patch to be pushed and candidate
newly-pulled commits could be cheaper.

But both techniques would miss a case that current "push -m" seems to
handle: when a patch was applied and then reversed upstream.


[1] FWIW, my (naive) stg-mdiff script:

====================
#!/bin/bash
set -e

# stg-mdiff - display meta-diffs, ie. diffs of diffs

# Main use: show evolutions of a patch.
# eg. stg-mdiff foo@stable foo
#     stg-mdiff foo 012345567ABCD # sha1 for "foo" as integrated upstream

# Copyright (c) 2007 Yann Dirson <ydirson@altern.org>
# Subject to the GNU GPL, version 2.

usage()
{
    echo "Usage: $(basename $0) <patch1> <patch2>"
    exit 1
}

if [ "$#" != 2 ]; then
    usage
fi

colordiff -u <(stg show "$1") <(stg show "$2") | less -RFX
====================