2008/6/5 Karl Hasselström <kha@treskal.com>:
quoted text
> 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?

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

quoted text
> (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.

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

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

quoted text
>> +    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.

quoted text
>> +    @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.

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

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

quoted text
>> +    @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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html