{"thread":{"id":"7900","subject":"[StGIT RFC PATCH] Don't use refs/bases/","startedAt":"2007-04-29T22:15:52Z","lastAt":"2007-05-02T13:10:11Z","messageCount":8,"participants":["Karl Hasselström","Catalin Marinas","Marco Costalba"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"40721","messageId":"20070429220832.5832.251.stgit@yoghurt","threadId":"7900","inReplyTo":null,"subject":"[StGIT RFC PATCH] Don't use refs/bases/","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2007-04-29T22:15:52Z","receivedAt":"2007-04-29T22:15:52Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"From: Karl Hasselström \n\nIt's silly to save the stack base in a ref when it can trivially be\ncomputed from the bottommost applied patch, if any. (If there are no\napplied patches, it's simply equal to HEAD.)\n\nSigned-off-by: Karl Hasselström \n---\n\nStGIT maintains too much metadata that can be inferred by asking git\nthe right questions. There are two downsides with this:\n\n * With more metadata, the design gets more complicated and harder to\n understand.\n\n * The duplicated metadata can get out of sync, and that situation\n has to be taken care of (otherwise, StGIT breaks). For example,\n there was code to set the base to HEAD whenever there were no\n patches applied and base != HEAD. With this patch, the data is\n only stored in one place and can never get stale in the first\n place.\n\nIf there are no objections, I'll probably send more patches to\neliminate more redundant metadata.\n\n Documentation/tutorial.txt | 3 --\n stgit/commands/uncommit.py | 5 +---\n stgit/stack.py | 53 ++++++--------------------------------------\n t/t1000-branch-create.sh | 14 ------------\n t/t1200-push-modified.sh | 7 +++---\n t/t1201-pull-trailing.sh | 7 +++---\n t/t2200-rebase.sh | 2 +-\n 7 files changed, 17 insertions(+), 74 deletions(-)\n\ndiff --git a/Documentation/tutorial.txt b/Documentation/tutorial.txt\nindex 5899c38..2b8e4e7 100644\n--- a/Documentation/tutorial.txt\n+++ b/Documentation/tutorial.txt\n@@ -387,9 +387,6 @@ Layout of the .git directory\n heads/\n master\t\t- the master commit id\n ...\n- bases/\n- master\t\t- the bottom id of the stack (to get a big diff)\n- ...\n tags/\n ...\n branches/\ndiff --git a/stgit/commands/uncommit.py b/stgit/commands/uncommit.py\nindex 0ee1585..462846c 100644\n--- a/stgit/commands/uncommit.py\n+++ b/stgit/commands/uncommit.py\n@@ -80,11 +80,9 @@ def func(parser, options, args):\n print 'Uncommitting %d patches...' % patch_nr,\n sys.stdout.flush()\n \n- base_file = crt_series.get_base_file()\n-\n for n in xrange(0, patch_nr):\n # retrieve the commit (only commits with a single parent are allowed)\n- commit_id = read_string(base_file)\n+ commit_id = crt_series.get_base()\n commit = git.Commit(commit_id)\n try:\n parent, = commit.get_parents()\n@@ -107,6 +105,5 @@ def func(parser, options, args):\n author_name = author_name,\n author_email = author_email,\n author_date = author_date)\n- write_string(base_file, parent)\n \n print 'done'\ndiff --git a/stgit/stack.py b/stgit/stack.py\nindex b0a01dd..2477ac6 100644\n--- a/stgit/stack.py\n+++ b/stgit/stack.py\n@@ -291,8 +291,6 @@ class Series(StgitObject):\n self._set_dir(os.path.join(self.__base_dir, 'patches', self.__name))\n self.__refs_dir = os.path.join(self.__base_dir, 'refs', 'patches',\n self.__name)\n- self.__base_file = os.path.join(self.__base_dir, 'refs', 'bases',\n- self.__name)\n \n self.__applied_file = os.path.join(self._dir(), 'applied')\n self.__unapplied_file = os.path.join(self._dir(), 'unapplied')\n@@ -378,12 +376,14 @@ class Series(StgitObject):\n f.close()\n return names\n \n- def get_base_file(self):\n- self.__begin_stack_check()\n- return self.__base_file\n-\n def get_base(self):\n- return read_string(self.get_base_file())\n+ # Return the parent of the bottommost patch, if there is one.\n+ if os.path.isfile(self.__applied_file):\n+ bottommost = file(self.__applied_file).readline().strip()\n+ if bottommost:\n+ return self.get_patch(bottommost).get_bottom()\n+ # No bottommost patch, so just return HEAD\n+ return git.get_head()\n \n def get_head(self):\n \"\"\"Return the head of the branch\n@@ -482,22 +482,6 @@ class Series(StgitObject):\n otherwise.\"\"\"\n return self.patch_applied(name) or self.patch_unapplied(name)\n \n- def __begin_stack_check(self):\n- \"\"\"Save the current HEAD into .git/refs/heads/base if the stack\n- is empty\n- \"\"\"\n- if len(self.get_applied()) == 0:\n- head = git.get_head()\n- write_string(self.__base_file, head)\n-\n- def __end_stack_check(self):\n- \"\"\"Remove .git/refs/heads/base if the stack is empty.\n- This warning should never happen\n- \"\"\"\n- if len(self.get_applied()) == 0 \\\n- and read_string(self.__base_file) != git.get_head():\n- print 'Warning: stack empty but the HEAD and base are different'\n-\n def head_top_equal(self):\n \"\"\"Return true if the head and the top are the same\n \"\"\"\n@@ -519,8 +503,6 @@ class Series(StgitObject):\n raise StackException, self.__patch_dir + ' already exists'\n if os.path.exists(self.__refs_dir):\n raise StackException, self.__refs_dir + ' already exists'\n- if os.path.exists(self.__base_file):\n- raise StackException, self.__base_file + ' already exists'\n \n if (create_at!=False):\n git.create_branch(self.__name, create_at)\n@@ -528,15 +510,12 @@ class Series(StgitObject):\n os.makedirs(self.__patch_dir)\n \n self.set_parent(parent_remote, parent_branch)\n- \n- create_dirs(os.path.join(self.__base_dir, 'refs', 'bases'))\n \n self.create_empty_field('applied')\n self.create_empty_field('unapplied')\n self.create_empty_field('description')\n os.makedirs(os.path.join(self._dir(), 'patches'))\n os.makedirs(self.__refs_dir)\n- self.__begin_stack_check()\n self._set_field('orig-base', git.get_head())\n \n def convert(self):\n@@ -582,17 +561,12 @@ class Series(StgitObject):\n \n if to_stack.is_initialised():\n raise StackException, '\"%s\" already exists' % to_stack.get_branch()\n- if os.path.exists(to_stack.__base_file):\n- os.remove(to_stack.__base_file)\n \n git.rename_branch(self.__name, to_name)\n \n if os.path.isdir(self._dir()):\n rename(os.path.join(self.__base_dir, 'patches'),\n self.__name, to_stack.__name)\n- if os.path.exists(self.__base_file):\n- rename(os.path.join(self.__base_dir, 'refs', 'bases'),\n- self.__name, to_stack.__name)\n if os.path.exists(self.__refs_dir):\n rename(os.path.join(self.__base_dir, 'refs', 'patches'),\n self.__name, to_stack.__name)\n@@ -698,10 +672,6 @@ class Series(StgitObject):\n except OSError:\n print 'Refs directory %s is not empty.' % self.__refs_dir\n \n- if os.path.exists(self.__base_file):\n- remove_file_and_dirs(\n- os.path.join(self.__base_dir, 'refs', 'bases'), self.__name)\n-\n # Cleanup parent informations\n # FIXME: should one day make use of git-config --section-remove,\n # scheduled for 1.5.1\n@@ -824,8 +794,6 @@ class Series(StgitObject):\n \n head = git.get_head()\n \n- self.__begin_stack_check()\n-\n patch = Patch(name, self.__patch_dir, self.__refs_dir)\n patch.create()\n \n@@ -893,8 +861,6 @@ class Series(StgitObject):\n if self.patch_hidden(name):\n self.unhide_patch(name)\n \n- self.__begin_stack_check()\n-\n def forward_patches(self, names):\n \"\"\"Try to fast-forward an array of patches.\n \n@@ -902,7 +868,6 @@ class Series(StgitObject):\n stack. Apply the rest with push_patch\n \"\"\"\n unapplied = self.get_unapplied()\n- self.__begin_stack_check()\n \n forwarded = 0\n top = git.get_head()\n@@ -1001,8 +966,6 @@ class Series(StgitObject):\n unapplied = self.get_unapplied()\n assert(name in unapplied)\n \n- self.__begin_stack_check()\n-\n patch = Patch(name, self.__patch_dir, self.__refs_dir)\n \n head = git.get_head()\n@@ -1140,8 +1103,6 @@ class Series(StgitObject):\n else:\n self.__set_current(applied[-1])\n \n- self.__end_stack_check()\n-\n def empty_patch(self, name):\n \"\"\"Returns True if the patch is empty\n \"\"\"\ndiff --git a/t/t1000-branch-create.sh b/t/t1000-branch-create.sh\nindex f0c2367..58209e7 100755\n--- a/t/t1000-branch-create.sh\n+++ b/t/t1000-branch-create.sh\n@@ -39,20 +39,6 @@ test_expect_success \\\n '\n \n test_expect_failure \\\n- 'Try to create an stgit branch with a spurious refs/bases/ entry' \\\n- 'find .git -name foo | xargs rm -rf &&\n- touch .git/refs/bases/foo &&\n- stg branch -c foo\n-'\n-\n-test_expect_success \\\n- 'Check no part of the branch was created' \\\n- 'test \"`find .git -name foo | tee /dev/stderr`\" = \".git/refs/bases/foo\" &&\n- ( grep foo .git/HEAD; test $? = 1 )\n-'\n-\n-\n-test_expect_failure \\\n 'Try to create an stgit branch with an existing git branch by that name' \\\n 'find .git -name foo | xargs rm -rf &&\n cp .git/refs/heads/master .git/refs/heads/foo &&\ndiff --git a/t/t1200-push-modified.sh b/t/t1200-push-modified.sh\nindex 7847a38..6769667 100755\n--- a/t/t1200-push-modified.sh\n+++ b/t/t1200-push-modified.sh\n@@ -30,11 +30,12 @@ test_expect_success \\\n \n test_expect_success \\\n 'Port those patches to orig tree' \\\n- \"(cd foo &&\n- GIT_DIR=../bar/.git git-format-patch --stdout bases/master..HEAD |\n+ '(cd foo &&\n+ GIT_DIR=../bar/.git git-format-patch --stdout \\\n+ $(cd ../bar && stg id base@master)..HEAD |\n git-am -3 -k\n )\n-\"\n+ '\n \n test_expect_success \\\n 'Pull to sync with parent, preparing for the problem' \\\ndiff --git a/t/t1201-pull-trailing.sh b/t/t1201-pull-trailing.sh\nindex 42c6619..46d9f82 100755\n--- a/t/t1201-pull-trailing.sh\n+++ b/t/t1201-pull-trailing.sh\n@@ -28,11 +28,12 @@ test_expect_success \\\n \n test_expect_success \\\n 'Port those patches to orig tree' \\\n- \"(cd foo &&\n- GIT_DIR=../bar/.git git-format-patch --stdout bases/master..HEAD |\n+ '(cd foo &&\n+ GIT_DIR=../bar/.git git-format-patch --stdout \\\n+ $(cd ../bar && stg id base@master)..HEAD |\n git-am -3 -k\n )\n-\"\n+ '\n \n test_expect_success \\\n 'Pull those patches applied upstream, without pushing' \\\ndiff --git a/t/t2200-rebase.sh b/t/t2200-rebase.sh\nindex e2d9d9a..52462dd 100755\n--- a/t/t2200-rebase.sh\n+++ b/t/t2200-rebase.sh\n@@ -27,7 +27,7 @@ test_expect_success \\\n \t'Rebase to previous commit' \\\n \t'\n \tstg rebase master~1 &&\n-\ttest `git rev-parse bases/stack` = `git rev-parse master~1`\n+\ttest `stg id base@stack` = `git rev-parse master~1`\n \t'\n \n test_done\n"},{"id":"40804","messageId":"b0943d9e0705010137q4a35f818m7dbbc9d2e77e2fcf@mail.gmail.com","threadId":"7900","inReplyTo":"20070429220832.5832.251.stgit@yoghurt","subject":"Re: [StGIT RFC PATCH] Don't use refs/bases/","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2007-05-01T08:37:39Z","receivedAt":"2007-05-01T08:37:39Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"On 30/04/07, Karl Hasselström wrote:\n> It's silly to save the stack base in a ref when it can trivially be\n> computed from the bottommost applied patch, if any. (If there are no\n> applied patches, it's simply equal to HEAD.)\n\nThe reason I initially had the base ref was to see what's on top of\nthe stack when using gitk. I later added refs/patches//...\nwhich are shown by gitk and the base ref would be redundant.\n\nI'm OK with this patch as long as tools like qgit don't rely on this ref.\n\n-- \nCatalin\n"},{"id":"40807","messageId":"e5bfff550705010210i352ac9eej6ff7a78aae6535c9@mail.gmail.com","threadId":"7900","inReplyTo":"b0943d9e0705010137q4a35f818m7dbbc9d2e77e2fcf@mail.gmail.com","subject":"Re: [StGIT RFC PATCH] Don't use refs/bases/","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2007-05-01T09:10:47Z","receivedAt":"2007-05-01T09:10:47Z","isPatch":true,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 5/1/07, Catalin Marinas wrote:\n> On 30/04/07, Karl Hasselström wrote:\n> > It's silly to save the stack base in a ref when it can trivially be\n> > computed from the bottommost applied patch, if any. (If there are no\n> > applied patches, it's simply equal to HEAD.)\n>\n> The reason I initially had the base ref was to see what's on top of\n> the stack when using gitk. I later added refs/patches//...\n> which are shown by gitk and the base ref would be redundant.\n>\n> I'm OK with this patch as long as tools like qgit don't rely on this ref.\n>\n\nIt's OK for me. A recent qgit already filters out content of\nrefs/bases to reduce visual 'noise'.\n\nThe only StGit data read directly are patches sha's; qgit walks\nrecursively all the files called \"top\" under directory tree\n\n /patches/\n\nto get sha list of each applied and unapplied patch in one go.\n\nThis is much faster then calling \"stg id \" for all the patches.\n\n Marco\n"},{"id":"40847","messageId":"20070501185615.GA32727@diana.vm.bytemark.co.uk","threadId":"7900","inReplyTo":"e5bfff550705010210i352ac9eej6ff7a78aae6535c9@mail.gmail.com","subject":"Re: [StGIT RFC PATCH] Don't use refs/bases/","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2007-05-01T18:56:15Z","receivedAt":"2007-05-01T18:56:15Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2007-05-01 11:10:47 +0200, Marco Costalba wrote:\n\n> On 5/1/07, Catalin Marinas wrote:\n>\n> > I'm OK with this patch as long as tools like qgit don't rely on\n> > this ref.\n>\n> It's OK for me. A recent qgit already filters out content of\n> refs/bases to reduce visual 'noise'.\n\nGood.\n\n> The only StGit data read directly are patches sha's; qgit walks\n> recursively all the files called \"top\" under directory tree\n>\n> /patches/\n>\n> to get sha list of each applied and unapplied patch in one go. This\n> is much faster then calling \"stg id \" for all the\n> patches.\n\nHmm. These are on my kill list too. :-)\n\nThe patch tops are already recorded in\nrefs/patches// to keep them from being garbage\ncollected, so these top files are redundant. But it isn't _that_ bad,\nso if removing them would break qgit, I guess I could try to restrain\nmyself. At least all the other metadata is fair game. :-)\n\n(But if I were you, I'd look for the patches under patches/refs\nanyway; they _have_ to be there to survive garbage collection, so no\namount of stgit refactoring will break qgit.)\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"40851","messageId":"e5bfff550705011259w567a98dj6d0a68b0fbe90994@mail.gmail.com","threadId":"7900","inReplyTo":"20070501185615.GA32727@diana.vm.bytemark.co.uk","subject":"Re: [StGIT RFC PATCH] Don't use refs/bases/","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2007-05-01T19:59:34Z","receivedAt":"2007-05-01T19:59:34Z","isPatch":true,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 5/1/07, Karl Hasselström wrote:\n> On 2007-05-01 11:10:47 +0200, Marco Costalba wrote:\n>\n> > The only StGit data read directly are patches sha's; qgit walks\n> > recursively all the files called \"top\" under directory tree\n> >\n> > /patches/\n> >\n> > to get sha list of each applied and unapplied patch in one go. This\n> > is much faster then calling \"stg id \" for all the\n> > patches.\n>\n> Hmm. These are on my kill list too. :-)\n>\n> The patch tops are already recorded in\n> refs/patches// to keep them from being garbage\n> collected, so these top files are redundant. But it isn't _that_ bad,\n> so if removing them would break qgit, I guess I could try to restrain\n> myself. At least all the other metadata is fair game. :-)\n>\n> (But if I were you, I'd look for the patches under patches/refs\n> anyway; they _have_ to be there to survive garbage collection, so no\n> amount of stgit refactoring will break qgit.)\n>\n\nWell, I did. ;-)\n\nActually I pushed a patch few hours ago to read patches sha under refs/patches.\n\nThe problem is that the patch (for now) is pushed only for the new\ndevelopment version of qgit, not the stable one and, worse, all the\ncurrently released versions will break if you remove /patches/ directory.\n\nSo please, if possible deprecate /patches/ directory but do\nnot remove for a while, so to let users to upgrade gracefully.\n\nThanks\nMarco\n"},{"id":"40879","messageId":"20070502065054.GA9919@diana.vm.bytemark.co.uk","threadId":"7900","inReplyTo":"e5bfff550705011259w567a98dj6d0a68b0fbe90994@mail.gmail.com","subject":"Re: [StGIT RFC PATCH] Don't use refs/bases/","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2007-05-02T06:50:54Z","receivedAt":"2007-05-02T06:50:54Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2007-05-01 21:59:34 +0200, Marco Costalba wrote:\n\n> On 5/1/07, Karl Hasselström wrote:\n>\n> > (But if I were you, I'd look for the patches under patches/refs\n> > anyway; they _have_ to be there to survive garbage collection, so\n> > no amount of stgit refactoring will break qgit.)\n>\n> Well, I did. ;-)\n>\n> Actually I pushed a patch few hours ago to read patches sha under\n> refs/patches.\n\nGood. This was precisely what I was hoping to scare you into doing!\n:-)\n\n> The problem is that the patch (for now) is pushed only for the new\n> development version of qgit, not the stable one and, worse, all the\n> currently released versions will break if you remove\n> /patches/ directory.\n>\n> So please, if possible deprecate /patches/ directory but do\n> not remove for a while, so to let users to upgrade gracefully.\n\nIt's not near the top of my kill list by any stretch of the\nimagination, so no need to worry. And even if it were, Catalin would\ncertainly stand as a wall of sanity between qgit and my chain saw. :-)\n\nThanks.\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"40889","messageId":"e5bfff550705020417h14f6297fu957eb64e58119770@mail.gmail.com","threadId":"7900","inReplyTo":"20070502065054.GA9919@diana.vm.bytemark.co.uk","subject":"Re: [StGIT RFC PATCH] Don't use refs/bases/","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2007-05-02T11:17:01Z","receivedAt":"2007-05-02T11:17:01Z","isPatch":true,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 5/2/07, Karl Hasselström wrote:\n>\n> It's not near the top of my kill list by any stretch of the\n> imagination, so no need to worry. And even if it were, Catalin would\n> certainly stand as a wall of sanity between qgit and my chain saw. :-)\n>\n\nCurrently I check for the existence of /patches directory as\na quick exit in case a repository does NOT have a StGIT repo on it\n(the common case).\n\nThis avoids a costly and 99% of cases not needed 'stg ' call.\n\nI ask if it will be still a safe check in the long period or it is\nbetter to change the check to something else (as existence of /refs/patches) instead ?\n\nThanks\nMarco\n"},{"id":"40895","messageId":"20070502131011.GA13426@diana.vm.bytemark.co.uk","threadId":"7900","inReplyTo":"e5bfff550705020417h14f6297fu957eb64e58119770@mail.gmail.com","subject":"Re: [StGIT RFC PATCH] Don't use refs/bases/","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2007-05-02T13:10:11Z","receivedAt":"2007-05-02T13:10:11Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2007-05-02 13:17:01 +0200, Marco Costalba wrote:\n\n> On 5/2/07, Karl Hasselström wrote:\n>\n> > It's not near the top of my kill list by any stretch of the\n> > imagination, so no need to worry. And even if it were, Catalin\n> > would certainly stand as a wall of sanity between qgit and my\n> > chain saw. :-)\n>\n> Currently I check for the existence of /patches directory\n> as a quick exit in case a repository does NOT have a StGIT repo on\n> it (the common case).\n>\n> This avoids a costly and 99% of cases not needed 'stg '\n> call.\n>\n> I ask if it will be still a safe check in the long period or it is\n> better to change the check to something else (as existence of\n> /refs/patches) instead ?\n\nI personally have no plan to attempt to remove all of .git/patches --\na lot of data in there is redundant, but certainly not all of it, and\nthere's no compelling reason to move it. But Catalin has the final\nword, of course.\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"}]}