[StGIT PATCH 0/4] Cleanups of the Series object.

[StGIT PATCH 0/4] Cleanups of the Series object.

[Yann Dirson]

These cleanups and under-the-hood evolutions are mostly driven by my
(still preliminar) work at allowing to work on several stacks at a
time using hydras.

-- 
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/>

[StGIT PATCH 1/4] Use get/set_name for a stack's name.

[Yann Dirson]

It is much more clear to use get_name() than get_branch(), and this is
a step towards factoring stuff into a PatchSet class.
---

 stgit/commands/branch.py |   12 +++---
 stgit/commands/export.py |    2 +
 stgit/commands/pull.py   |    2 +
 stgit/commands/series.py |    2 +
 stgit/commands/sync.py   |    2 +
 stgit/stack.py           |   88 +++++++++++++++++++++++-----------------------
 6 files changed, 54 insertions(+), 54 deletions(-)

diff --git a/stgit/commands/branch.py b/stgit/commands/branch.py
index 07bdca8..c22e143 100644
--- a/stgit/commands/branch.py
+++ b/stgit/commands/branch.py
@@ -68,7 +68,7 @@ options = [make_option('-c', '--create',
 
 
 def __is_current_branch(branch_name):
-    return crt_series.get_branch() == branch_name
+    return crt_series.get_name() == branch_name
 
 def __print_branch(branch_name, length):
     initialized = ' '
@@ -168,7 +168,7 @@ def func(parser, options, args):
     elif options.clone:
 
         if len(args) == 0:
-            clone = crt_series.get_branch() + \
+            clone = crt_series.get_name() + \
                     time.strftime('-%C%y%m%d-%H%M%S')
         elif len(args) == 1:
             clone = args[0]
@@ -215,7 +215,7 @@ def func(parser, options, args):
     elif options.protect:
 
         if len(args) == 0:
-            branch_name = crt_series.get_branch()
+            branch_name = crt_series.get_name()
         elif len(args) == 1:
             branch_name = args[0]
         else:
@@ -249,7 +249,7 @@ def func(parser, options, args):
     elif options.unprotect:
 
         if len(args) == 0:
-            branch_name = crt_series.get_branch()
+            branch_name = crt_series.get_name()
         elif len(args) == 1:
             branch_name = args[0]
         else:
@@ -269,7 +269,7 @@ def func(parser, options, args):
     elif options.description is not None:
 
         if len(args) == 0:
-            branch_name = crt_series.get_branch()
+            branch_name = crt_series.get_name()
         elif len(args) == 1:
             branch_name = args[0]
         else:
@@ -303,4 +303,4 @@ def func(parser, options, args):
     if len(args) != 0:
         parser.error('incorrect number of arguments')
 
-    print crt_series.get_branch()
+    print crt_series.get_name()
diff --git a/stgit/commands/export.py b/stgit/commands/export.py
index 35851bc..8424f9d 100644
--- a/stgit/commands/export.py
+++ b/stgit/commands/export.py
@@ -75,7 +75,7 @@ def func(parser, options, args):
     if options.dir:
         dirname = options.dir
     else:
-        dirname = 'patches-%s' % crt_series.get_branch()
+        dirname = 'patches-%s' % crt_series.get_name()
 
     if not options.branch and git.local_changes():
         out.warn('Local changes in the tree;'
diff --git a/stgit/commands/pull.py b/stgit/commands/pull.py
index beaa7b5..5f72f9b 100644
--- a/stgit/commands/pull.py
+++ b/stgit/commands/pull.py
@@ -50,7 +50,7 @@ options = [make_option('-n', '--nopush',
 def func(parser, options, args):
     """Pull the changes from a remote repository
     """
-    policy = config.get('branch.%s.stgit.pull-policy' % crt_series.get_branch()) or \
+    policy = config.get('branch.%s.stgit.pull-policy' % crt_series.get_name()) or \
              config.get('stgit.pull-policy')
 
     if policy == 'rebase':
diff --git a/stgit/commands/series.py b/stgit/commands/series.py
index fe4e468..0b3efe0 100644
--- a/stgit/commands/series.py
+++ b/stgit/commands/series.py
@@ -181,7 +181,7 @@ def func(parser, options, args):
         return
 
     if options.showbranch:
-        branch_str = '@' + crt_series.get_branch()
+        branch_str = '@' + crt_series.get_name()
     else:
         branch_str = ''
 
diff --git a/stgit/commands/sync.py b/stgit/commands/sync.py
index 8359061..e769602 100644
--- a/stgit/commands/sync.py
+++ b/stgit/commands/sync.py
@@ -85,7 +85,7 @@ def func(parser, options, args):
         # branch
         remote_series = crt_series
         stgit.commands.common.crt_series = crt_series = stack.Series()
-        if options.branch == crt_series.get_branch():
+        if options.branch == crt_series.get_name():
             raise CmdException, 'Cannot synchronise with the current branch'
         remote_patches = remote_series.get_applied()
 
diff --git a/stgit/stack.py b/stgit/stack.py
index a64aff6..0d475da 100644
--- a/stgit/stack.py
+++ b/stgit/stack.py
@@ -356,21 +356,21 @@ class Series(StgitObject):
         """
         try:
             if name:
-                self.__name = name
+                self.set_name (name)
             else:
-                self.__name = git.get_head_file()
+                self.set_name (git.get_head_file())
             self.__base_dir = basedir.get()
         except git.GitException, ex:
             raise StackException, 'GIT tree not initialised: %s' % ex
 
-        self._set_dir(os.path.join(self.__base_dir, 'patches', self.__name))
+        self._set_dir(os.path.join(self.__base_dir, 'patches', self.get_name()))
 
         # Update the branch to the latest format version if it is
         # initialized, but don't touch it if it isn't.
-        update_to_current_format_version(self.__name, self.__base_dir)
+        update_to_current_format_version(self.get_name(), self.__base_dir)
 
         self.__refs_dir = os.path.join(self.__base_dir, 'refs', 'patches',
-                                       self.__name)
+                                       self.get_name())
 
         self.__applied_file = os.path.join(self._dir(), 'applied')
         self.__unapplied_file = os.path.join(self._dir(), 'unapplied')
@@ -388,11 +388,11 @@ class Series(StgitObject):
         if not name or re.search('[^\w.-]', name):
             raise StackException, 'Invalid patch name: "%s"' % name
 
-    def get_branch(self):
-        """Return the branch name for the Series object
-        """
+    def get_name(self):
         return self.__name
-
+    def set_name(self, name):
+        self.__name = name
+        
     def get_patch(self, name):
         """Return a Patch object for the given name
         """
@@ -422,12 +422,12 @@ class Series(StgitObject):
 
     def get_applied(self):
         if not os.path.isfile(self.__applied_file):
-            raise StackException, 'Branch "%s" not initialised' % self.__name
+            raise StackException, 'Branch "%s" not initialised' % self.get_name()
         return read_strings(self.__applied_file)
 
     def get_unapplied(self):
         if not os.path.isfile(self.__unapplied_file):
-            raise StackException, 'Branch "%s" not initialised' % self.__name
+            raise StackException, 'Branch "%s" not initialised' % self.get_name()
         return read_strings(self.__unapplied_file)
 
     def get_hidden(self):
@@ -467,7 +467,7 @@ class Series(StgitObject):
             os.remove(protect_file)
 
     def __branch_descr(self):
-        return 'branch.%s.description' % self.get_branch()
+        return 'branch.%s.description' % self.get_name()
 
     def get_description(self):
         return config.get(self.__branch_descr()) or ''
@@ -479,41 +479,41 @@ class Series(StgitObject):
             config.unset(self.__branch_descr())
 
     def get_parent_remote(self):
-        value = config.get('branch.%s.remote' % self.__name)
+        value = config.get('branch.%s.remote' % self.get_name())
         if value:
             return value
         elif 'origin' in git.remotes_list():
             out.note(('No parent remote declared for stack "%s",'
-                      ' defaulting to "origin".' % self.__name),
+                      ' defaulting to "origin".' % self.get_name()),
                      ('Consider setting "branch.%s.remote" and'
                       ' "branch.%s.merge" with "git repo-config".'
-                      % (self.__name, self.__name)))
+                      % (self.get_name(), self.get_name())))
             return 'origin'
         else:
-            raise StackException, 'Cannot find a parent remote for "%s"' % self.__name
+            raise StackException, 'Cannot find a parent remote for "%s"' % self.get_name()
 
     def __set_parent_remote(self, remote):
-        value = config.set('branch.%s.remote' % self.__name, remote)
+        value = config.set('branch.%s.remote' % self.get_name(), remote)
 
     def get_parent_branch(self):
-        value = config.get('branch.%s.stgit.parentbranch' % self.__name)
+        value = config.get('branch.%s.stgit.parentbranch' % self.get_name())
         if value:
             return value
         elif git.rev_parse('heads/origin'):
             out.note(('No parent branch declared for stack "%s",'
-                      ' defaulting to "heads/origin".' % self.__name),
+                      ' defaulting to "heads/origin".' % self.get_name()),
                      ('Consider setting "branch.%s.stgit.parentbranch"'
-                      ' with "git repo-config".' % self.__name))
+                      ' with "git repo-config".' % self.get_name()))
             return 'heads/origin'
         else:
-            raise StackException, 'Cannot find a parent branch for "%s"' % self.__name
+            raise StackException, 'Cannot find a parent branch for "%s"' % self.get_name()
 
     def __set_parent_branch(self, name):
-        if config.get('branch.%s.remote' % self.__name):
+        if config.get('branch.%s.remote' % self.get_name()):
             # Never set merge if remote is not set to avoid
             # possibly-erroneous lookups into 'origin'
-            config.set('branch.%s.merge' % self.__name, name)
-        config.set('branch.%s.stgit.parentbranch' % self.__name, name)
+            config.set('branch.%s.merge' % self.get_name(), name)
+        config.set('branch.%s.stgit.parentbranch' % self.get_name(), name)
 
     def set_parent(self, remote, localbranch):
         if localbranch:
@@ -521,7 +521,7 @@ class Series(StgitObject):
             self.__set_parent_branch(localbranch)
         # We'll enforce this later
 #         else:
-#             raise StackException, 'Parent branch (%s) should be specified for %s' % localbranch, self.__name
+#             raise StackException, 'Parent branch (%s) should be specified for %s' % localbranch, self.get_name()
 
     def __patch_is_current(self, patch):
         return patch.get_name() == self.get_current()
@@ -559,19 +559,19 @@ class Series(StgitObject):
     def is_initialised(self):
         """Checks if series is already initialised
         """
-        return bool(config.get(format_version_key(self.get_branch())))
+        return bool(config.get(format_version_key(self.get_name())))
 
     def init(self, create_at=False, parent_remote=None, parent_branch=None):
         """Initialises the stgit series
         """
         if self.is_initialised():
-            raise StackException, '%s already initialized' % self.get_branch()
+            raise StackException, '%s already initialized' % self.get_name()
         for d in [self._dir(), self.__refs_dir]:
             if os.path.exists(d):
                 raise StackException, '%s already exists' % d
 
         if (create_at!=False):
-            git.create_branch(self.__name, create_at)
+            git.create_branch(self.get_name(), create_at)
 
         os.makedirs(self.__patch_dir)
 
@@ -582,7 +582,7 @@ class Series(StgitObject):
         os.makedirs(self.__refs_dir)
         self._set_field('orig-base', git.get_head())
 
-        config.set(format_version_key(self.get_branch()), str(FORMAT_VERSION))
+        config.set(format_version_key(self.get_name()), str(FORMAT_VERSION))
 
     def rename(self, to_name):
         """Renames a series
@@ -590,19 +590,19 @@ class Series(StgitObject):
         to_stack = Series(to_name)
 
         if to_stack.is_initialised():
-            raise StackException, '"%s" already exists' % to_stack.get_branch()
+            raise StackException, '"%s" already exists' % to_stack.get_name()
 
-        git.rename_branch(self.__name, to_name)
+        git.rename_branch(self.get_name(), to_name)
 
         if os.path.isdir(self._dir()):
             rename(os.path.join(self.__base_dir, 'patches'),
-                   self.__name, to_stack.__name)
+                   self.get_name(), to_stack.get_name())
         if os.path.exists(self.__refs_dir):
             rename(os.path.join(self.__base_dir, 'refs', 'patches'),
-                   self.__name, to_stack.__name)
+                   self.get_name(), to_stack.get_name())
 
         # Rename the config section
-        config.rename_section("branch.%s" % self.__name,
+        config.rename_section("branch.%s" % self.get_name(),
                               "branch.%s" % to_name)
 
         self.__init__(to_name)
@@ -619,7 +619,7 @@ class Series(StgitObject):
         new_series = Series(target_series)
 
         # generate an artificial description file
-        new_series.set_description('clone of "%s"' % self.__name)
+        new_series.set_description('clone of "%s"' % self.get_name())
 
         # clone self's entire series as unapplied patches
         try:
@@ -649,15 +649,15 @@ class Series(StgitObject):
         new_series.forward_patches(applied)
 
         # Clone parent informations
-        value = config.get('branch.%s.remote' % self.__name)
+        value = config.get('branch.%s.remote' % self.get_name())
         if value:
             config.set('branch.%s.remote' % target_series, value)
 
-        value = config.get('branch.%s.merge' % self.__name)
+        value = config.get('branch.%s.merge' % self.get_name())
         if value:
             config.set('branch.%s.merge' % target_series, value)
 
-        value = config.get('branch.%s.stgit.parentbranch' % self.__name)
+        value = config.get('branch.%s.stgit.parentbranch' % self.get_name())
         if value:
             config.set('branch.%s.stgit.parentbranch' % target_series, value)
 
@@ -708,10 +708,10 @@ class Series(StgitObject):
         # Cleanup parent informations
         # FIXME: should one day make use of git-config --section-remove,
         # scheduled for 1.5.1
-        config.unset('branch.%s.remote' % self.__name)
-        config.unset('branch.%s.merge' % self.__name)
-        config.unset('branch.%s.stgit.parentbranch' % self.__name)
-        config.unset('branch.%s.stgitformatversion' % self.__name)
+        config.unset('branch.%s.remote' % self.get_name())
+        config.unset('branch.%s.merge' % self.get_name())
+        config.unset('branch.%s.stgit.parentbranch' % self.get_name())
+        config.unset('branch.%s.stgitformatversion' % self.get_name())
 
     def refresh_patch(self, files = None, message = None, edit = False,
                       show_patch = False,
@@ -1093,13 +1093,13 @@ class Series(StgitObject):
 
         patch = Patch(name, self.__patch_dir, self.__refs_dir)
 
-        if git.get_head_file() == self.get_branch():
+        if git.get_head_file() == self.get_name():
             if keep and not git.apply_diff(git.get_head(), patch.get_bottom()):
                 raise StackException(
                     'Failed to pop patches while preserving the local changes')
             git.switch(patch.get_bottom(), keep)
         else:
-            git.set_branch(self.get_branch(), patch.get_bottom())
+            git.set_branch(self.get_name(), patch.get_bottom())
 
         # save the new applied list
         idx = applied.index(name) + 1

[StGIT PATCH 2/4] Abstract a PatchSet object out of Series.

[Yann Dirson]

The goal is to move into PatchSet all functionnality that is not
dependant of Series being a real stack, paving the way for handling
parallel (hydra) stacks, or any other way to organize patches.
---

 stgit/stack.py |  110 +++++++++++++++++++++++++++++---------------------------
 1 files changed, 56 insertions(+), 54 deletions(-)

diff --git a/stgit/stack.py b/stgit/stack.py
index 0d475da..35840e2 100644
--- a/stgit/stack.py
+++ b/stgit/stack.py
@@ -348,7 +348,62 @@ def update_to_current_format_version(branch, git_dir):
         raise StackException('Branch %s is at format version %d, expected %d'
                              % (branch, get_format_version(), FORMAT_VERSION))
 
-class Series(StgitObject):
+class PatchSet(StgitObject):
+    def get_name(self):
+        return self.__name
+    def set_name(self, name):
+        self.__name = name
+
+    def get_head(self):
+        """Return the head of the branch
+        """
+        crt = self.get_current_patch()
+        if crt:
+            return crt.get_top()
+        else:
+            return self.get_base()
+
+    def get_protected(self):
+        return os.path.isfile(os.path.join(self._dir(), 'protected'))
+
+    def protect(self):
+        protect_file = os.path.join(self._dir(), 'protected')
+        if not os.path.isfile(protect_file):
+            create_empty_file(protect_file)
+
+    def unprotect(self):
+        protect_file = os.path.join(self._dir(), 'protected')
+        if os.path.isfile(protect_file):
+            os.remove(protect_file)
+
+    def __branch_descr(self):
+        return 'branch.%s.description' % self.get_name()
+
+    def get_description(self):
+        return config.get(self.__branch_descr()) or ''
+
+    def set_description(self, line):
+        if line:
+            config.set(self.__branch_descr(), line)
+        else:
+            config.unset(self.__branch_descr())
+
+    def head_top_equal(self):
+        """Return true if the head and the top are the same
+        """
+        crt = self.get_current_patch()
+        if not crt:
+            # we don't care, no patches applied
+            return True
+        return git.get_head() == crt.get_top()
+
+    def is_initialised(self):
+        """Checks if series is already initialised
+        """
+        return bool(config.get(format_version_key(self.get_name())))
+
+
+class Series(PatchSet):
     """Class including the operations on series
     """
     def __init__(self, name = None):
@@ -388,11 +443,6 @@ class Series(StgitObject):
         if not name or re.search('[^\w.-]', name):
             raise StackException, 'Invalid patch name: "%s"' % name
 
-    def get_name(self):
-        return self.__name
-    def set_name(self, name):
-        self.__name = name
-        
     def get_patch(self, name):
         """Return a Patch object for the given name
         """
@@ -444,40 +494,6 @@ class Series(StgitObject):
         # No bottommost patch, so just return HEAD
         return git.get_head()
 
-    def get_head(self):
-        """Return the head of the branch
-        """
-        crt = self.get_current_patch()
-        if crt:
-            return crt.get_top()
-        else:
-            return self.get_base()
-
-    def get_protected(self):
-        return os.path.isfile(os.path.join(self._dir(), 'protected'))
-
-    def protect(self):
-        protect_file = os.path.join(self._dir(), 'protected')
-        if not os.path.isfile(protect_file):
-            create_empty_file(protect_file)
-
-    def unprotect(self):
-        protect_file = os.path.join(self._dir(), 'protected')
-        if os.path.isfile(protect_file):
-            os.remove(protect_file)
-
-    def __branch_descr(self):
-        return 'branch.%s.description' % self.get_name()
-
-    def get_description(self):
-        return config.get(self.__branch_descr()) or ''
-
-    def set_description(self, line):
-        if line:
-            config.set(self.__branch_descr(), line)
-        else:
-            config.unset(self.__branch_descr())
-
     def get_parent_remote(self):
         value = config.get('branch.%s.remote' % self.get_name())
         if value:
@@ -547,20 +563,6 @@ class Series(StgitObject):
         return self.patch_applied(name) or self.patch_unapplied(name) \
                or self.patch_hidden(name)
 
-    def head_top_equal(self):
-        """Return true if the head and the top are the same
-        """
-        crt = self.get_current_patch()
-        if not crt:
-            # we don't care, no patches applied
-            return True
-        return git.get_head() == crt.get_top()
-
-    def is_initialised(self):
-        """Checks if series is already initialised
-        """
-        return bool(config.get(format_version_key(self.get_name())))
-
     def init(self, create_at=False, parent_remote=None, parent_branch=None):
         """Initialises the stgit series
         """

Re: [StGIT PATCH 2/4] Abstract a PatchSet object out of Series.

[Catalin Marinas]

On 11/06/07, Yann Dirson <ydirson@altern.org> wrote:
> The goal is to move into PatchSet all functionnality that is not
> dependant of Series being a real stack, paving the way for handling
> parallel (hydra) stacks, or any other way to organize patches.

Thanks for the patches. I applied this one as well and I like the
idea. What is your long-term plan with this split?

The HEAD in my repository fails the test suite. Do you have any
additional patches pending (some patches were not applied in order as
I had to manually fix the conflicts). Anyway, please check my
repository for any missing patches.

Thanks.

-- 
Catalin

Re: [StGIT PATCH 2/4] Abstract a PatchSet object out of Series.

[Yann Dirson]

On Tue, Jun 12, 2007 at 11:09:50PM +0100, Catalin Marinas wrote:
> On 11/06/07, Yann Dirson <ydirson@altern.org> wrote:
> >The goal is to move into PatchSet all functionnality that is not
> >dependant of Series being a real stack, paving the way for handling
> >parallel (hydra) stacks, or any other way to organize patches.
> 
> Thanks for the patches. I applied this one as well and I like the
> idea. What is your long-term plan with this split?

I have started to work on a Hydra class (available at [1], but be
aware it doesn't do much more than creating an octopus, and breaking
many current StGIT assumptions), with a testcase to demonstrate its
use), which binds together a set of stacks with an octopus, to allow
keeping related patches together, and allow people to pull from one
topic stack without getting unrelated work.

I'm however facing a problem, with the pervasive assumption that we
only have stacks :).  Basically, we probably need a factory of
PatchSet objects, which would return a Series or an Hydra according to
the specified name - I'm currently thinking about this, but don't
expect any working code too rapidly.


> The HEAD in my repository fails the test suite. Do you have any
> additional patches pending (some patches were not applied in order as
> I had to manually fix the conflicts). Anyway, please check my
> repository for any missing patches.

Oh, I had not noticed you had applied
bd69feaf7c3c94b6e7e216ea8091064af9cdfa97.  Sorry, I was not explicit
enough when posing this, only the cover mail included "RFC" in the
subject.  As described in that mail, there are problems both with the
original approach (Karl's test failing), and with that new one (that
exisiting test failing).

Do you have any idea on how we could overcome the problem ?  In the
meantime, we could possibly just comment that testcase out (or add
support for continuing the testsuite even with a failure) - the
problem it exhibits is probably less common than the one that was
fixed.

BTW, a02ba4077f12578fe31c99d903488804a656e1c4 has a slight problem: it
is a patch by Karl, which I re-sent in the same group since it was
exhibiting the problem (and with the intent of adding a signed-off-by
line, but my way of adding them trough a buggy template showed its
limits and the commit now have 2 signed-off-by lines with Karl's
name).  However, it was applied with myself as author, which is quite
wrong: could that be a but in "stg import" ignoring the Author field ?

Best regards,
-- 
Yann.

[1] http://repo.or.cz/w/stgit/ydirson.git

Re: StGIT rebasing safeguard

[Yann Dirson]

On Wed, Jun 13, 2007 at 10:38:21PM +0200, Yann Dirson wrote:
> > The HEAD in my repository fails the test suite. Do you have any
> > additional patches pending (some patches were not applied in order as
> > I had to manually fix the conflicts). Anyway, please check my
> > repository for any missing patches.
> 
> Oh, I had not noticed you had applied
> bd69feaf7c3c94b6e7e216ea8091064af9cdfa97.  Sorry, I was not explicit
> enough when posing this, only the cover mail included "RFC" in the
> subject.  As described in that mail, there are problems both with the
> original approach (Karl's test failing), and with that new one (that
> exisiting test failing).
> 
> Do you have any idea on how we could overcome the problem ?  In the
> meantime, we could possibly just comment that testcase out (or add
> support for continuing the testsuite even with a failure) - the
> problem it exhibits is probably less common than the one that was
> fixed.

Well, the patch I just sent which adds GIT_SKIP_TESTS support allows
to see that this patch indeed breaks another test - the testsuite can
be forced to skip them [1] for the sake of checking one did not break
anything else, but the second test that does not pass is annoying as
well, due to a similar problem:

When the parent branch is a rewinding one (eg. an stgit stack), then
the old version of the patch will be turned to unreachable by
pull/rebase, and we probably have even no way of telling stgit that it
is indeed expected, since the parent stack is a local one.  My own
workflow on StGIT is affected by the issue, since my "bugs" stack is
forked off my "master" stack (but hopefully an hydra will help me ;).

That makes me suspecting the reachability approach is a dead-end, and
we should either get back to the approach of recording old-base, or
find another solution.

Best regards,
-- 
Yann

[1] GIT_SKIP_TESTS='t2100.4 t2102.3' make test

Re: StGIT rebasing safeguard

[Catalin Marinas]

On 14/06/07, Yann Dirson <ydirson@altern.org> wrote:
> When the parent branch is a rewinding one (eg. an stgit stack), then
> the old version of the patch will be turned to unreachable by
> pull/rebase, and we probably have even no way of telling stgit that it
> is indeed expected, since the parent stack is a local one.  My own
> workflow on StGIT is affected by the issue, since my "bugs" stack is
> forked off my "master" stack (but hopefully an hydra will help me ;).

If I understand correctly, is this the case where you do a 'stg
commit'? This command is meant for branches that are never rebased
(i.e. my master stgit branch). For this branch one wouldn't have a
remote branch configured and hence git fetch shouldn't do anything.

> That makes me suspecting the reachability approach is a dead-end, and
> we should either get back to the approach of recording old-base, or
> find another solution.

Maybe defaulting to 'git pull' and only use 'git fetch' if explicitly
asked. A git-pull would still keep the old patch accessible from HEAD.

-- 
Catalin

Re: StGIT rebasing safeguard

[Catalin Marinas]

On 19/06/07, Catalin Marinas <catalin.marinas@gmail.com> wrote:
> On 14/06/07, Yann Dirson <ydirson@altern.org> wrote:
> > When the parent branch is a rewinding one (eg. an stgit stack), then
> > the old version of the patch will be turned to unreachable by
> > pull/rebase, and we probably have even no way of telling stgit that it
> > is indeed expected, since the parent stack is a local one.  My own
> > workflow on StGIT is affected by the issue, since my "bugs" stack is
> > forked off my "master" stack (but hopefully an hydra will help me ;).
>
> If I understand correctly, is this the case where you do a 'stg
> commit'? This command is meant for branches that are never rebased
> (i.e. my master stgit branch). For this branch one wouldn't have a
> remote branch configured and hence git fetch shouldn't do anything.

I got confused - you were talking about 'stg rebase' rather than the
'pull' strategy. But the question remains - are you referring to the
user running 'stg commit' and losing this commit after a rebase?

The rebase should be equivalent to a git-reset but with
popping/pushing of the patches one the stack. Once committed, the
patch is no longer managed by StGIT and therefore we shouldn't care.

-- 
Catalin

Re: StGIT rebasing safeguard

[Yann Dirson]

On Tue, Jun 19, 2007 at 11:18:56PM +0100, Catalin Marinas wrote:
> On 19/06/07, Catalin Marinas <catalin.marinas@gmail.com> wrote:
> >On 14/06/07, Yann Dirson <ydirson@altern.org> wrote:
> >> When the parent branch is a rewinding one (eg. an stgit stack), then
> >> the old version of the patch will be turned to unreachable by
> >> pull/rebase, and we probably have even no way of telling stgit that it
> >> is indeed expected, since the parent stack is a local one.  My own
> >> workflow on StGIT is affected by the issue, since my "bugs" stack is
> >> forked off my "master" stack (but hopefully an hydra will help me ;).
> >
> >If I understand correctly, is this the case where you do a 'stg
> >commit'? This command is meant for branches that are never rebased
> >(i.e. my master stgit branch). For this branch one wouldn't have a
> >remote branch configured and hence git fetch shouldn't do anything.
> 
> I got confused - you were talking about 'stg rebase' rather than the
> 'pull' strategy. But the question remains - are you referring to the
> user running 'stg commit' and losing this commit after a rebase?

Not at all.  I'm talking about an stgit stack, whose base is the head
of another stack (eg. a "pu" branch).  When you want to rebase, the
old heads/pu is only reachable from your stack base.

Maybe we can use the parent's reflog, but I'm not sure it would cover
all cases, and a reflog can possibly be expired before the information
in there gets used.


> The rebase should be equivalent to a git-reset but with
> popping/pushing of the patches one the stack. Once committed, the
> patch is no longer managed by StGIT and therefore we shouldn't care.

That is another issue - and I rather believe we should not allow a
user to do that without --force, if what he committed will be
unreachable after rebasing.  If he intended to loose it, he would
usually rather have used "stg delete" than such a convoluted sequence
of actions :)

Best regards,
-- 
Yann.

Re: [StGIT PATCH 2/4] Abstract a PatchSet object out of Series.

[Catalin Marinas]

On 13/06/07, Yann Dirson <ydirson@altern.org> wrote:
> I have started to work on a Hydra class (available at [1], but be
> aware it doesn't do much more than creating an octopus, and breaking
> many current StGIT assumptions), with a testcase to demonstrate its
> use), which binds together a set of stacks with an octopus, to allow
> keeping related patches together, and allow people to pull from one
> topic stack without getting unrelated work.

If it works, it would be really useful. Do the stacks need to be
independent? I can group my patches easily (and I was thinking about
"collapse/expand" commands for better viewing) but one stack might
still depend on patches from a different one. It would be nice if one
could also set the base of a series in this kind of hydra structure
(unless it makes it difficult to understand).

> > The HEAD in my repository fails the test suite. Do you have any
> > additional patches pending (some patches were not applied in order as
> > I had to manually fix the conflicts). Anyway, please check my
> > repository for any missing patches.
>
> Oh, I had not noticed you had applied
> bd69feaf7c3c94b6e7e216ea8091064af9cdfa97.  Sorry, I was not explicit
> enough when posing this, only the cover mail included "RFC" in the
> subject.

OK, they were left as unread in my inbox and thought they were new.

> As described in that mail, there are problems both with the
> original approach (Karl's test failing), and with that new one (that
> exisiting test failing).
>
> Do you have any idea on how we could overcome the problem ?  In the
> meantime, we could possibly just comment that testcase out (or add
> support for continuing the testsuite even with a failure) - the
> problem it exhibits is probably less common than the one that was
> fixed.

I am happy with only 2 modes - one using ORIG_HEAD for people using
StGIT in combination with plain GIT and the other overriding the base
without checks. The second mode is for people not mixing StGIT with
plain GIT. For the first mode, they have to deal with conflicts as
with the standard GIT.

> BTW, a02ba4077f12578fe31c99d903488804a656e1c4 has a slight problem: it
> is a patch by Karl, which I re-sent in the same group since it was
> exhibiting the problem (and with the intent of adding a signed-off-by
> line, but my way of adding them trough a buggy template showed its
> limits and the commit now have 2 signed-off-by lines with Karl's
> name).  However, it was applied with myself as author, which is quite
> wrong: could that be a but in "stg import" ignoring the Author field ?

It's not a bug. The import command just uses the e-mail sender or a
"From:" line before the patch description (see the default mail
template). It doesn't check the sign lines (it is following the kernel
patch submission guidelines).

