Add a couple of safety checks to series creation

- we must check first whether the operation can complete, instead of
  bombing out halfway.  That means checking that nothing will prevent
  the creation of stgit data (note that calling is_initialised() is
  not enough, as it does not catch a bogus file), which is tested by
  the 3 first couple of testcases, and fixed in stack.py

- creating the git branch unconditionally before knowing whether
  creation of the stgit stuff can be completed is a problem as well:
  being atomic would be much much better.  To emulate atomicity (which
  comeds in the next patch), this patch does a somewhat dirty hack to
  branch.py: we first attempt to create the stgit stuff, and if that
  succeeds, we create the git branch: it is much easier to do at first
  a quick check that the latter will succeed.  Testcase 7/8 ensure
  that such a safety check has not been forgotten.

- when git already reports a problem with that head we would like to
  create, we should catch it.  Testcase 9/10 creates such a situation,
  and the fix to git.py allows to catch the error spit out by
  git-rev-parse.  I cannot tell why the stderr lines were not included
  by the Popen3 object.

Signed-off-by: Yann Dirson <ydirson@altern.org>

diff --git a/stgit/commands/branch.py b/stgit/commands/branch.py
index c4b5945..be501a8 100644 (file)
--- a/stgit/commands/branch.py
+++ b/stgit/commands/branch.py
@@ -126,8 +126,11 @@ def func(parser, options, args):
         if len(args) == 2:
             tree_id = git_id(args[1])
 
-        git.create_branch(args[0], tree_id)
+        if git.branch_exists(args[0]):
+            raise CmdException, 'Branch "%s" already exists' % args[0]
+        
         stack.Series(args[0]).init()
+        git.create_branch(args[0], tree_id)
 
         print 'Branch "%s" created.' % args[0]
         return

diff --git a/stgit/git.py b/stgit/git.py
index d75b54e..8523455 100644 (file)
--- a/stgit/git.py
+++ b/stgit/git.py
@@ -272,9 +272,11 @@ def rev_parse(git_id):
 def branch_exists(branch):
     """Existence check for the named branch
     """
-    for line in _output_lines(['git-rev-parse', '--symbolic', '--all']):
+    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
 
 def create_branch(new_branch, tree_id = None):

diff --git a/stgit/stack.py b/stgit/stack.py
index f4d7490..c14e029 100644 (file)
--- a/stgit/stack.py
+++ b/stgit/stack.py
@@ -431,8 +431,13 @@ class Series:
         """
         bases_dir = os.path.join(self.__base_dir, 'refs', 'bases')
 
-        if self.is_initialised():
+        if os.path.exists(self.__patch_dir):
             raise StackException, self.__patch_dir + ' already exists'
+        if os.path.exists(self.__refs_dir):
+            raise StackException, self.__refs_dir + ' already exists'
+        if os.path.exists(self.__base_file):
+            raise StackException, self.__base_file + ' already exists'
+
         os.makedirs(self.__patch_dir)
 
         if not os.path.isdir(bases_dir):

diff --git a/t/t1000-branch-create.sh b/t/t1000-branch-create.sh
new file mode 100755 (executable)
index 0000000..f0c2367
--- /dev/null
+++ b/t/t1000-branch-create.sh
@@ -0,0 +1,82 @@
+#!/bin/sh
+#
+# Copyright (c) 2006 Yann Dirson
+#
+
+test_description='Branch operations.
+
+Exercises the "stg branch" commands.
+'
+
+. ./test-lib.sh
+
+stg init
+
+test_expect_failure \
+    'Try to create an stgit branch with a spurious refs/patches/ entry' \
+    'find .git -name foo | xargs rm -rf &&
+     touch .git/refs/patches/foo &&
+     stg branch -c foo
+'
+
+test_expect_success \
+    'Check 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_failure \
+    'Try to create an stgit branch with a spurious patches/ entry' \
+    'find .git -name foo | xargs rm -rf &&
+     touch .git/patches/foo &&
+     stg branch -c foo
+'
+
+test_expect_success \
+    'Check no part of the branch was created' \
+    'test "`find .git -name foo | tee /dev/stderr`" = ".git/patches/foo" &&
+     ( grep foo .git/HEAD; test $? = 1 )
+'
+
+test_expect_failure \
+    'Try to create an stgit branch with a spurious refs/bases/ entry' \
+    'find .git -name foo | xargs rm -rf &&
+     touch .git/refs/bases/foo &&
+     stg branch -c foo
+'
+
+test_expect_success \
+    'Check no part of the branch was created' \
+    'test "`find .git -name foo | tee /dev/stderr`" = ".git/refs/bases/foo" &&
+     ( grep foo .git/HEAD; test $? = 1 )
+'
+
+
+test_expect_failure \
+    'Try to create an stgit branch with an existing git branch by that name' \
+    'find .git -name foo | xargs rm -rf &&
+     cp .git/refs/heads/master .git/refs/heads/foo &&
+     stg branch -c foo
+'
+
+test_expect_success \
+    'Check 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_expect_failure \
+    'Try to create an stgit branch with an invalid refs/heads/ entry' \
+    'find .git -name foo | xargs rm -rf &&
+     touch .git/refs/heads/foo &&
+     stg branch -c foo
+'
+
+test_expect_success \
+    'Check 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