[StGIT PATCH 2/2] Don't touch ref files manually

["Karl Hasselström" <kha@treskal.com>]

Messing with files manually doesn't work if the refs are packed. The
officially preferred way is to use git-update-ref, git-show-ref,
et.al. So do that.

As a consequence of this, we have some small behavior changes:

  * We used to not leave empty directories behind in the refs tree.
    But now that's all in the hands of git-update-ref, which does
    leave them behind. Tests that assumed the old behavior have been
    fixed.

  * We (that is, git-show-ref) no longer distinguish between a ref
    that doesn't exist and a ref that contains garbage. So the tests
    that assumed we'd fail when encountering a spurious ref with
    garbage in it have had to go through attitude readjustment.

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

---

 stgit/commands/branch.py   |    5 +--
 stgit/commands/common.py   |    4 +-
 stgit/git.py               |   79 ++++++++++++++++++++++++++------------------
 stgit/stack.py             |   70 +++++++++++++++++++++------------------
 t/t0001-subdir-branches.sh |    7 ++--
 t/t1000-branch-create.sh   |   30 +----------------
 t/t1001-branch-rename.sh   |    2 +
 7 files changed, 91 insertions(+), 106 deletions(-)

diff --git a/stgit/commands/branch.py b/stgit/commands/branch.py
index 2fb5f59..75a9046 100644
--- a/stgit/commands/branch.py
+++ b/stgit/commands/branch.py
@@ -208,10 +208,7 @@ def func(parser, options, args):
         if len(args) != 0:
             parser.error('incorrect number of arguments')
 
-        branches = []
-        basepath = os.path.join(basedir.get(), 'refs', 'heads')
-        for path, files, dirs in walk_tree(basepath):
-            branches += [os.path.join(path, f) for f in files]
+        branches = git.get_heads()
         branches.sort()
 
         if branches:
diff --git a/stgit/commands/common.py b/stgit/commands/common.py
index 14dbf67..dddee85 100644
--- a/stgit/commands/common.py
+++ b/stgit/commands/common.py
@@ -42,9 +42,7 @@ def parse_rev(rev):
     """Parse a revision specification into its
     patchname@branchname//patch_id parts. If no branch name has a slash
     in it, also accept / instead of //."""
-    files, dirs = list_files_and_dirs(os.path.join(basedir.get(),
-                                                   'refs', 'heads'))
-    if len(dirs) != 0:
+    if '/' in ''.join(git.get_heads()):
         # We have branch names with / in them.
         branch_chars = r'[^@]'
         patch_id_mark = r'//'
diff --git a/stgit/git.py b/stgit/git.py
index 72bf889..832a2dc 100644
--- a/stgit/git.py
+++ b/stgit/git.py
@@ -270,6 +270,13 @@ def local_changes(verbose = True):
     """
     return len(tree_status(verbose = verbose)) != 0
 
+def get_heads():
+    heads = []
+    for line in _output_lines(['git-show-ref', '--heads']):
+        m = re.match('^[0-9a-f]{40} refs/heads/(.+)$', line)
+        heads.append(m.group(1))
+    return heads
+
 # HEAD value cached
 __head = None
 
@@ -294,14 +301,16 @@ def set_head_file(ref):
     # head cache flushing is needed since we might have a different value
     # in the new head
     __clear_head_cache()
-    if __run('git-symbolic-ref HEAD',
-             [os.path.join('refs', 'heads', ref)]) != 0:
+    if __run('git-symbolic-ref HEAD', ['refs/heads/%s' % ref]) != 0:
         raise GitException, 'Could not set head to "%s"' % ref
 
+def set_ref(ref, val):
+    """Point ref at a new commit object."""
+    if __run('git-update-ref', [ref, val]) != 0:
+        raise GitException, 'Could not update %s to "%s".' % (ref, val)
+
 def set_branch(branch, val):
-    """Point branch at a new commit object."""
-    if __run('git-update-ref', [branch, val]) != 0:
-        raise GitException, 'Could not update %s to "%s".' % (branch, val)
+    set_ref('refs/heads/%s' % branch, val)
 
 def __set_head(val):
     """Sets the HEAD value
@@ -309,7 +318,7 @@ def __set_head(val):
     global __head
 
     if not __head or __head != val:
-        set_branch('HEAD', val)
+        set_ref('HEAD', val)
         __head = val
 
     # only allow SHA1 hashes
@@ -335,16 +344,15 @@ def rev_parse(git_id):
     except GitException:
         raise GitException, 'Unknown revision: %s' % git_id
 
+def ref_exists(ref):
+    try:
+        rev_parse(ref)
+        return True
+    except GitException:
+        return False
+
 def branch_exists(branch):