I would drop both the above commits for release 0.13. Are you OK with this?

-- 
Catalin

Re: [StGIT PATCH 2/4] Abstract a PatchSet object out of Series.

[Yann Dirson]

On Thu, Jun 14, 2007 at 11:56:47PM +0100, Catalin Marinas wrote:
> On 13/06/07, Yann Dirson <ydirson@altern.org> wrote:
> >I have started to work on a Hydra class (available at [1], but be
> >aware it doesn't do much more than creating an octopus, and breaking
> >many current StGIT assumptions), with a testcase to demonstrate its
> >use), which binds together a set of stacks with an octopus, to allow
> >keeping related patches together, and allow people to pull from one
> >topic stack without getting unrelated work.
> 
> If it works, it would be really useful. Do the stacks need to be
> independent? I can group my patches easily (and I was thinking about
> "collapse/expand" commands for better viewing) but one stack might
> still depend on patches from a different one. It would be nice if one
> could also set the base of a series in this kind of hydra structure
> (unless it makes it difficult to understand).

Indeed one of the things that naturally come to mind after hydra, is
to abstract a parent class above PatchSet and Patch, and allow those
to be mostly used everywhere one of them is allowed.

That way we can have a (sub-)stack anywhere between 2 patches in a
stack, and that should I think address the need you describe.  But
that would also allow to have an hydra built of single patches instead
of stacks, which would be quite similar to how darcs organizes
patches.  Combinations are endless, and I don't even count the
possibility of adding new structures besides stacks and hydras :)


