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

From: Karl Hasselström <kha@treskal.com>
Date: Jun 5, 2008, 07:28 UTC
Message-ID: <20080605072822.GD23209@diana.vm.bytemark.co.uk>
In-Reply-To: <20080604211343.32531.41429.stgit@localhost.localdomain>

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.

Show 12 quoted lines

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

Show 15 quoted lines

> @@ -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.

Show 6 quoted lines

> @@ -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.

Show 7 quoted lines

> +    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?)

Show 21 quoted lines

> +    @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?

Show 9 quoted lines

> +
> +    @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