-    """Existence check for the named branch
-    """
-    branch = os.path.join('refs', 'heads', branch)
-    for line in _output_lines('git-rev-parse --symbolic --all 2>&1'):
-        if line.strip() == branch:
-            return True
-        if re.compile('[ |/]'+branch+' ').search(line):
-            raise GitException, 'Bogus branch: %s' % line
-    return False
+    return ref_exists('refs/heads/%s' % branch)
 
 def create_branch(new_branch, tree_id = None):
     """Create a new branch in the git repository
@@ -371,8 +379,7 @@ def switch_branch(new_branch):
     if not branch_exists(new_branch):
         raise GitException, 'Branch "%s" does not exist' % new_branch
 
-    tree_id = rev_parse(os.path.join('refs', 'heads', new_branch)
-                        + '^{commit}')
+    tree_id = rev_parse('refs/heads/%s^{commit}' % new_branch)
     if tree_id != get_head():
         refresh_index()
         if __run('git-read-tree -u -m', [get_head(), tree_id]) != 0:
@@ -383,27 +390,33 @@ def switch_branch(new_branch):
     if os.path.isfile(os.path.join(basedir.get(), 'MERGE_HEAD')):
         os.remove(os.path.join(basedir.get(), 'MERGE_HEAD'))
 
+def delete_ref(ref):
+    if not ref_exists(ref):
+        raise GitException, '%s does not exist' % ref
+    sha1 = _output_one_line(['git-show-ref', '-s', ref])
+    if __run('git-update-ref -d %s %s' % (ref, sha1)):
+        raise GitException, 'Failed to delete ref %s' % ref
+
 def delete_branch(name):
-    """Delete a git branch
-    """
-    if not branch_exists(name):
-        raise GitException, 'Branch "%s" does not exist' % name
-    remove_file_and_dirs(os.path.join(basedir.get(), 'refs', 'heads'),
-                         name)
+    delete_ref('refs/heads/%s' % name)
 
-def rename_branch(from_name, to_name):
-    """Rename a git branch
-    """
-    if not branch_exists(from_name):
-        raise GitException, 'Branch "%s" does not exist' % from_name
-    if branch_exists(to_name):
-        raise GitException, 'Branch "%s" already exists' % to_name
+def rename_ref(from_ref, to_ref):
+    if not ref_exists(from_ref):
+        raise GitException, '"%s" does not exist' % from_ref
+    if ref_exists(to_ref):
+        raise GitException, '"%s" already exists' % to_ref
 
+    sha1 = _output_one_line(['git-show-ref', '-s', from_ref])
+    if __run('git-update-ref %s %s %s' % (to_ref, sha1, '0'*40)):
+        raise GitException, 'Failed to create new ref %s' % to_ref
+    if __run('git-update-ref -d %s %s' % (from_ref, sha1)):
+        raise GitException, 'Failed to delete ref %s' % from_ref
+
+def rename_branch(from_name, to_name):
+    """Rename a git branch."""
+    rename_ref('refs/heads/%s' % from_name, 'refs/heads/%s' % to_name)
     if get_head_file() == from_name:
         set_head_file(to_name)
-    rename(os.path.join(basedir.get(), 'refs', 'heads'),
-           from_name, to_name)
-
     reflog_dir = os.path.join(basedir.get(), 'logs', 'refs', 'heads')
     if os.path.exists(reflog_dir) \
            and os.path.exists(os.path.join(reflog_dir, from_name)):
diff --git a/stgit/stack.py b/stgit/stack.py
index a54a3b2..3880a94 100644
--- a/stgit/stack.py
+++ b/stgit/stack.py
@@ -142,14 +142,16 @@ class StgitObject:
 class Patch(StgitObject):
     """Basic patch implementation
     """
-    def __init__(self, name, series_dir, refs_dir):
+    def __init_refs(self):
+        self.__top_ref = self.__refs_base + '/' + self.__name
+        self.__log_ref = self.__top_ref + '.log'
+
+    def __init__(self, name, series_dir, refs_base):
         self.__series_dir = series_dir
         self.__name = name
         self._set_dir(os.path.join(self.__series_dir, self.__name))
-        self.__refs_dir = refs_dir
-        self.__top_ref_file = os.path.join(self.__refs_dir, self.__name)
-        self.__log_ref_file = os.path.join(self.__refs_dir,
-                                           self.__name + '.log')
+        self.__refs_base = refs_base
+        self.__init_refs()
 
     def create(self):
         os.mkdir(self._dir())
@@ -160,33 +162,31 @@ class Patch(StgitObject):
         for f in os.listdir(self._dir()):
             os.remove(os.path.join(self._dir(), f))
         os.rmdir(self._dir())
-        os.remove(self.__top_ref_file)
-        if os.path.exists(self.__log_ref_file):
-            os.remove(self.__log_ref_file)
+        git.delete_ref(self.__top_ref)
+        if git.ref_exists(self.__log_ref):
+            git.delete_ref(self.__log_ref)
 
     def get_name(self):
         return self.__name
 
     def rename(self, newname):
         olddir = self._dir()
-        old_top_ref_file = self.__top_ref_file
-        old_log_ref_file = self.__log_ref_file
+        old_top_ref = self.__top_ref
+        old_log_ref = self.__log_ref
         self.__name = newname
         self._set_dir(os.path.join(self.__series_dir, self.__name))
-        self.__top_ref_file = os.path.join(self.__refs_dir, self.__name)
-        self.__log_ref_file = os.path.join(self.__refs_dir,
-                                           self.__name + '.log')
+        self.__init_refs()
 
+        git.rename_ref(old_top_ref, self.__top_ref)
+        if git.ref_exists(old_log_ref):
+            git.rename_ref(old_log_ref, self.__log_ref)
         os.rename(olddir, self._dir())
-        os.rename(old_top_ref_file, self.__top_ref_file)
-        if os.path.exists(old_log_ref_file):
-            os.rename(old_log_ref_file, self.__log_ref_file)
 
     def __update_top_ref(self, ref):
-        write_string(self.__top_ref_file, ref)
+        git.set_ref(self.__top_ref, ref)
 
     def __update_log_ref(self, ref):
-        write_string(self.__log_ref_file, ref)
+        git.set_ref(self.__log_ref, ref)
 
     def update_top_ref(self):
         top = self.get_top()
@@ -358,8 +358,7 @@ class Series(PatchSet):
         # initialized, but don't touch it if it isn't.
         self.update_to_current_format_version()
 
-        self.__refs_dir = os.path.join(self._basedir(), 'refs', 'patches',
-                                       self.get_name())
+        self.__refs_base = 'refs/patches/%s' % self.get_name()
 
         self.__applied_file = os.path.join(self._dir(), 'applied')
         self.__unapplied_file = os.path.join(self._dir(), 'unapplied')
@@ -416,20 +415,22 @@ class Series(PatchSet):
         def rm(f):
             if os.path.exists(f):
                 os.remove(f)
+        def rm_ref(ref):
+            if git.ref_exists(ref):
+                git.delete_ref(ref)
 
         # Update 0 -> 1.
         if get_format_version() == 0:
             mkdir(os.path.join(branch_dir, 'trash'))
             patch_dir = os.path.join(branch_dir, 'patches')
             mkdir(patch_dir)
-            refs_dir = os.path.join(self._basedir(), 'refs', 'patches', self.get_name())
-            mkdir(refs_dir)
+            refs_base = 'refs/patches/%s' % self.get_name()
             for patch in (file(os.path.join(branch_dir, 'unapplied')).readlines()
                           + file(os.path.join(branch_dir, 'applied')).readlines()):
                 patch = patch.strip()
                 os.rename(os.path.join(branch_dir, patch),
                           os.path.join(patch_dir, patch))
-                Patch(patch, patch_dir, refs_dir).update_top_ref()
+                Patch(patch, patch_dir, refs_base).update_top_ref()
             set_format_version(1)
 
         # Update 1 -> 2.
@@ -441,7 +442,7 @@ class Series(PatchSet):
                     config.set('branch.%s.description' % self.get_name(), desc)
                 rm(desc_file)
             rm(os.path.join(branch_dir, 'current'))
-            rm(os.path.join(self._basedir(), 'refs', 'bases', self.get_name()))
+            rm_ref('refs/bases/%s' % self.get_name())
             set_format_version(2)
 
         # Make sure we're at the latest version.
@@ -458,7 +459,7 @@ class Series(PatchSet):
     def get_patch(self, name):
         """Return a Patch object for the given name
         """
-        return Patch(name, self.__patch_dir, self.__refs_dir)
+        return Patch(name, self.__patch_dir, self.__refs_base)
 
     def get_current_patch(self):
         """Return a Patch object representing the topmost patch, or