> >As described in that mail, there are problems both with the
> >original approach (Karl's test failing), and with that new one (that
> >exisiting test failing).
> >
> >Do you have any idea on how we could overcome the problem ?  In the
> >meantime, we could possibly just comment that testcase out (or add
> >support for continuing the testsuite even with a failure) - the
> >problem it exhibits is probably less common than the one that was
> >fixed.
> 
> I am happy with only 2 modes - one using ORIG_HEAD for people using
> StGIT in combination with plain GIT and the other overriding the base
> without checks. The second mode is for people not mixing StGIT with
> plain GIT. For the first mode, they have to deal with conflicts as
> with the standard GIT.

Well, I have to think about that in details - but right now I'm a bit
busy with the hydra thing.


> >BTW, a02ba4077f12578fe31c99d903488804a656e1c4 has a slight problem: it
> >is a patch by Karl, which I re-sent in the same group since it was
> >exhibiting the problem (and with the intent of adding a signed-off-by
> >line, but my way of adding them trough a buggy template showed its
> >limits and the commit now have 2 signed-off-by lines with Karl's
> >name).  However, it was applied with myself as author, which is quite
> >wrong: could that be a but in "stg import" ignoring the Author field ?
> 
> It's not a bug. The import command just uses the e-mail sender or a
> "From:" line before the patch description (see the default mail
> template). It doesn't check the sign lines (it is following the kernel
> patch submission guidelines).

