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

From: Catalin Marinas <catalin.marinas@gmail.com>
Date: Jun 5, 2008, 12:42 UTC
Message-ID: <b0943d9e0806050542w663c64a8o93f81503eb811cf9@mail.gmail.com>
In-Reply-To: <20080605072822.GD23209@diana.vm.bytemark.co.uk>

2008/6/5 Karl Hasselström <kha@treskal.com>:

Show 19 quoted lines

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

I was more referring to objects in the lib.stack module, not only the Stack class. Can this be expressed in any way with the epydoc format?

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

It is a bit more complicated. PatchOrder requires a Stack object in __init__ but the stack is not fully set up at this point.

> (You
> might even consider having these files auto-created whenever you
> instantiate a PatchOrder object and they don't yet exist.)

This is definitely a solution but at the moment we have to support the old infrastructure which fails if the files aren't present. Once we don't have any reference to the stgit.stack module, we can do this more dynamically.

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

See my reply to a previous patch with my view on ownership and separation of functionality.

Show 9 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.

Yes.

Show 13 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?)

I don't fully follow it either :-). I just copied the code from stgit.stack.Series but I don't make any use of it yet. I think it was initially written by Yann Dirson for the "branch" command which I haven't translated yet. I think branch.<branchname>.stgit.parentbranch might be used by the current "pull" or "rebase" implementations. I'll comment it out (but still keep it for now) and put a FIXME so that I remember when converting the "branch" and "pull" commands.

Show 24 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.

That was the intention. For creation, there is Stack.create which handles the Git branch creation as well as the StGIT initialisation.

Show 9 quoted lines

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

That was the original behaviour in stgit.stack.Series but if you happen to try to access a patch or patchorder, you get some exception on missing file form those objects rather than "stack not initialised" from Stack.

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

I'm not fully convinced with automatic initialisation. I have some branches where I don't use StGIT but I might type a "stg series" by mistake. The branch will be promoted to a stack but later Git doesn't know about extra files in .git/patches and they aren't handled (removed, renamed etc.). I'm more in favour of the explicit initialisation.

Show 13 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.

Yes, I'll add some description.

Thanks for your comments.

-- 
Catalin