@@ -580,7 +581,7 @@ class Series(PatchSet):
         """
         if self.is_initialised():
             raise StackException, '%s already initialized' % self.get_name()
-        for d in [self._dir(), self.__refs_dir]:
+        for d in [self._dir()]:
             if os.path.exists(d):
                 raise StackException, '%s already exists' % d
 
@@ -593,7 +594,6 @@ class Series(PatchSet):
 
         self.create_empty_field('applied')
         self.create_empty_field('unapplied')
-        os.makedirs(self.__refs_dir)
         self._set_field('orig-base', git.get_head())
 
         config.set(self.format_version_key(), str(FORMAT_VERSION))
@@ -606,14 +606,18 @@ class Series(PatchSet):
         if to_stack.is_initialised():
             raise StackException, '"%s" already exists' % to_stack.get_name()
 
+        patches = self.get_applied() + self.get_unapplied()
+
         git.rename_branch(self.get_name(), to_name)
 
+        for patch in patches:
+            git.rename_ref('refs/patches/%s/%s' % (self.get_name(), patch),
+                           'refs/patches/%s/%s' % (to_name, patch))
+            git.rename_ref('refs/patches/%s/%s.log' % (self.get_name(), patch),
+                           'refs/patches/%s/%s.log' % (to_name, patch))
         if os.path.isdir(self._dir()):
             rename(os.path.join(self._basedir(), 'patches'),
                    self.get_name(), to_stack.get_name())
-        if os.path.exists(self.__refs_dir):
-            rename(os.path.join(self._basedir(), 'refs', 'patches'),
-                   self.get_name(), to_stack.get_name())
 
         # Rename the config section
         config.rename_section("branch.%s" % self.get_name(),
@@ -715,9 +719,9 @@ class Series(PatchSet):
                                      % self._dir())
 
             try:
-                os.removedirs(self.__refs_dir)
-            except OSError:
-                out.warn('Refs directory %s is not empty' % self.__refs_dir)
+                git.delete_branch(self.get_name())
+            except GitException:
+                out.warn('Could not delete branch "%s"' % self.get_name())
 
         # Cleanup parent informations
         # FIXME: should one day make use of git-config --section-remove,
diff --git a/t/t0001-subdir-branches.sh b/t/t0001-subdir-branches.sh
index fac6339..1685233 100755
--- a/t/t0001-subdir-branches.sh
+++ b/t/t0001-subdir-branches.sh
@@ -50,10 +50,11 @@ test_expect_success 'Create patch in slashy branch' \
 
 test_expect_success 'Rename branches' \
   'stg branch --rename master goo/gaa &&
-   test ! -e .git/refs/heads/master &&
+   ! git show-ref --verify --quiet refs/heads/master &&
    stg branch --rename goo/gaa x1/x2/x3/x4 &&
-   test ! -e .git/refs/heads/goo &&
+   ! git show-ref --verify --quiet refs/heads/goo/gaa &&
    stg branch --rename x1/x2/x3/x4 servant &&
-   test ! -e .git/refs/heads/x1'
+   ! git show-ref --verify --quiet refs/heads/x1/x2/x3/x4
+'
 
 test_done
diff --git a/t/t1000-branch-create.sh b/t/t1000-branch-create.sh
index cca5504..e920e93 100755
--- a/t/t1000-branch-create.sh
+++ b/t/t1000-branch-create.sh
@@ -13,26 +13,9 @@ Exercises the "stg branch" commands.
 stg init
 
 test_expect_success \
-    'Create a spurious refs/patches/ entry' '
-    find .git -name foo | xargs rm -rf &&
-    touch .git/refs/patches/foo
-'
-
-test_expect_failure \
-    'Try to create an stgit branch with a spurious refs/patches/ entry' '
-    stg branch -c foo
-'
-
-test_expect_success \
-    'Check that no part of the branch was created' '
-    test "`find .git -name foo | tee /dev/stderr`" = ".git/refs/patches/foo" &&
-    ( grep foo .git/HEAD; test $? = 1 )
-'
-
-test_expect_success \
     'Create a spurious patches/ entry' '
     find .git -name foo | xargs rm -rf &&
-    touch .git/patches/foo
+    mkdir -p .git/patches && touch .git/patches/foo
 '
 
 test_expect_failure \
@@ -69,15 +52,4 @@ test_expect_success \
     touch .git/refs/heads/foo
 '
 
-test_expect_failure \
-    'Try to create an stgit branch with an invalid refs/heads/ entry' '
-    stg branch -c foo
-'
-
-test_expect_success \
-    'Check that no part of the branch was created' '
-    test "`find .git -name foo | tee /dev/stderr`" = ".git/refs/heads/foo" &&
-    ( grep foo .git/HEAD; test $? = 1 )
-'
-
 test_done
diff --git a/t/t1001-branch-rename.sh b/t/t1001-branch-rename.sh
index 28da15c..4e1ec84 100755
--- a/t/t1001-branch-rename.sh
+++ b/t/t1001-branch-rename.sh
@@ -26,7 +26,7 @@ test_expect_success \
     'Rename an stgit branch' \
     'stg branch -c buz &&
      stg branch -r foo bar &&
-     test -z `find .git -name foo | tee /dev/stderr`
+     [ -z $(find .git -type f | grep foo | tee /dev/stderr) ]
 '
 
 test_done