We could surely improve things (and I'm not suggesting we should look
at sign lines).  Eg, by having stg-mail add an Author pseudo-header
when the patch author is different from the sender, and having
stg-import use that when available.


> I would drop both the above commits for release 0.13. Are you OK with this?

Seems reasonable: the previous situation only seems to hurt when using
a workflow not involving stgit alone, whereas with this patch we also
break stgit-only workflows.

Best regards,
-- 
Yann.

Re: [StGIT PATCH 2/4] Abstract a PatchSet object out of Series.

[Catalin Marinas]

On 15/06/07, Yann Dirson <ydirson@altern.org> wrote:
> Indeed one of the things that naturally come to mind after hydra, is
> to abstract a parent class above PatchSet and Patch, and allow those
> to be mostly used everywhere one of them is allowed.
>
> That way we can have a (sub-)stack anywhere between 2 patches in a
> stack, and that should I think address the need you describe.  But
> that would also allow to have an hydra built of single patches instead
> of stacks, which would be quite similar to how darcs organizes
> patches.  Combinations are endless, and I don't even count the
> possibility of adding new structures besides stacks and hydras :)

If you can track the patch dependencies, it would be enough to no
longer have a stack but a pool of patches (like darcs but without
exact patch commuting, the diff3 merging provides a good approximation
of this operation and in a much faster time). Simple patch dependency
could be detected by trying to apply a patch without others (it can be
optimised so that only patches touching common files should be
checked).

