Re: [StGIT PATCH 4/5] Add stack creation and initialisation support to lib.Stack

["Karl Hasselström" <kha@treskal.com>]

On 2008-06-04 22:13:43 +0100, Catalin Marinas wrote:

> This patch adds the create and initialise Stack classmethods to
> handle the initialisation of StGIT patch series on a Git branch.

> diff --git a/stgit/lib/stack.py b/stgit/lib/stack.py
> index aca7a36..7375d41 100644
> --- a/stgit/lib/stack.py
> +++ b/stgit/lib/stack.py
> @@ -3,6 +3,10 @@
>  import os.path
>  from stgit import exception, utils
>  from stgit.lib import git, stackupgrade
> +from stgit.config import config
> +
> +class StackException(exception.StgException):
> +    """Exception raised by stack objects."""

s/stack/L{Stack}/, perhaps?

> @@ -105,6 +109,14 @@ class PatchOrder(object):
>      all = property(lambda self: self.applied + self.unapplied + self.hidden)
>      all_visible = property(lambda self: self.applied + self.unapplied)
>  
> +    @staticmethod
> +    def create(stackdir):
> +        """Create the PatchOrder specific files
> +        """
> +        utils.create_empty_file(os.path.join(stackdir, 'applied'))
> +        utils.create_empty_file(os.path.join(stackdir, 'unapplied'))
> +        utils.create_empty_file(os.path.join(stackdir, 'hidden'))
> +
>  class Patches(object):
>      """Creates L{Patch} objects. Makes sure there is only one such object
>      per patch."""

Wouldn't it be more consistent if the create function actually
returned a PatchOrder object, like other creation functions? (You
might even consider having these files auto-created whenever you
instantiate a PatchOrder object and they don't yet exist.)

Also, the creation function might instead live in the Stack class,
since it owns the patch order.

> @@ -133,12 +145,14 @@ class Patches(object):
>  class Stack(git.Branch):
>      """Represents an StGit stack (that is, a git branch with some extra
>      metadata)."""
> +    __repo_subdir = 'patches'
> +

This needs to be in the previous patch, I think, since you use it
there.

> +    def set_parents(self, remote, localbranch):
> +        if not localbranch:
> +            return
> +        if remote:
> +            self.set_parent_remote(remote)
> +        self.set_parent_branch(localbranch)
> +        config.set('branch.%s.stgit.parentbranch' % self._name, localbranch)

Hmm, I don't quite follow. Why is this a no-op if you give a false
localbranch? And why is branch.<branchname>.stgit.parentbranch needed,
when it's always the same as branch.<branchname>.merge? (Backwards
compatibility? Would you mind making a comment about that, in that
case?)

> +    @classmethod
> +    def initialise(cls, repository, name = None):
> +        """Initialise a Git branch to handle patch series."""
> +        if not name:
> +            name = repository.current_branch_name
> +        # make sure that the corresponding Git branch exists
> +        git.Branch(repository, name)
> +
> +        dir = os.path.join(repository.directory, cls.__repo_subdir, name)
> +        compat_dir = os.path.join(dir, 'patches')
> +        if os.path.exists(dir):
> +            raise StackException('%s: branch already initialized' % name)
> +
> +        # create the stack directory and files
> +        utils.create_dirs(dir)
> +        utils.create_dirs(compat_dir)
> +        PatchOrder.create(dir)
> +        config.set(stackupgrade.format_version_key(name),
> +                   str(stackupgrade.FORMAT_VERSION))
> +
> +        return repository.get_stack(name)

This is not quite like the other "create" functions, since it just
promotes a branch, without really creating it.

What I'd really like to see here, I think, is something like this:

  1. You get a Stack object from some stack.Repository method.

  2. The Stack object works without having to be initialized, but the
     operations that need initialization throw an exception.

  3. The Stack object has an initialize() method -- just a normal
     method, not a class method.

This will pave the way for automatic initialization -- just call
self.initialize() instead of throwing an exception in step (2).

What do you think?

> +
> +    @classmethod
> +    def create(cls, repository, name,
> +               create_at = None, parent_remote = None, parent_branch = None):
> +        """Create and initialise a Git branch returning the L{Stack} object."""
> +        git.Branch.create(repository, name, create_at = create_at)
> +        stack = cls.initialise(repository, name)
> +        stack.set_parents(parent_remote, parent_branch)
> +        return stack

Same point as with the other creation functions.

And I'd appreciate some documentation on what the parameters mean --
either here, or in the methods you call from here.

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