{"thread":{"id":"13803","subject":"[StGIT PATCH 1/5] Allow stack.patchorder.all to return hidden patches","startedAt":"2008-06-04T21:13:08Z","lastAt":"2008-06-09T07:07:22Z","messageCount":28,"participants":["Catalin Marinas","Karl Hasselström","David Aguilar","Sverre Rabbelier"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"78671","messageId":"20080604210655.32531.82580.stgit@localhost.localdomain","threadId":"13803","inReplyTo":null,"subject":"[StGIT PATCH 0/5] Various updates to the new infrastructure","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-06-04T21:13:08Z","receivedAt":"2008-06-04T21:13:08Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"For a wider review and to avoid merging conflicts with other developer's \ntree (especially Karl's), I created a \"proposed\" branch in my StGIT tree\n(http://homepage.ntlworld.com/cmarinas/stgit.git) containing patches that \nI intend to merge on \"master\". I'll also post these patches on the list.\n\n---\n\nCatalin Marinas (5):\n Add stack creation and deletion support to the new infrastructure\n Add stack creation and initialisation support to lib.Stack\n Create a git.Branch class as ancestor of stack.Stack\n Rename Repository.head to Repository.head_ref\n Allow stack.patchorder.all to return hidden patches\n\n\n stgit/commands/commit.py | 4 +-\n stgit/commands/init.py | 14 ++------\n stgit/commands/new.py | 3 +-\n stgit/lib/git.py | 55 ++++++++++++++++++++++++++++-\n stgit/lib/stack.py | 84 ++++++++++++++++++++++++++++++++-------------\n stgit/lib/stackupgrade.py | 7 ++++\n 6 files changed, 127 insertions(+), 40 deletions(-)\n\n-- \nCatalin\n"},{"id":"78669","messageId":"20080604211316.32531.84226.stgit@localhost.localdomain","threadId":"13803","inReplyTo":"20080604210655.32531.82580.stgit@localhost.localdomain","subject":"[StGIT PATCH 1/5] Allow stack.patchorder.all to return hidden patches","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-06-04T21:13:17Z","receivedAt":"2008-06-04T21:13:17Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"A new property, patchorder.all_visible, was added to return only the\napplied + unapplied patches. This is used in the \"commit\" command to\navoid automatically committing the hidden patches.\n\nSigned-off-by: Catalin Marinas \n---\n\n stgit/commands/commit.py | 4 ++--\n stgit/lib/stack.py | 4 ++--\n stgit/lib/stackupgrade.py | 7 +++++++\n 3 files changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/stgit/commands/commit.py b/stgit/commands/commit.py\nindex cc2f13a..1bdbeea 100644\n--- a/stgit/commands/commit.py\n+++ b/stgit/commands/commit.py\n@@ -45,11 +45,11 @@ options = [make_option('-n', '--number', type = 'int',\n def func(parser, options, args):\n \"\"\"Commit a number of patches.\"\"\"\n stack = directory.repository.current_stack\n- args = common.parse_patches(args, list(stack.patchorder.all))\n+ args = common.parse_patches(args, list(stack.patchorder.all_visible))\n if len([x for x in [args, options.number != None, options.all] if x]) > 1:\n parser.error('too many options')\n if args:\n- patches = [pn for pn in stack.patchorder.all if pn in args]\n+ patches = [pn for pn in stack.patchorder.all_visible if pn in args]\n bad = set(args) - set(patches)\n if bad:\n raise common.CmdException('Bad patch names: %s'\ndiff --git a/stgit/lib/stack.py b/stgit/lib/stack.py\nindex f9e750e..bdd21b1 100644\n--- a/stgit/lib/stack.py\n+++ b/stgit/lib/stack.py\n@@ -102,8 +102,8 @@ class PatchOrder(object):\n lambda self, val: self.__set_list('unapplied', val))\n hidden = property(lambda self: self.__get_list('hidden'),\n lambda self, val: self.__set_list('hidden', val))\n- # don't return the hidden patches, these have to be returned explicitly\n- all = property(lambda self: self.applied + self.unapplied)\n+ all = property(lambda self: self.applied + self.unapplied + self.hidden)\n+ all_visible = property(lambda self: self.applied + self.unapplied)\n \n class Patches(object):\n \"\"\"Creates L{Patch} objects. Makes sure there is only one such object\ndiff --git a/stgit/lib/stackupgrade.py b/stgit/lib/stackupgrade.py\nindex 96ccb79..4b437dc 100644\n--- a/stgit/lib/stackupgrade.py\n+++ b/stgit/lib/stackupgrade.py\n@@ -90,6 +90,13 @@ def update_to_current_format_version(repository, branch):\n rm_ref('refs/bases/%s' % branch)\n set_format_version(2)\n \n+ # compatibility with the new infrastructure. The changes here do not\n+ # affect the compatibility with the old infrastructure (format version 2)\n+ if get_format_version() == 2:\n+ hidden_file = os.path.join(branch_dir, 'hidden')\n+ if not os.path.isfile(hidden_file):\n+ utils.create_empty_file(hidden_file)\n+\n # Make sure we're at the latest version.\n fv = get_format_version()\n if not fv in [None, FORMAT_VERSION]:\n"},{"id":"78672","messageId":"20080604211325.32531.84968.stgit@localhost.localdomain","threadId":"13803","inReplyTo":"20080604210655.32531.82580.stgit@localhost.localdomain","subject":"[StGIT PATCH 2/5] Rename Repository.head to Repository.head_ref","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-06-04T21:13:25Z","receivedAt":"2008-06-04T21:13:25Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"This is to avoid confusion with the Stack.head function which returns\na commit object rather than a file name. The patch also changes the\n\"new\" function to use stack.head directly rather than via the\nRepository.refs... object.\n\nSigned-off-by: Catalin Marinas \n---\n\n stgit/commands/new.py | 3 +--\n stgit/lib/git.py | 4 ++--\n 2 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/stgit/commands/new.py b/stgit/commands/new.py\nindex 15bb2e0..c4ee4e1 100644\n--- a/stgit/commands/new.py\n+++ b/stgit/commands/new.py\n@@ -58,9 +58,8 @@ def func(parser, options, args):\n else:\n parser.error('incorrect number of arguments')\n \n- head = directory.repository.refs.get(directory.repository.head)\n cd = gitlib.CommitData(\n- tree = head.data.tree, parents = [head], message = '',\n+ tree = stack.head.data.tree, parents = [stack.head], message = '',\n author = gitlib.Person.author(), committer = gitlib.Person.committer())\n \n # Set patch commit message from commandline.\ndiff --git a/stgit/lib/git.py b/stgit/lib/git.py\nindex c9f01e3..fd66f6d 100644\n--- a/stgit/lib/git.py\n+++ b/stgit/lib/git.py\n@@ -443,13 +443,13 @@ class Repository(RunWithEnv):\n ).output_one_line()\n return self.get_commit(sha1)\n @property\n- def head(self):\n+ def head_ref(self):\n try:\n return self.run(['git', 'symbolic-ref', '-q', 'HEAD']\n ).output_one_line()\n except run.RunException:\n raise DetachedHeadException()\n- def set_head(self, ref, msg):\n+ def set_head_ref(self, ref, msg):\n self.run(['git', 'symbolic-ref', '-m', msg, 'HEAD', ref]).no_output()\n def simple_merge(self, base, ours, theirs):\n \"\"\"Given three L{Tree}s, tries to do an in-index merge with a\n"},{"id":"78673","messageId":"20080604211334.32531.74258.stgit@localhost.localdomain","threadId":"13803","inReplyTo":"20080604210655.32531.82580.stgit@localhost.localdomain","subject":"[StGIT PATCH 3/5] Create a git.Branch class as ancestor of stack.Stack","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-06-04T21:13:35Z","receivedAt":"2008-06-04T21:13:35Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"This class deals with Git-specific branch commands. The Stack class is a\ndirect child of Branch and some of its functionality was moved to the new\nclass.\n\nSigned-off-by: Catalin Marinas \n---\n\n stgit/lib/git.py | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++\n stgit/lib/stack.py | 25 ++++---------------------\n 2 files changed, 55 insertions(+), 21 deletions(-)\n\ndiff --git a/stgit/lib/git.py b/stgit/lib/git.py\nindex fd66f6d..6393af2 100644\n--- a/stgit/lib/git.py\n+++ b/stgit/lib/git.py\n@@ -28,6 +28,9 @@ class RepositoryException(exception.StgException):\n \"\"\"Base class for all exceptions due to failed L{Repository}\n operations.\"\"\"\n \n+class BranchException(exception.StgException):\n+ \"\"\"Exception raised by failed L{Branch} operations.\"\"\"\n+\n class DateException(exception.StgException):\n \"\"\"Exception raised when a date+time string could not be parsed.\"\"\"\n def __init__(self, string, type):\n@@ -379,6 +382,10 @@ class Repository(RunWithEnv):\n except run.RunException:\n raise RepositoryException('Cannot find git repository')\n @property\n+ def current_branch_name(self):\n+ \"\"\"Return the name of the current branch.\"\"\"\n+ return utils.strip_leading('refs/heads/', self.head_ref)\n+ @property\n def default_index(self):\n \"\"\"An L{Index} object representing the default index file for the\n repository.\"\"\"\n@@ -619,3 +626,47 @@ class IndexAndWorktree(RunWithEnvCwd):\n def update_index(self, files):\n self.run(['git', 'update-index', '--remove', '-z', '--stdin']\n ).input_nulterm(files).discard_output()\n+\n+class Branch(object):\n+ \"\"\"Represents Git branch.\"\"\"\n+ def __init__(self, repository, name):\n+ self._repository = repository\n+ self._name = name\n+ try:\n+ self.head\n+ except KeyError:\n+ raise BranchException('%s: no such branch' % name)\n+\n+ name = property(lambda self: self._name)\n+ repository = property(lambda self: self._repository)\n+\n+ def __ref(self):\n+ return 'refs/heads/%s' % self._name\n+ @property\n+ def head(self):\n+ return self._repository.refs.get(self.__ref())\n+ def set_head(self, commit, msg):\n+ self._repository.refs.set(self.__ref(), commit, msg)\n+\n+ def set_parent_remote(self, name):\n+ value = config.set('branch.%s.remote' % self._name, name)\n+ def set_parent_branch(self, name):\n+ if config.get('branch.%s.remote' % self._name):\n+ # Never set merge if remote is not set to avoid\n+ # possibly-erroneous lookups into 'origin'\n+ config.set('branch.%s.merge' % self._name, name)\n+\n+ @classmethod\n+ def create(cls, repository, name, create_at = None):\n+ \"\"\"Create a new Git branch and return the corresponding L{Branch} object.\"\"\"\n+ try:\n+ branch = cls(repository, name)\n+ except BranchException:\n+ branch = None\n+ if branch:\n+ raise BranchException('%s: branch already exists' % name)\n+\n+ cmd = ['git', 'branch']\n+ if create_at:\n+ cmd.append(create_at.sha1)\n+ self._repository.run(['git', 'branch', create_at.sha1]).discard_output()\ndiff --git a/stgit/lib/stack.py b/stgit/lib/stack.py\nindex bdd21b1..aca7a36 100644\n--- a/stgit/lib/stack.py\n+++ b/stgit/lib/stack.py\n@@ -130,34 +130,20 @@ class Patches(object):\n self.__patches[name] = p\n return p\n \n-class Stack(object):\n+class Stack(git.Branch):\n \"\"\"Represents an StGit stack (that is, a git branch with some extra\n metadata).\"\"\"\n def __init__(self, repository, name):\n- self.__repository = repository\n- self.__name = name\n- try:\n- self.head\n- except KeyError:\n- raise exception.StgException('%s: no such branch' % name)\n+ git.Branch.__init__(self, repository, name)\n self.__patchorder = PatchOrder(self)\n self.__patches = Patches(self)\n if not stackupgrade.update_to_current_format_version(repository, name):\n raise exception.StgException('%s: branch not initialized' % name)\n- name = property(lambda self: self.__name)\n- repository = property(lambda self: self.__repository)\n patchorder = property(lambda self: self.__patchorder)\n patches = property(lambda self: self.__patches)\n @property\n def directory(self):\n- return os.path.join(self.__repository.directory, 'patches', self.__name)\n- def __ref(self):\n- return 'refs/heads/%s' % self.__name\n- @property\n- def head(self):\n- return self.__repository.refs.get(self.__ref())\n- def set_head(self, commit, msg):\n- self.__repository.refs.set(self.__ref(), commit, msg)\n+ return os.path.join(self._repository.directory, self.__repo_subdir, self._name)\n @property\n def base(self):\n if self.patchorder.applied:\n@@ -177,14 +163,11 @@ class Repository(git.Repository):\n git.Repository.__init__(self, *args, **kwargs)\n self.__stacks = {} # name -> Stack\n @property\n- def current_branch(self):\n- return utils.strip_leading('refs/heads/', self.head)\n- @property\n def current_stack(self):\n return self.get_stack()\n def get_stack(self, name = None):\n if not name:\n- name = self.current_branch\n+ name = self.current_branch_name\n if not name in self.__stacks:\n self.__stacks[name] = Stack(self, name)\n return self.__stacks[name]\n"},{"id":"78675","messageId":"20080604211343.32531.41429.stgit@localhost.localdomain","threadId":"13803","inReplyTo":"20080604210655.32531.82580.stgit@localhost.localdomain","subject":"[StGIT PATCH 4/5] Add stack creation and initialisation support to lib.Stack","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-06-04T21:13:43Z","receivedAt":"2008-06-04T21:13:43Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"This patch adds the create and initialise Stack classmethods to handle\nthe initialisation of StGIT patch series on a Git branch.\n\nSigned-off-by: Catalin Marinas \n---\n\n stgit/lib/stack.py | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 files changed, 54 insertions(+), 1 deletions(-)\n\ndiff --git a/stgit/lib/stack.py b/stgit/lib/stack.py\nindex aca7a36..7375d41 100644\n--- a/stgit/lib/stack.py\n+++ b/stgit/lib/stack.py\n@@ -3,6 +3,10 @@\n import os.path\n from stgit import exception, utils\n from stgit.lib import git, stackupgrade\n+from stgit.config import config\n+\n+class StackException(exception.StgException):\n+ \"\"\"Exception raised by stack objects.\"\"\"\n \n class Patch(object):\n \"\"\"Represents an StGit patch. This class is mainly concerned with\n@@ -105,6 +109,14 @@ class PatchOrder(object):\n all = property(lambda self: self.applied + self.unapplied + self.hidden)\n all_visible = property(lambda self: self.applied + self.unapplied)\n \n+ @staticmethod\n+ def create(stackdir):\n+ \"\"\"Create the PatchOrder specific files\n+ \"\"\"\n+ utils.create_empty_file(os.path.join(stackdir, 'applied'))\n+ utils.create_empty_file(os.path.join(stackdir, 'unapplied'))\n+ utils.create_empty_file(os.path.join(stackdir, 'hidden'))\n+\n class Patches(object):\n \"\"\"Creates L{Patch} objects. Makes sure there is only one such object\n per patch.\"\"\"\n@@ -133,12 +145,14 @@ class Patches(object):\n class Stack(git.Branch):\n \"\"\"Represents an StGit stack (that is, a git branch with some extra\n metadata).\"\"\"\n+ __repo_subdir = 'patches'\n+\n def __init__(self, repository, name):\n git.Branch.__init__(self, repository, name)\n self.__patchorder = PatchOrder(self)\n self.__patches = Patches(self)\n if not stackupgrade.update_to_current_format_version(repository, name):\n- raise exception.StgException('%s: branch not initialized' % name)\n+ raise StackException('%s: branch not initialized' % name)\n patchorder = property(lambda self: self.__patchorder)\n patches = property(lambda self: self.__patches)\n @property\n@@ -156,6 +170,45 @@ class Stack(git.Branch):\n return True\n return self.head == self.patches.get(self.patchorder.applied[-1]).commit\n \n+ def set_parents(self, remote, localbranch):\n+ if not localbranch:\n+ return\n+ if remote:\n+ self.set_parent_remote(remote)\n+ self.set_parent_branch(localbranch)\n+ config.set('branch.%s.stgit.parentbranch' % self._name, localbranch)\n+\n+ @classmethod\n+ def initialise(cls, repository, name = None):\n+ \"\"\"Initialise a Git branch to handle patch series.\"\"\"\n+ if not name:\n+ name = repository.current_branch_name\n+ # make sure that the corresponding Git branch exists\n+ git.Branch(repository, name)\n+\n+ dir = os.path.join(repository.directory, cls.__repo_subdir, name)\n+ compat_dir = os.path.join(dir, 'patches')\n+ if os.path.exists(dir):\n+ raise StackException('%s: branch already initialized' % name)\n+\n+ # create the stack directory and files\n+ utils.create_dirs(dir)\n+ utils.create_dirs(compat_dir)\n+ PatchOrder.create(dir)\n+ config.set(stackupgrade.format_version_key(name),\n+ str(stackupgrade.FORMAT_VERSION))\n+\n+ return repository.get_stack(name)\n+\n+ @classmethod\n+ def create(cls, repository, name,\n+ create_at = None, parent_remote = None, parent_branch = None):\n+ \"\"\"Create and initialise a Git branch returning the L{Stack} object.\"\"\"\n+ git.Branch.create(repository, name, create_at = create_at)\n+ stack = cls.initialise(repository, name)\n+ stack.set_parents(parent_remote, parent_branch)\n+ return stack\n+\n class Repository(git.Repository):\n \"\"\"A git L{Repository} with some added StGit-specific\n operations.\"\"\"\n"},{"id":"78674","messageId":"20080604211352.32531.10821.stgit@localhost.localdomain","threadId":"13803","inReplyTo":"20080604210655.32531.82580.stgit@localhost.localdomain","subject":"[StGIT PATCH 5/5] Add stack creation and deletion support to the new infrastructure","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-06-04T21:13:52Z","receivedAt":"2008-06-04T21:13:52Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"Signed-off-by: Catalin Marinas \n---\n\n stgit/commands/init.py | 14 ++++----------\n 1 files changed, 4 insertions(+), 10 deletions(-)\n\ndiff --git a/stgit/commands/init.py b/stgit/commands/init.py\nindex 475a4ce..b68acd7 100644\n--- a/stgit/commands/init.py\n+++ b/stgit/commands/init.py\n@@ -16,13 +16,8 @@ along with this program; if not, write to the Free Software\n Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA\n \"\"\"\n \n-import sys, os\n-from optparse import OptionParser, make_option\n-\n-from stgit.commands.common import *\n-from stgit.utils import *\n-from stgit import stack, git\n-\n+from stgit.commands import common\n+from stgit.lib import stack\n \n help = 'initialise the current branch for use with StGIT'\n usage = \"\"\"%prog [options]\n@@ -31,14 +26,13 @@ Initialise the current GIT branch to be used as an StGIT stack. Note\n that you must already be in a GIT repository and .git/HEAD must point\n to a valid file in refs/heads/.\"\"\"\n \n-directory = DirectoryHasRepository()\n+directory = common.DirectoryHasRepositoryLib()\n options = []\n \n-\n def func(parser, options, args):\n \"\"\"Performs the repository initialisation\n \"\"\"\n if len(args) != 0:\n parser.error('incorrect number of arguments')\n \n- crt_series.init()\n+ stack.Stack.initialise(directory.repository)\n"},{"id":"78705","messageId":"20080605064152.GA23209@diana.vm.bytemark.co.uk","threadId":"13803","inReplyTo":"20080604211316.32531.84226.stgit@localhost.localdomain","subject":"Re: [StGIT PATCH 1/5] Allow stack.patchorder.all to return hidden patches","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-06-05T06:41:52Z","receivedAt":"2008-06-05T06:41:52Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-06-04 22:13:17 +0100, Catalin Marinas wrote:\n\n> A new property, patchorder.all_visible, was added to return only the\n> applied + unapplied patches. This is used in the \"commit\" command to\n> avoid automatically committing the hidden patches.\n>\n> Signed-off-by: Catalin Marinas \n\nAcked-by: Karl Hasselström \n\nProvided you improve the error message (see below).\n\n> diff --git a/stgit/commands/commit.py b/stgit/commands/commit.py\n> index cc2f13a..1bdbeea 100644\n> --- a/stgit/commands/commit.py\n> +++ b/stgit/commands/commit.py\n> @@ -45,11 +45,11 @@ options = [make_option('-n', '--number', type = 'int',\n> def func(parser, options, args):\n> \"\"\"Commit a number of patches.\"\"\"\n> stack = directory.repository.current_stack\n> - args = common.parse_patches(args, list(stack.patchorder.all))\n> + args = common.parse_patches(args, list(stack.patchorder.all_visible))\n> if len([x for x in [args, options.number != None, options.all] if x]) > 1:\n> parser.error('too many options')\n> if args:\n> - patches = [pn for pn in stack.patchorder.all if pn in args]\n> + patches = [pn for pn in stack.patchorder.all_visible if pn in args]\n> bad = set(args) - set(patches)\n> if bad:\n> raise common.CmdException('Bad patch names: %s'\n\nForbidding commit to operate on hidden patches? OK, why not? But\nsaying \"Bad patch name\" if the user gives a hidden patch is probably a\nbit on the grumpy side ... :-)\n\n> diff --git a/stgit/lib/stack.py b/stgit/lib/stack.py\n> index f9e750e..bdd21b1 100644\n> --- a/stgit/lib/stack.py\n> +++ b/stgit/lib/stack.py\n> @@ -102,8 +102,8 @@ class PatchOrder(object):\n> lambda self, val: self.__set_list('unapplied', val))\n> hidden = property(lambda self: self.__get_list('hidden'),\n> lambda self, val: self.__set_list('hidden', val))\n> - # don't return the hidden patches, these have to be returned explicitly\n> - all = property(lambda self: self.applied + self.unapplied)\n> + all = property(lambda self: self.applied + self.unapplied + self.hidden)\n> + all_visible = property(lambda self: self.applied + self.unapplied)\n> \n> class Patches(object):\n> \"\"\"Creates L{Patch} objects. Makes sure there is only one such object\n\nNice compromise to the debate on whether \"all\" should include hidden\npatches.\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"78706","messageId":"20080605064633.GB23209@diana.vm.bytemark.co.uk","threadId":"13803","inReplyTo":"20080604211325.32531.84968.stgit@localhost.localdomain","subject":"Re: [StGIT PATCH 2/5] Rename Repository.head to Repository.head_ref","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-06-05T06:46:33Z","receivedAt":"2008-06-05T06:46:33Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-06-04 22:13:25 +0100, Catalin Marinas wrote:\n\n> This is to avoid confusion with the Stack.head function which\n> returns a commit object rather than a file name.\n\nA much better name. But you don't fix any callers -- aren't there any?\n\n> The patch also changes the \"new\" function to use stack.head directly\n> rather than via the Repository.refs... object.\n\nOh, here's a caller. But just one.\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"78709","messageId":"20080605070126.GC23209@diana.vm.bytemark.co.uk","threadId":"13803","inReplyTo":"20080604211334.32531.74258.stgit@localhost.localdomain","subject":"Re: [StGIT PATCH 3/5] Create a git.Branch class as ancestor of stack.Stack","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-06-05T07:01:26Z","receivedAt":"2008-06-05T07:01:26Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-06-04 22:13:35 +0100, Catalin Marinas wrote:\n\n> This class deals with Git-specific branch commands. The Stack class\n> is a direct child of Branch and some of its functionality was moved\n> to the new class.\n\nLooks good except for a few minor points.\n\n> @@ -619,3 +626,47 @@ class IndexAndWorktree(RunWithEnvCwd):\n> def update_index(self, files):\n> self.run(['git', 'update-index', '--remove', '-z', '--stdin']\n> ).input_nulterm(files).discard_output()\n> +\n> +class Branch(object):\n> + \"\"\"Represents Git branch.\"\"\"\n\nMissing \"a\".\n\n> + def __init__(self, repository, name):\n> + self._repository = repository\n> + self._name = name\n\nUsing double leading underscores will give you a class-local name. Why\njust one?\n\n> + @classmethod\n> + def create(cls, repository, name, create_at = None):\n\nMinor nit: in case of things that are clearly \"owned\" by other things\n(git objects are owned by a git repository, for example), I've tried\nto put the creation function in the owner, not the owned. I'm not\ninsisting, though.\n\n> + \"\"\"Create a new Git branch and return the corresponding L{Branch} object.\"\"\"\n\nLong line.\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"78712","messageId":"20080605072822.GD23209@diana.vm.bytemark.co.uk","threadId":"13803","inReplyTo":"20080604211343.32531.41429.stgit@localhost.localdomain","subject":"Re: [StGIT PATCH 4/5] Add stack creation and initialisation support to lib.Stack","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-06-05T07:28:22Z","receivedAt":"2008-06-05T07:28:22Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-06-04 22:13:43 +0100, Catalin Marinas wrote:\n\n> This patch adds the create and initialise Stack classmethods to\n> handle the initialisation of StGIT patch series on a Git branch.\n\n> diff --git a/stgit/lib/stack.py b/stgit/lib/stack.py\n> index aca7a36..7375d41 100644\n> --- a/stgit/lib/stack.py\n> +++ b/stgit/lib/stack.py\n> @@ -3,6 +3,10 @@\n> import os.path\n> from stgit import exception, utils\n> from stgit.lib import git, stackupgrade\n> +from stgit.config import config\n> +\n> +class StackException(exception.StgException):\n> + \"\"\"Exception raised by stack objects.\"\"\"\n\ns/stack/L{Stack}/, perhaps?\n\n> @@ -105,6 +109,14 @@ class PatchOrder(object):\n> all = property(lambda self: self.applied + self.unapplied + self.hidden)\n> all_visible = property(lambda self: self.applied + self.unapplied)\n> \n> + @staticmethod\n> + def create(stackdir):\n> + \"\"\"Create the PatchOrder specific files\n> + \"\"\"\n> + utils.create_empty_file(os.path.join(stackdir, 'applied'))\n> + utils.create_empty_file(os.path.join(stackdir, 'unapplied'))\n> + utils.create_empty_file(os.path.join(stackdir, 'hidden'))\n> +\n> class Patches(object):\n> \"\"\"Creates L{Patch} objects. Makes sure there is only one such object\n> per patch.\"\"\"\n\nWouldn't it be more consistent if the create function actually\nreturned a PatchOrder object, like other creation functions? (You\nmight even consider having these files auto-created whenever you\ninstantiate a PatchOrder object and they don't yet exist.)\n\nAlso, the creation function might instead live in the Stack class,\nsince it owns the patch order.\n\n> @@ -133,12 +145,14 @@ class Patches(object):\n> class Stack(git.Branch):\n> \"\"\"Represents an StGit stack (that is, a git branch with some extra\n> metadata).\"\"\"\n> + __repo_subdir = 'patches'\n> +\n\nThis needs to be in the previous patch, I think, since you use it\nthere.\n\n> + def set_parents(self, remote, localbranch):\n> + if not localbranch:\n> + return\n> + if remote:\n> + self.set_parent_remote(remote)\n> + self.set_parent_branch(localbranch)\n> + config.set('branch.%s.stgit.parentbranch' % self._name, localbranch)\n\nHmm, I don't quite follow. Why is this a no-op if you give a false\nlocalbranch? And why is branch..stgit.parentbranch needed,\nwhen it's always the same as branch..merge? (Backwards\ncompatibility? Would you mind making a comment about that, in that\ncase?)\n\n> + @classmethod\n> + def initialise(cls, repository, name = None):\n> + \"\"\"Initialise a Git branch to handle patch series.\"\"\"\n> + if not name:\n> + name = repository.current_branch_name\n> + # make sure that the corresponding Git branch exists\n> + git.Branch(repository, name)\n> +\n> + dir = os.path.join(repository.directory, cls.__repo_subdir, name)\n> + compat_dir = os.path.join(dir, 'patches')\n> + if os.path.exists(dir):\n> + raise StackException('%s: branch already initialized' % name)\n> +\n> + # create the stack directory and files\n> + utils.create_dirs(dir)\n> + utils.create_dirs(compat_dir)\n> + PatchOrder.create(dir)\n> + config.set(stackupgrade.format_version_key(name),\n> + str(stackupgrade.FORMAT_VERSION))\n> +\n> + return repository.get_stack(name)\n\nThis is not quite like the other \"create\" functions, since it just\npromotes a branch, without really creating it.\n\nWhat I'd really like to see here, I think, is something like this:\n\n 1. You get a Stack object from some stack.Repository method.\n\n 2. The Stack object works without having to be initialized, but the\n operations that need initialization throw an exception.\n\n 3. The Stack object has an initialize() method -- just a normal\n method, not a class method.\n\nThis will pave the way for automatic initialization -- just call\nself.initialize() instead of throwing an exception in step (2).\n\nWhat do you think?\n\n> +\n> + @classmethod\n> + def create(cls, repository, name,\n> + create_at = None, parent_remote = None, parent_branch = None):\n> + \"\"\"Create and initialise a Git branch returning the L{Stack} object.\"\"\"\n> + git.Branch.create(repository, name, create_at = create_at)\n> + stack = cls.initialise(repository, name)\n> + stack.set_parents(parent_remote, parent_branch)\n> + return stack\n\nSame point as with the other creation functions.\n\nAnd I'd appreciate some documentation on what the parameters mean --\neither here, or in the methods you call from here.\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"78713","messageId":"20080605073429.GE23209@diana.vm.bytemark.co.uk","threadId":"13803","inReplyTo":"20080604211352.32531.10821.stgit@localhost.localdomain","subject":"Re: [StGIT PATCH 5/5] Add stack creation and deletion support to the new infrastructure","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-06-05T07:34:29Z","receivedAt":"2008-06-05T07:34:29Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"The commit message is misleading. What this patch does is make \"stg\ninit\" _use_ the new-infrastructure stack init support added in your\nprevious patches.\n\nOn 2008-06-04 22:13:52 +0100, Catalin Marinas wrote:\n\n> diff --git a/stgit/commands/init.py b/stgit/commands/init.py\n> index 475a4ce..b68acd7 100644\n> --- a/stgit/commands/init.py\n> +++ b/stgit/commands/init.py\n> @@ -16,13 +16,8 @@ along with this program; if not, write to the Free Software\n> Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA\n> \"\"\"\n> \n> -import sys, os\n> -from optparse import OptionParser, make_option\n> -\n> -from stgit.commands.common import *\n> -from stgit.utils import *\n> -from stgit import stack, git\n> -\n> +from stgit.commands import common\n> +from stgit.lib import stack\n\nLots of imports gone, and the remaining ones aren't spilled directly\ninto the local namespace. Very nice.\n\n> - crt_series.init()\n> + stack.Stack.initialise(directory.repository)\n\nThis would need minor tweaking if you decide to take my advice for\n4/5.\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"78714","messageId":"20080605073805.GF23209@diana.vm.bytemark.co.uk","threadId":"13803","inReplyTo":"20080604210655.32531.82580.stgit@localhost.localdomain","subject":"Re: [StGIT PATCH 0/5] Various updates to the new infrastructure","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-06-05T07:38:05Z","receivedAt":"2008-06-05T07:38:05Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-06-04 22:13:08 +0100, Catalin Marinas wrote:\n\n> I'll also post these patches on the list.\n\nI hope my copious whining doesn't make you regret it. ;-)\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"78732","messageId":"b0943d9e0806050243j141a5998j4800f633ba471927@mail.gmail.com","threadId":"13803","inReplyTo":"20080605073429.GE23209@diana.vm.bytemark.co.uk","subject":"Re: [StGIT PATCH 5/5] Add stack creation and deletion support to the new infrastructure","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-06-05T09:43:40Z","receivedAt":"2008-06-05T09:43:40Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2008/6/5 Karl Hasselström :\n> The commit message is misleading. What this patch does is make \"stg\n> init\" _use_ the new-infrastructure stack init support added in your\n> previous patches.\n\nYes, it's misleading. I had a big patch containing the previous 3-4\npatches and split it afterwards. This being the last one, I forgot to\nchange the commit message.\n\n> On 2008-06-04 22:13:52 +0100, Catalin Marinas wrote:\n>> - crt_series.init()\n>> + stack.Stack.initialise(directory.repository)\n>\n> This would need minor tweaking if you decide to take my advice for\n> 4/5.\n\nI'll reply to 4/5 separately.\n\n-- \nCatalin\n"},{"id":"78748","messageId":"b0943d9e0806050446w27fc5482i9e96a0f5201586c@mail.gmail.com","threadId":"13803","inReplyTo":"20080605064152.GA23209@diana.vm.bytemark.co.uk","subject":"Re: [StGIT PATCH 1/5] Allow stack.patchorder.all to return hidden patches","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-06-05T11:46:01Z","receivedAt":"2008-06-05T11:46:01Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2008/6/5 Karl Hasselström :\n> On 2008-06-04 22:13:17 +0100, Catalin Marinas wrote:\n>> if bad:\n>> raise common.CmdException('Bad patch names: %s'\n>\n> Forbidding commit to operate on hidden patches? OK, why not? But\n> saying \"Bad patch name\" if the user gives a hidden patch is probably a\n> bit on the grumpy side ... :-)\n\nOK. Changed it to \"Nonexistent or hidden patch names\". Could be done\nbetter but I wouldn't bother too much with this error.\n\n-- \nCatalin\n"},{"id":"78749","messageId":"b0943d9e0806050449s4b55d06cu1b7701a562c7a6ba@mail.gmail.com","threadId":"13803","inReplyTo":"20080605064633.GB23209@diana.vm.bytemark.co.uk","subject":"Re: [StGIT PATCH 2/5] Rename Repository.head to Repository.head_ref","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-06-05T11:49:15Z","receivedAt":"2008-06-05T11:49:15Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2008/6/5 Karl Hasselström :\n> On 2008-06-04 22:13:25 +0100, Catalin Marinas wrote:\n>\n>> This is to avoid confusion with the Stack.head function which\n>> returns a commit object rather than a file name.\n>\n> A much better name. But you don't fix any callers -- aren't there any?\n\nThere was actually another caller but missed it since I worked on a\nbigger initially. The stack.Repository.current_branch uses it but this\nwas later moved to git.Repository.current_branch_name. I'll add in\ncase we ever need to bisect.\n\n-- \nCatalin\n"},{"id":"78750","messageId":"20080605115820.GA27430@diana.vm.bytemark.co.uk","threadId":"13803","inReplyTo":"b0943d9e0806050449s4b55d06cu1b7701a562c7a6ba@mail.gmail.com","subject":"Re: [StGIT PATCH 2/5] Rename Repository.head to Repository.head_ref","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-06-05T11:58:20Z","receivedAt":"2008-06-05T11:58:20Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-06-05 12:49:15 +0100, Catalin Marinas wrote:\n\n> 2008/6/5 Karl Hasselström :\n>\n> > A much better name. But you don't fix any callers -- aren't there any?\n>\n> There was actually another caller but missed it since I worked on a\n> bigger initially. The stack.Repository.current_branch uses it but\n> this was later moved to git.Repository.current_branch_name. I'll add\n> in case we ever need to bisect.\n\nAh, good.\n\n( Did you run the test suite on your patches? I always do something\n like this (untested -- I have this command in my bash history on\n another computer)\n\n $ stg pop -a\n $ while make test; do stg push || break; done\n\n before sending out patches. It takes a few minutes per patch, but\n it's fully automatic. )\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"78752","messageId":"b0943d9e0806050503x40191104ye332d6a8435811a5@mail.gmail.com","threadId":"13803","inReplyTo":"20080605070126.GC23209@diana.vm.bytemark.co.uk","subject":"Re: [StGIT PATCH 3/5] Create a git.Branch class as ancestor of stack.Stack","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-06-05T12:03:55Z","receivedAt":"2008-06-05T12:03:55Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2008/6/5 Karl Hasselström :\n> On 2008-06-04 22:13:35 +0100, Catalin Marinas wrote:\n>> + def __init__(self, repository, name):\n>> + self._repository = repository\n>> + self._name = name\n>\n> Using double leading underscores will give you a class-local name. Why\n> just one?\n\nJust a convention: double underscore - private, simple underscore -\nprotected (using C++ or Java naming). The _repository or _name are\nmeant to be made available to a derived class (stack.Stack in our\ncase), though we have the corresponding public properties already.\n\n>> + @classmethod\n>> + def create(cls, repository, name, create_at = None):\n>\n> Minor nit: in case of things that are clearly \"owned\" by other things\n> (git objects are owned by a git repository, for example), I've tried\n> to put the creation function in the owner, not the owned. I'm not\n> insisting, though.\n\nAs I see these things, the git.Repository and the derived\nstack.Repository are indeed object factories but they are mainly\nresponsible for creating/returning a new Python object and passing\nsome arguments to the class' __init__. The Repository object doesn't\nneed to know more than the name as the rest is handled by __init__.\n\nThe same goes for the \"create\" function, the repository shouldn't need\nto know the directory or files structure that a Stack or Branch are\nusing. I think we get a clearer separation this way.\n\n-- \nCatalin\n"},{"id":"78753","messageId":"b0943d9e0806050506u25c0441cy86caaec708ba838f@mail.gmail.com","threadId":"13803","inReplyTo":"20080605115820.GA27430@diana.vm.bytemark.co.uk","subject":"Re: [StGIT PATCH 2/5] Rename Repository.head to Repository.head_ref","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-06-05T12:06:39Z","receivedAt":"2008-06-05T12:06:39Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2008/6/5 Karl Hasselström :\n> On 2008-06-05 12:49:15 +0100, Catalin Marinas wrote:\n> ( Did you run the test suite on your patches? I always do something\n> like this (untested -- I have this command in my bash history on\n> another computer)\n>\n> $ stg pop -a\n> $ while make test; do stg push || break; done\n>\n> before sending out patches. It takes a few minutes per patch, but\n> it's fully automatic. )\n\nNot really. It would be good to have a Makefile target for this,\nsomething like \"test_patches\". I'll add it.\n\n-- \nCatalin\n"},{"id":"78756","messageId":"b0943d9e0806050542w663c64a8o93f81503eb811cf9@mail.gmail.com","threadId":"13803","inReplyTo":"20080605072822.GD23209@diana.vm.bytemark.co.uk","subject":"Re: [StGIT PATCH 4/5] Add stack creation and initialisation support to lib.Stack","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-06-05T12:42:03Z","receivedAt":"2008-06-05T12:42:03Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2008/6/5 Karl Hasselström :\n> On 2008-06-04 22:13:43 +0100, Catalin Marinas wrote:\n>\n>> This patch adds the create and initialise Stack classmethods to\n>> handle the initialisation of StGIT patch series on a Git branch.\n>\n>> diff --git a/stgit/lib/stack.py b/stgit/lib/stack.py\n>> index aca7a36..7375d41 100644\n>> --- a/stgit/lib/stack.py\n>> +++ b/stgit/lib/stack.py\n>> @@ -3,6 +3,10 @@\n>> import os.path\n>> from stgit import exception, utils\n>> from stgit.lib import git, stackupgrade\n>> +from stgit.config import config\n>> +\n>> +class StackException(exception.StgException):\n>> + \"\"\"Exception raised by stack objects.\"\"\"\n>\n> s/stack/L{Stack}/, perhaps?\n\nI was more referring to objects in the lib.stack module, not only the\nStack class. Can this be expressed in any way with the epydoc format?\n\n>> @@ -105,6 +109,14 @@ class PatchOrder(object):\n>> all = property(lambda self: self.applied + self.unapplied + self.hidden)\n>> all_visible = property(lambda self: self.applied + self.unapplied)\n>>\n>> + @staticmethod\n>> + def create(stackdir):\n>> + \"\"\"Create the PatchOrder specific files\n>> + \"\"\"\n>> + utils.create_empty_file(os.path.join(stackdir, 'applied'))\n>> + utils.create_empty_file(os.path.join(stackdir, 'unapplied'))\n>> + utils.create_empty_file(os.path.join(stackdir, 'hidden'))\n>> +\n>> class Patches(object):\n>> \"\"\"Creates L{Patch} objects. Makes sure there is only one such object\n>> per patch.\"\"\"\n>\n> Wouldn't it be more consistent if the create function actually\n> returned a PatchOrder object, like other creation functions?\n\nIt is a bit more complicated. PatchOrder requires a Stack object in\n__init__ but the stack is not fully set up at this point.\n\n> (You\n> might even consider having these files auto-created whenever you\n> instantiate a PatchOrder object and they don't yet exist.)\n\nThis is definitely a solution but at the moment we have to support the\nold infrastructure which fails if the files aren't present. Once we\ndon't have any reference to the stgit.stack module, we can do this\nmore dynamically.\n\n> Also, the creation function might instead live in the Stack class,\n> since it owns the patch order.\n\nSee my reply to a previous patch with my view on ownership and\nseparation of functionality.\n\n>> @@ -133,12 +145,14 @@ class Patches(object):\n>> class Stack(git.Branch):\n>> \"\"\"Represents an StGit stack (that is, a git branch with some extra\n>> metadata).\"\"\"\n>> + __repo_subdir = 'patches'\n>> +\n>\n> This needs to be in the previous patch, I think, since you use it\n> there.\n\nYes.\n\n>> + def set_parents(self, remote, localbranch):\n>> + if not localbranch:\n>> + return\n>> + if remote:\n>> + self.set_parent_remote(remote)\n>> + self.set_parent_branch(localbranch)\n>> + config.set('branch.%s.stgit.parentbranch' % self._name, localbranch)\n>\n> Hmm, I don't quite follow. Why is this a no-op if you give a false\n> localbranch? And why is branch..stgit.parentbranch needed,\n> when it's always the same as branch..merge? (Backwards\n> compatibility? Would you mind making a comment about that, in that\n> case?)\n\nI don't fully follow it either :-). I just copied the code from\nstgit.stack.Series but I don't make any use of it yet. I think it was\ninitially written by Yann Dirson for the \"branch\" command which I\nhaven't translated yet. I think branch..stgit.parentbranch\nmight be used by the current \"pull\" or \"rebase\" implementations. I'll\ncomment it out (but still keep it for now) and put a FIXME so that I\nremember when converting the \"branch\" and \"pull\" commands.\n\n>> + @classmethod\n>> + def initialise(cls, repository, name = None):\n>> + \"\"\"Initialise a Git branch to handle patch series.\"\"\"\n>> + if not name:\n>> + name = repository.current_branch_name\n>> + # make sure that the corresponding Git branch exists\n>> + git.Branch(repository, name)\n>> +\n>> + dir = os.path.join(repository.directory, cls.__repo_subdir, name)\n>> + compat_dir = os.path.join(dir, 'patches')\n>> + if os.path.exists(dir):\n>> + raise StackException('%s: branch already initialized' % name)\n>> +\n>> + # create the stack directory and files\n>> + utils.create_dirs(dir)\n>> + utils.create_dirs(compat_dir)\n>> + PatchOrder.create(dir)\n>> + config.set(stackupgrade.format_version_key(name),\n>> + str(stackupgrade.FORMAT_VERSION))\n>> +\n>> + return repository.get_stack(name)\n>\n> This is not quite like the other \"create\" functions, since it just\n> promotes a branch, without really creating it.\n\nThat was the intention. For creation, there is Stack.create which\nhandles the Git branch creation as well as the StGIT initialisation.\n\n> What I'd really like to see here, I think, is something like this:\n>\n> 1. You get a Stack object from some stack.Repository method.\n>\n> 2. The Stack object works without having to be initialized, but the\n> operations that need initialization throw an exception.\n>\n> 3. The Stack object has an initialize() method -- just a normal\n> method, not a class method.\n\nThat was the original behaviour in stgit.stack.Series but if you\nhappen to try to access a patch or patchorder, you get some exception\non missing file form those objects rather than \"stack not initialised\"\nfrom Stack.\n\n> This will pave the way for automatic initialization -- just call\n> self.initialize() instead of throwing an exception in step (2).\n\nI'm not fully convinced with automatic initialisation. I have some\nbranches where I don't use StGIT but I might type a \"stg series\" by\nmistake. The branch will be promoted to a stack but later Git doesn't\nknow about extra files in .git/patches and they aren't handled\n(removed, renamed etc.). I'm more in favour of the explicit\ninitialisation.\n\n>> + @classmethod\n>> + def create(cls, repository, name,\n>> + create_at = None, parent_remote = None, parent_branch = None):\n>> + \"\"\"Create and initialise a Git branch returning the L{Stack} object.\"\"\"\n>> + git.Branch.create(repository, name, create_at = create_at)\n>> + stack = cls.initialise(repository, name)\n>> + stack.set_parents(parent_remote, parent_branch)\n>> + return stack\n>\n> Same point as with the other creation functions.\n>\n> And I'd appreciate some documentation on what the parameters mean --\n> either here, or in the methods you call from here.\n\nYes, I'll add some description.\n\nThanks for your comments.\n\n-- \nCatalin\n"},{"id":"78758","messageId":"20080605124620.GA28995@diana.vm.bytemark.co.uk","threadId":"13803","inReplyTo":"b0943d9e0806050506u25c0441cy86caaec708ba838f@mail.gmail.com","subject":"Re: [StGIT PATCH 2/5] Rename Repository.head to Repository.head_ref","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-06-05T12:46:20Z","receivedAt":"2008-06-05T12:46:20Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-06-05 13:06:39 +0100, Catalin Marinas wrote:\n\n> 2008/6/5 Karl Hasselström :\n>\n> > ( Did you run the test suite on your patches? I always do\n> > something like this (untested -- I have this command in my bash\n> > history on another computer)\n> >\n> > $ stg pop -a\n> > $ while make test; do stg push || break; done\n> >\n> > before sending out patches. It takes a few minutes per patch, but\n> > it's fully automatic. )\n>\n> Not really. It would be good to have a Makefile target for this,\n> something like \"test_patches\". I'll add it.\n\nGood idea. But consider the case where the user doesn't want to test\nall the patches -- maybe you could give make FIRST_PATCH and/or\nLAST_PATCH parameters? (Defaulting to all patches, of course.)\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"78764","messageId":"20080605130415.GB28995@diana.vm.bytemark.co.uk","threadId":"13803","inReplyTo":"b0943d9e0806050503x40191104ye332d6a8435811a5@mail.gmail.com","subject":"Re: [StGIT PATCH 3/5] Create a git.Branch class as ancestor of stack.Stack","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-06-05T13:04:15Z","receivedAt":"2008-06-05T13:04:15Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-06-05 13:03:55 +0100, Catalin Marinas wrote:\n\n> 2008/6/5 Karl Hasselström :\n>\n> > On 2008-06-04 22:13:35 +0100, Catalin Marinas wrote:\n> >\n> > > + def __init__(self, repository, name):\n> > > + self._repository = repository\n> > > + self._name = name\n> >\n> > Using double leading underscores will give you a class-local name.\n> > Why just one?\n>\n> Just a convention: double underscore - private, simple underscore -\n> protected (using C++ or Java naming). The _repository or _name are\n> meant to be made available to a derived class (stack.Stack in our\n> case), though we have the corresponding public properties already.\n\nOK. Then I'd argue that these two should be private since there's no\nparticular reason for them to be just protected, but I think I've done\nenough nitpicking for one day already!\n\n> >> + @classmethod\n> >> + def create(cls, repository, name, create_at = None):\n> >\n> > Minor nit: in case of things that are clearly \"owned\" by other\n> > things (git objects are owned by a git repository, for example),\n> > I've tried to put the creation function in the owner, not the\n> > owned. I'm not insisting, though.\n>\n> As I see these things, the git.Repository and the derived\n> stack.Repository are indeed object factories but they are mainly\n> responsible for creating/returning a new Python object and passing\n> some arguments to the class' __init__. The Repository object doesn't\n> need to know more than the name as the rest is handled by __init__.\n\nYes. This is exactly how it works e.g. when a Repository creates Blob,\nTree, or Commit objects.\n\n> The same goes for the \"create\" function, the repository shouldn't\n> need to know the directory or files structure that a Stack or Branch\n> are using. I think we get a clearer separation this way.\n\nYes, you're right. Knowledge of internals belong in the class itself.\n\nWe'll have to be watchful against attempts to create objects other\nthan via the officially designated factories, though. Python doesn't\nreally have any mechanisms that help us here.\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"78881","messageId":"b0943d9e0806060144x48176566m277ba2969087b91e@mail.gmail.com","threadId":"13803","inReplyTo":"20080605130415.GB28995@diana.vm.bytemark.co.uk","subject":"Re: [StGIT PATCH 3/5] Create a git.Branch class as ancestor of stack.Stack","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-06-06T08:44:37Z","receivedAt":"2008-06-06T08:44:37Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2008/6/5 Karl Hasselström :\n> On 2008-06-05 13:03:55 +0100, Catalin Marinas wrote:\n>> 2008/6/5 Karl Hasselström :\n>> The same goes for the \"create\" function, the repository shouldn't\n>> need to know the directory or files structure that a Stack or Branch\n>> are using. I think we get a clearer separation this way.\n>\n> Yes, you're right. Knowledge of internals belong in the class itself.\n>\n> We'll have to be watchful against attempts to create objects other\n> than via the officially designated factories, though. Python doesn't\n> really have any mechanisms that help us here.\n\nDoesn't need to be enforced. The Stack and Branch objects can be\ninitialised by themselves provided that the repository is passed as\nargument at construction. The factory is more for convenience, passing\nthe repository to the newly created object.\n\nIf you really want enforcing this, it can probably be done by hacking\n__new__ and __init__.\n\n-- \nCatalin\n"},{"id":"79027","messageId":"20080607085949.GA32647@diana.vm.bytemark.co.uk","threadId":"13803","inReplyTo":"b0943d9e0806050542w663c64a8o93f81503eb811cf9@mail.gmail.com","subject":"Re: [StGIT PATCH 4/5] Add stack creation and initialisation support to lib.Stack","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-06-07T08:59:49Z","receivedAt":"2008-06-07T08:59:49Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-06-05 13:42:03 +0100, Catalin Marinas wrote:\n\n> 2008/6/5 Karl Hasselström :\n>\n> > On 2008-06-04 22:13:43 +0100, Catalin Marinas wrote:\n> >\n> > > +from stgit.config import config\n> > > +\n> > > +class StackException(exception.StgException):\n> > > + \"\"\"Exception raised by stack objects.\"\"\"\n> >\n> > s/stack/L{Stack}/, perhaps?\n>\n> I was more referring to objects in the lib.stack module, not only\n> the Stack class. Can this be expressed in any way with the epydoc\n> format?\n\nI think L{lib.stack} would work.\n\nSee http://epydoc.sourceforge.net/manual-epytext.html.\n\n> > Wouldn't it be more consistent if the create function actually\n> > returned a PatchOrder object, like other creation functions?\n>\n> It is a bit more complicated. PatchOrder requires a Stack object in\n> __init__ but the stack is not fully set up at this point.\n\nI see. How about calling the method \"create_files\" or something, then,\nto avoid breaking with the newly established convention of having the\ncreate() function create new underlying data and returning a Python\nobject that represents it?\n\n> > (You might even consider having these files auto-created whenever\n> > you instantiate a PatchOrder object and they don't yet exist.)\n>\n> This is definitely a solution but at the moment we have to support\n> the old infrastructure which fails if the files aren't present. Once\n> we don't have any reference to the stgit.stack module, we can do\n> this more dynamically.\n\nRight.\n\n> > Also, the creation function might instead live in the Stack class,\n> > since it owns the patch order.\n>\n> See my reply to a previous patch with my view on ownership and\n> separation of functionality.\n\nYes, I agree with you now.\n\n> > > + def set_parents(self, remote, localbranch):\n> > > + if not localbranch:\n> > > + return\n> > > + if remote:\n> > > + self.set_parent_remote(remote)\n> > > + self.set_parent_branch(localbranch)\n> > > + config.set('branch.%s.stgit.parentbranch' % self._name, localbranch)\n> >\n> > Hmm, I don't quite follow. Why is this a no-op if you give a false\n> > localbranch? And why is branch..stgit.parentbranch\n> > needed, when it's always the same as branch..merge?\n> > (Backwards compatibility? Would you mind making a comment about\n> > that, in that case?)\n>\n> I don't fully follow it either :-).\n\nHmmm. :-)\n\n> I just copied the code from stgit.stack.Series but I don't make any\n> use of it yet. I think it was initially written by Yann Dirson for\n> the \"branch\" command which I haven't translated yet. I think\n> branch..stgit.parentbranch might be used by the current\n> \"pull\" or \"rebase\" implementations. I'll comment it out (but still\n> keep it for now) and put a FIXME so that I remember when converting\n> the \"branch\" and \"pull\" commands.\n\nIn cases like this, when one wants to keep existing behavior unchanged\nbut doesn't quite understand the original reasons behind it, I like to\nsimply insert a big comment saying that this piece of code does this\nstrange thing for reasons unknown to me, but I kept them that way\nbecause I didn't want to change the behavior at the moment.\n\n( As an aside, I've also been thinking about pull/rebase. Let's have a\n discussion about that -- in another thread. )\n\n> > What I'd really like to see here, I think, is something like this:\n> >\n> > 1. You get a Stack object from some stack.Repository method.\n> >\n> > 2. The Stack object works without having to be initialized, but\n> > the operations that need initialization throw an exception.\n> >\n> > 3. The Stack object has an initialize() method -- just a normal\n> > method, not a class method.\n>\n> That was the original behaviour in stgit.stack.Series but if you\n> happen to try to access a patch or patchorder, you get some\n> exception on missing file form those objects rather than \"stack not\n> initialised\" from Stack.\n\nThis wouldn't be a problem if all methods that needed initialization\nbegain with a self.make_sure_were_initialized() call, which could\nthrow a suitable exception. (Though I see how this could get a touch\nunwieldy -- it's probably not something you'd want to do unless you\n_are_ shooting for auto-initialization.)\n\n> I'm not fully convinced with automatic initialisation. I have some\n> branches where I don't use StGIT but I might type a \"stg series\" by\n> mistake. The branch will be promoted to a stack but later Git\n> doesn't know about extra files in .git/patches and they aren't\n> handled (removed, renamed etc.). I'm more in favour of the explicit\n> initialisation.\n\nAutomatic initialization should work such that \"initialize\" is a\nno-op. For example, the \"applied\" and \"unapplied\" files shouldn't even\nexist if there are no patch names in them. That way, StGit metadata\nfiles (and refs) are only created when there's an actual need for them\n-- meaning that read-only StGit commands would never create any such\nfiles.\n\nRelated to this: Have you looked in any detail at the \"undo\" stuff in\nmy experimental branch? The patch stack log that makes undo possible\ncontains all the metadata that StGit needs, accessible from a single\nref. I currently keep writing all the old metadata files as well, but\nmy long-term evil plan is to have only the stack log and nothing else.\nIf not earlier, I'd like to propose a move to auto-init at the same\ntime.\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"79028","messageId":"20080607090636.GB32647@diana.vm.bytemark.co.uk","threadId":"13803","inReplyTo":"b0943d9e0806060144x48176566m277ba2969087b91e@mail.gmail.com","subject":"Re: [StGIT PATCH 3/5] Create a git.Branch class as ancestor of stack.Stack","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-06-07T09:06:36Z","receivedAt":"2008-06-07T09:06:36Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-06-06 09:44:37 +0100, Catalin Marinas wrote:\n\n> 2008/6/5 Karl Hasselström :\n>\n> > We'll have to be watchful against attempts to create objects other\n> > than via the officially designated factories, though. Python\n> > doesn't really have any mechanisms that help us here.\n>\n> Doesn't need to be enforced. The Stack and Branch objects can be\n> initialised by themselves provided that the repository is passed as\n> argument at construction. The factory is more for convenience,\n> passing the repository to the newly created object.\n\nThose classes cache stuff. So if you have, say, two PatchOrder objects\nrepresenting the same underlying files, an update in one will not be\nreflected in the other. An important role of the factory is to make\nsure that if an object is asked for twice, the _same_ object is\nreturned to both callers.\n\nThe Commit, Tree, and Blob objects produced by Repository are another\nexample where we rely on there being no more than one Python object\nrepresenting the same underlying data.\n\n> If you really want enforcing this, it can probably be done by\n> hacking __new__ and __init__.\n\nNo, I reckon you're right -- it's overkill. StGit is small enough that\nwe can probably enforce the relevant restrictions by hand.\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"79175","messageId":"b0943d9e0806081516y575faba9g8b5a27646615f72f@mail.gmail.com","threadId":"13803","inReplyTo":"20080607090636.GB32647@diana.vm.bytemark.co.uk","subject":"Re: [StGIT PATCH 3/5] Create a git.Branch class as ancestor of stack.Stack","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2008-06-08T22:16:02Z","receivedAt":"2008-06-08T22:16:02Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"On 07/06/2008, Karl Hasselström wrote:\n> On 2008-06-06 09:44:37 +0100, Catalin Marinas wrote:\n> > 2008/6/5 Karl Hasselström :\n> > > We'll have to be watchful against attempts to create objects other\n> > > than via the officially designated factories, though. Python\n> > > doesn't really have any mechanisms that help us here.\n> >\n> > If you really want enforcing this, it can probably be done by\n> > hacking __new__ and __init__.\n>\n> No, I reckon you're right -- it's overkill. StGit is small enough that\n> we can probably enforce the relevant restrictions by hand.\n\nHidden plan - once the new stgit.lib.git module is complete, we can\nadvertise it as a stand-alone object-oriented Git wrapper for Python\n:-). At that point, we can think about enforcing the factory\nmechanism.\n\n-- \nCatalin\n"},{"id":"79191","messageId":"402731c90806081707m384edef4p4c2328ac1ee1ea08@mail.gmail.com","threadId":"13803","inReplyTo":"b0943d9e0806081516y575faba9g8b5a27646615f72f@mail.gmail.com","subject":"Re: [StGIT PATCH 3/5] Create a git.Branch class as ancestor of stack.Stack","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2008-06-09T00:07:36Z","receivedAt":"2008-06-09T00:07:36Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sun, Jun 8, 2008 at 3:16 PM, Catalin Marinas\n wrote:\n> On 07/06/2008, Karl Hasselström wrote:\n>> On 2008-06-06 09:44:37 +0100, Catalin Marinas wrote:\n>> > 2008/6/5 Karl Hasselström :\n>> > > We'll have to be watchful against attempts to create objects other\n>> > > than via the officially designated factories, though. Python\n>> > > doesn't really have any mechanisms that help us here.\n>> >\n>> > If you really want enforcing this, it can probably be done by\n>> > hacking __new__ and __init__.\n>>\n>> No, I reckon you're right -- it's overkill. StGit is small enough that\n>> we can probably enforce the relevant restrictions by hand.\n>\n> Hidden plan - once the new stgit.lib.git module is complete, we can\n> advertise it as a stand-alone object-oriented Git wrapper for Python\n> :-). At that point, we can think about enforcing the factory\n> mechanism.\n>\n> --\n> Catalin\n\nSurely you don't want people to have to install stgit just to \"import git\"?\n(or perhaps you do, since that'd give people incentive to install stgit ;))\n\nBack to the point...\n\nA few folks are working towards an OO git interface for python.\nPerhaps joining forces would be a big win for everyone?\n\nhttp://gitorious.org/projects/git-python\n\nIt would be really cool if we could wrangle in all interested parties\ntowards a common goal for python. You guys (the stgit team) have a\nlot of experience in this domain so your ideas and opinions would be\ngreatly appreciated.\n\n-- \n David\n"},{"id":"79194","messageId":"bd6139dc0806081746q3b71dd3cp5dfef55184508930@mail.gmail.com","threadId":"13803","inReplyTo":"402731c90806081707m384edef4p4c2328ac1ee1ea08@mail.gmail.com","subject":"Re: [StGIT PATCH 3/5] Create a git.Branch class as ancestor of stack.Stack","fromName":"Sverre Rabbelier","fromEmail":"alturin@gmail.com","sentAt":"2008-06-09T00:46:18Z","receivedAt":"2008-06-09T00:46:18Z","isPatch":true,"sender":{"key":"alturin@gmail.com","avatar":null},"body":"On Mon, Jun 9, 2008 at 2:07 AM, David Aguilar wrote:\n> A few folks are working towards an OO git interface for python.\n> Perhaps joining forces would be a big win for everyone?\n>\n> http://gitorious.org/projects/git-python\n>\n> It would be really cool if we could wrangle in all interested parties\n> towards a common goal for python. You guys (the stgit team) have a\n> lot of experience in this domain so your ideas and opinions would be\n> greatly appreciated.\n\nI very much agree with you. I'm using git-python for my GSoC project\nmyself (although currently I'm only using the Git object really). I\nthink it would be awesome if git-python and stgit.lib.git would merge.\nI'd be happy to help with that in any way I can :).\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"79206","messageId":"20080609070722.GA4100@diana.vm.bytemark.co.uk","threadId":"13803","inReplyTo":"b0943d9e0806081516y575faba9g8b5a27646615f72f@mail.gmail.com","subject":"Re: [StGIT PATCH 3/5] Create a git.Branch class as ancestor of stack.Stack","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-06-09T07:07:22Z","receivedAt":"2008-06-09T07:07:22Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-06-08 23:16:02 +0100, Catalin Marinas wrote:\n\n> Hidden plan - once the new stgit.lib.git module is complete, we can\n> advertise it as a stand-alone object-oriented Git wrapper for Python\n> :-).\n\nAren't there already a couple of those? :-)\n\nThough I agree that having such a future separation in mind is useful\nfor making StGit nice and modular. Whether we actually carry it out in\nthe end would depend on whether someone comes along and actually wants\nto use it, I guess.\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"}]}