threads / patch / 9463

patch, 2 partsNew test: make sure that StGIT can handle packed refs

Subject: [StGIT PATCH 1/2] New test: make sure that StGIT can handle packed refs

## tl;dr

11 messages between Aug 10, 2007 and Aug 23, 2007. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Karl Hasselström· Aug 10, 2007, 03:23 UTC · lore

[StGIT PATCH 0/2] Teach StGIT to survive git-gc

... and here's the fix, and a test.

Note that while this is fairly intrusive (and took quite some time to debug), it's also a rather high-priority fix, since git-gc will break StGIT just fine even without the DAG patches.

You can also get this from git://repo.or.cz/stgit/kha.git.
-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle
Karl Hasselström· Aug 10, 2007, 03:23 UTC · re: Karl Hasselström · lore

It currently can't, which is why this test is needed. The situation has recently taken a turn for the worse, since git-gc nowadays packs refs by default.

---
 t/t1004-pack-ref.sh |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)
Show changes to t/t1004-pack-ref.sh +24 −0
diff --git a/t/t1004-pack-ref.sh b/t/t1004-pack-ref.sh
new file mode 100755
index 0000000..a5eb17c
--- /dev/null
+++ b/t/t1004-pack-ref.sh
@@ -0,0 +1,24 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 Karl Hasselström
+#
+
+test_description='Test that StGIT can handle packed refs'
+
+. ./test-lib.sh
+stg init
+
+test_expect_success \
+    'Pack refs and make sure that we can still see them' '
+    stg branch -c foo &&
+    [ $(stg branch -l | tee /dev/stderr | wc -l) -eq 2 ] &&
+    git pack-refs --all &&
+    [ $(stg branch -l | tee /dev/stderr | wc -l) -eq 2 ]
+'
+
+test_expect_success \
+    'Try to delete a branch whose ref has been packed' '
+    stg branch -d master
+'
+
+test_done
Karl Hasselström· Aug 10, 2007, 03:23 UTC · re: Karl Hasselström · lore

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

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(-)
Show changes to 7 files +91 −106

stgit/commands/branch.py, stgit/commands/common.py, stgit/git.py, stgit/stack.py, t/t0001-subdir-branches.sh, t/t1000-branch-create.sh, t/t1001-branch-rename.sh

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
Catalin Marinas· Aug 21, 2007, 13:23 UTC · re: Karl Hasselström · lore

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

Hi Karl,
On 10/08/07, Karl Hasselström <kha@treskal.com> wrote:
> 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.
Thanks for this patch. I'll apply it but I have some questions below.
Show 8 quoted lines
> @@ -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

Why replacing os.path.join with a single string? I think the former is more portable if, in the future, someone will try to run StGIT on Windows (and GIT would be compiled directly, without cygwin).

Show 11 quoted lines
> 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
I haven't tried this test but is there any problem if we keep them?
Thanks.
-- 
Catalin
Karl Hasselström· Aug 21, 2007, 15:58 UTC · re: Catalin Marinas · lore

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

On 2007-08-21 14:23:29 +0100, Catalin Marinas wrote:
Show 21 quoted lines
> On 10/08/07, Karl Hasselström <kha@treskal.com> wrote:
>
> > 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.
>
> Thanks for this patch. I'll apply it but I have some questions
> below.
>
> > @@ -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
>
> Why replacing os.path.join with a single string? I think the former
> is more portable if, in the future, someone will try to run StGIT on
> Windows (and GIT would be compiled directly, without cygwin).

Yes, that's the case iff it is a pathname in the file system. But I think forward slashes are used even on Windows in this case -- otherwise too mmuch configuration would have to be changed. This is what I _think_, however; I haven't actually checked.

Show 13 quoted lines
> > 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
>
> I haven't tried this test but is there any problem if we keep them?
Yes.

This test started to fail after my change. I don't remember exactly, but I think it was git-symbolic-ref actually reading the file, finding no ref there, and ignoring it. So this empty file didn't mess up the machinery anymore, and since this isn't a common usecase anyway, and since this change is to rely on git for all the low-level ref manipulation, I just deleted the test.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle
Karl Hasselström· Aug 21, 2007, 16:09 UTC · re: Karl Hasselström · lore

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