> > It's not a bug. The import command just uses the e-mail sender or a
> > "From:" line before the patch description (see the default mail
> > template). It doesn't check the sign lines (it is following the kernel
> > patch submission guidelines).
>
> We could surely improve things (and I'm not suggesting we should look
> at sign lines).  Eg, by having stg-mail add an Author pseudo-header
> when the patch author is different from the sender, and having
> stg-import use that when available.

But the 'stg mail' command already adds a "From:" line in the body
(with the default template) and 'import' checks for 'From:' or
'Author:' which override the e-mail sender. You might be using a
different e-mail template.

-- 
Catalin

[StGIT PATCH 3/4] Move stack format-version checks into Stack class.

[Yann Dirson]

This defines an interface which is called from PatchSet, and can
(must) be implemented in classes inheriting PatchSet.
---

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

diff --git a/stgit/stack.py b/stgit/stack.py
index 35840e2..89b5a55 100644
--- a/stgit/stack.py
+++ b/stgit/stack.py
@@ -276,78 +276,6 @@ class Patch(StgitObject):
 # The current StGIT metadata format version.
 FORMAT_VERSION = 2
 
-def format_version_key(branch):
-    return 'branch.%s.stgitformatversion' % branch
-
-def update_to_current_format_version(branch, git_dir):
-    """Update a potentially older StGIT directory structure to the
-    latest version. Note: This function should depend as little as
-    possible on external functions that may change during a format
-    version bump, since it must remain able to process older formats."""
-
-    branch_dir = os.path.join(git_dir, 'patches', branch)
-    def get_format_version():
-        """Return the integer format version number, or None if the
-        branch doesn't have any StGIT metadata at all, of any version."""
-        fv = config.get(format_version_key(branch))
-        if fv:
-            # Great, there's an explicitly recorded format version
-            # number, which means that the branch is initialized and
-            # of that exact version.
-            return int(fv)
-        elif os.path.isdir(os.path.join(branch_dir, 'patches')):
-            # There's a .git/patches/<branch>/patches dirctory, which
-            # means this is an initialized version 1 branch.
-            return 1
-        elif os.path.isdir(branch_dir):
-            # There's a .git/patches/<branch> directory, which means
-            # this is an initialized version 0 branch.
-            return 0
-        else:
-            # The branch doesn't seem to be initialized at all.
-            return None
-    def set_format_version(v):
-        out.info('Upgraded branch %s to format version %d' % (branch, v))
-        config.set(format_version_key(branch), '%d' % v)
-    def mkdir(d):
-        if not os.path.isdir(d):
-            os.makedirs(d)
-    def rm(f):
-        if os.path.exists(f):
-            os.remove(f)
-
-    # Update 0 -> 1.
-    if get_format_version() == 0:
-        mkdir(os.path.join(branch_dir, 'trash'))
-        patch_dir = os.path.join(branch_dir, 'patches')
-        mkdir(patch_dir)
-        refs_dir = os.path.join(git_dir, 'refs', 'patches', branch)
-        mkdir(refs_dir)
-        for patch in (file(os.path.join(branch_dir, 'unapplied')).readlines()
-                      + file(os.path.join(branch_dir, 'applied')).readlines()):
-            patch = patch.strip()
-            os.rename(os.path.join(branch_dir, patch),
-                      os.path.join(patch_dir, patch))
-            Patch(patch, patch_dir, refs_dir).update_top_ref()
-        set_format_version(1)
-
-    # Update 1 -> 2.
-    if get_format_version() == 1:
-        desc_file = os.path.join(branch_dir, 'description')
-        if os.path.isfile(desc_file):
-            desc = read_string(desc_file)
-            if desc:
-                config.set('branch.%s.description' % branch, desc)
-            rm(desc_file)
-        rm(os.path.join(branch_dir, 'current'))
-        rm(os.path.join(git_dir, 'refs', 'bases', branch))
-        set_format_version(2)
-
-    # Make sure we're at the latest version.
-    if not get_format_version() in [None, FORMAT_VERSION]:
-        raise StackException('Branch %s is at format version %d, expected %d'
-                             % (branch, get_format_version(), FORMAT_VERSION))
-
 class PatchSet(StgitObject):
     def get_name(self):
         return self.__name
