From: Karl Hasselström <kha@treskal.com>
Date: Sat, 07 Jun 2008 09:30:51 GMT
Subject: Re: [StGIT PATCH 3/5] Add stack creation and initialisation support to lib.Stack
Message-ID: <20080607093051.GE32647@diana.vm.bytemark.co.uk>
In-Reply-To: <20080606204554.8805.37257.stgit@localhost.localdomain>

On 2008-06-06 21:45:54 +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.
>
> Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>

Acked-by: Karl Hasselström <kha@treskal.com>

I still have some comments, bu the patch looks good as-is.

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

Maybe s/create/create_files/ or soething to that effect here, as I
suggested in another mail a few minutes ago.

> +    def set_parents(self, remote, branch):
> +        if remote:
> +            self.set_parent_remote(remote)
> +        if branch:
> +            self.set_parent_branch(branch)

Much clearer now. Interesting that the only reason we need this
function at all is to make sure that set_parent_remote and
set_parent_branch are called in the correct order.

Maybe set_parent_branch should throw an exception instead of silently
doing nothing when there is no parent yet? It would force the callers
to be more aware of the limitation -- though we might not actually
want that. Your call.

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