On 2007-08-21 17:58:45 +0200, Karl Hasselström wrote:
Show 27 quoted lines
> On 2007-08-21 14:23:29 +0100, Catalin Marinas wrote:
>
> > On 10/08/07, Karl Hasselström <kha@treskal.com> wrote:
> >
> > > 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
> >
> > I haven't tried this test but is there any problem if we keep
> > them?
>
> Yes.
>
> This test started to fail after my change. I don't remember exactly,
> but I think it was git-symbolic-ref actually reading the file,
> finding no ref there, and ignoring it. So this empty file didn't
> mess up the machinery anymore, and since this isn't a common usecase
> anyway, and since this change is to rely on git for all the
> low-level ref manipulation, I just deleted the test.

BTW, I just saw that this was explained by the second bullet point in the commit message. :-)

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle
Johannes Sixt· Aug 21, 2007, 16:12 UTC · re: Karl Hasselström · lore

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

Karl Hasselström wrote:
Show 19 quoted lines
> On 2007-08-21 14:23:29 +0100, Catalin Marinas wrote:
> > On 10/08/07, Karl Hasselström <kha@treskal.com> wrote:
> > > @@ -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
> >
> > Why replacing os.path.join with a single string? I think the former
> > is more portable if, in the future, someone will try to run StGIT on
> > Windows (and GIT would be compiled directly, without cygwin).
> 
> Yes, that's the case iff it is a pathname in the file system. But I
> think forward slashes are used even on Windows in this case --
> otherwise too mmuch configuration would have to be changed. This is
> what I _think_, however; I haven't actually checked.

There are some instances where the MinGW port accepts backslashes in ref names, but you are on the safe side with forward slashes.

-- Hannes
Karl Hasselström· Aug 21, 2007, 16:38 UTC · re: Johannes Sixt · lore

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

On 2007-08-21 18:12:30 +0200, Johannes Sixt wrote:
> There are some instances where the MinGW port accepts backslashes in
> ref names, but you are on the safe side with forward slashes.
This was just what I was expecting. Thanks for the confirmation!
-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle
Karl Hasselström· Aug 21, 2007, 16:46 UTC · re: Catalin Marinas · lore

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

On 2007-08-21 14:23:29 +0100, Catalin Marinas wrote:
> Thanks for this patch.
Two things:
  * The test in your tree has the latin1 form of my name in it,
    instead of utf8. (I noticed because I got a conflict when I
    rebased onto your master.)
  * The test suite now fails on your master (since you've applied just
    the test and not the fix). Please don't do that; it makes
    development needlessly hard. (To be fair, this is partly my fault,
    since I posted the fixin a separate patch. I'll change my ways.)
-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle
Karl Hasselström· Aug 21, 2007, 20:48 UTC · re: Karl Hasselström · lore

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

On 2007-08-21 18:46:29 +0200, Karl Hasselström wrote:
>   * The test in your tree has the latin1 form of my name in it,
>     instead of utf8. (I noticed because I got a conflict when I
>     rebased onto your master.)
That seems to have happened to the other new test too.
-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle
Catalin Marinas· Aug 23, 2007, 14:04 UTC · re: Karl Hasselström · lore

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

On 21/08/07, Karl Hasselström <kha@treskal.com> wrote:
Show 9 quoted lines
> On 2007-08-21 14:23:29 +0100, Catalin Marinas wrote:
>
> > Thanks for this patch.
>
> Two things:
>
>   * The test in your tree has the latin1 form of my name in it,
>     instead of utf8. (I noticed because I got a conflict when I
>     rebased onto your master.)

I imported it on a different PC (at work) where I think the locale support isn't properly set up. Xterm accepted a copy-paste operation with your name and I passed it through emacs which probably broke it. The From: header had a different encoding and hence the author was correct.

I'll try to do it on my home PC only (or just pull from your repo :-)).
>   * The test suite now fails on your master (since you've applied just
>     the test and not the fix). Please don't do that; it makes
>     development needlessly hard. (To be fair, this is partly my fault,
>     since I posted the fixin a separate patch. I'll change my ways.)

I think it's OK to send separate patches and I'm OK with a test failing in a development stage. The 2nd patch didn't apply cleanly and didn't have time to see what was wrong. I'll push it tonight, please have a look for things I might have missed.

Thanks.
-- 
Catalin

← back to recent threads