@@ -400,7 +328,7 @@ class PatchSet(StgitObject):
     def is_initialised(self):
         """Checks if series is already initialised
         """
-        return bool(config.get(format_version_key(self.get_name())))
+        return bool(config.get(self.format_version_key()))
 
 
 class Series(PatchSet):
@@ -422,7 +350,7 @@ class Series(PatchSet):
 
         # Update the branch to the latest format version if it is
         # initialized, but don't touch it if it isn't.
-        update_to_current_format_version(self.get_name(), self.__base_dir)
+        self.update_to_current_format_version()
 
         self.__refs_dir = os.path.join(self.__base_dir, 'refs', 'patches',
                                        self.get_name())
@@ -437,6 +365,78 @@ class Series(PatchSet):
         # trash directory
         self.__trash_dir = os.path.join(self._dir(), 'trash')
 
+    def format_version_key(self):
+        return 'branch.%s.stgitformatversion' % self.get_name()
+
+    def update_to_current_format_version(self):
+        """Update a potentially older StGIT directory structure to the
+        latest version. Note: This function should depend as little as
+        possible on external functions that may change during a format
+        version bump, since it must remain able to process older formats."""
+
+        branch_dir = os.path.join(self.__base_dir, 'patches', self.get_name())
+        def get_format_version():
+            """Return the integer format version number, or None if the
+            branch doesn't have any StGIT metadata at all, of any version."""
+            fv = config.get(self.format_version_key())
+            if fv:
+                # Great, there's an explicitly recorded format version
+                # number, which means that the branch is initialized and
+                # of that exact version.
+                return int(fv)
+            elif os.path.isdir(os.path.join(branch_dir, 'patches')):
+                # There's a .git/patches/<branch>/patches dirctory, which
+                # means this is an initialized version 1 branch.
+                return 1
+            elif os.path.isdir(branch_dir):
+                # There's a .git/patches/<branch> directory, which means
+                # this is an initialized version 0 branch.
+                return 0
+            else:
+                # The branch doesn't seem to be initialized at all.
+                return None
+        def set_format_version(v):
+            out.info('Upgraded branch %s to format version %d' % (self.get_name(), v))
+            config.set(self.format_version_key(), '%d' % v)
+        def mkdir(d):
+            if not os.path.isdir(d):
+                os.makedirs(d)
+        def rm(f):
+            if os.path.exists(f):
+                os.remove(f)
+
+        # Update 0 -> 1.
+        if get_format_version() == 0:
+            mkdir(os.path.join(branch_dir, 'trash'))
+            patch_dir = os.path.join(branch_dir, 'patches')
+            mkdir(patch_dir)
+            refs_dir = os.path.join(self.__base_dir, 'refs', 'patches', self.get_name())
+            mkdir(refs_dir)
+            for patch in (file(os.path.join(branch_dir, 'unapplied')).readlines()
+                          + file(os.path.join(branch_dir, 'applied')).readlines()):
+                patch = patch.strip()
+                os.rename(os.path.join(branch_dir, patch),
+                          os.path.join(patch_dir, patch))
+                Patch(patch, patch_dir, refs_dir).update_top_ref()
+            set_format_version(1)
+
+        # Update 1 -> 2.
+        if get_format_version() == 1:
+            desc_file = os.path.join(branch_dir, 'description')
+            if os.path.isfile(desc_file):
+                desc = read_string(desc_file)
+                if desc:
+                    config.set('branch.%s.description' % self.get_name(), desc)
+                rm(desc_file)
+            rm(os.path.join(branch_dir, 'current'))
+            rm(os.path.join(self.__base_dir, 'refs', 'bases', self.get_name()))
+            set_format_version(2)
+
+        # Make sure we're at the latest version.
+        if not get_format_version() in [None, FORMAT_VERSION]:
+            raise StackException('Branch %s is at format version %d, expected %d'
+                                 % (self.get_name(), get_format_version(), FORMAT_VERSION))
+
     def __patch_name_valid(self, name):
         """Raise an exception if the patch name is not valid.
         """
@@ -584,7 +584,7 @@ class Series(PatchSet):
         os.makedirs(self.__refs_dir)
         self._set_field('orig-base', git.get_head())
 
-        config.set(format_version_key(self.get_name()), str(FORMAT_VERSION))
+        config.set(self.format_version_key(), str(FORMAT_VERSION))
 
     def rename(self, to_name):
         """Renames a series

[StGIT PATCH 4/4] Rename branch.*.stgitformatversion to branch.*.stgit.stackformatversion.

[Yann Dirson]

Nearly as discussed on the ml, with the name slightly modified to
better allow other types of PatchSet.
---

 stgit/stack.py |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/stgit/stack.py b/stgit/stack.py
index 89b5a55..634588d 100644
--- a/stgit/stack.py
+++ b/stgit/stack.py
@@ -366,7 +366,7 @@ class Series(PatchSet):
         self.__trash_dir = os.path.join(self._dir(), 'trash')
 
     def format_version_key(self):
-        return 'branch.%s.stgitformatversion' % self.get_name()
+        return 'branch.%s.stgit.stackformatversion' % self.get_name()
 
     def update_to_current_format_version(self):
         """Update a potentially older StGIT directory structure to the
@@ -379,11 +379,17 @@ class Series(PatchSet):
             """Return the integer format version number, or None if the
             branch doesn't have any StGIT metadata at all, of any version."""
             fv = config.get(self.format_version_key())
+            ofv = config.get('branch.%s.stgitformatversion' % self.get_name())
             if fv:
                 # Great, there's an explicitly recorded format version
                 # number, which means that the branch is initialized and
                 # of that exact version.
                 return int(fv)
+            elif ofv:
+                # Old name for the version info, upgrade it
+                config.set(self.format_version_key(), ofv)
+                config.unset('branch.%s.stgitformatversion' % self.get_name())
+                return int(ofv)
             elif os.path.isdir(os.path.join(branch_dir, 'patches')):
                 # There's a .git/patches/<branch>/patches dirctory, which
                 # means this is an initialized version 1 branch.
@@ -713,7 +719,7 @@ class Series(PatchSet):
         config.unset('branch.%s.remote' % self.get_name())
         config.unset('branch.%s.merge' % self.get_name())
         config.unset('branch.%s.stgit.parentbranch' % self.get_name())
-        config.unset('branch.%s.stgitformatversion' % self.get_name())
+        config.unset(self.format_version_key())
 
     def refresh_patch(self, files = None, message = None, edit = False,
                       show_patch = False,