Re: [PATCH v4] git-new-workdir: Don't fail if the target directory is empty

2014-12-10 Thread Paul Smith 
On Wed, 2014-11-26 at 15:16 -0800, Junio C Hamano wrote:
 Paul Smith p...@mad-scientist.net writes:
 
  This is what happens for a file:
 
  $ rm -f foo
 
  $ touch foo
 
  $ ./src/git/contrib/workdir/git-new-workdir src/git foo master
  mkdir: cannot create directory ‘foo’: Not a directory
  unable to create new workdir foo!
 
 ;-)  That comes from mkdir || fail which is indeed sufficient.

Hi Junio;

Is this all set to be applied or is there more to do?

Cheers!

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] git-new-workdir: Don't fail if the target directory is empty

2014-11-28 Thread Paul Smith 
On Wed, 2014-11-26 at 15:16 -0800, Junio C Hamano wrote:
  $ ./src/git/contrib/workdir/git-new-workdir src/git foo master
  mkdir: cannot create directory ‘foo’: Not a directory
  unable to create new workdir foo!
 
 ;-)  That comes from mkdir || fail which is indeed sufficient.

Right.  Often I find it simpler/clearer to let the underlying commands
give the errors: they use perror() and can often provide more specific
error messages than my script can, unless I spend a lot of effort trying
to determine exactly what the problem is (permissions, disk space, bad
symlink, existing file, whatever).

Should I respin this with the \$new_workdir\ - '$new_workdir' change
(I actually prefer the latter myself but the former was used somewhere
so I kept it)?

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4] git-new-workdir: Don't fail if the target directory is empty

2014-11-26 Thread Paul Smith 
Allow new workdirs to be created in an empty directory (similar to git
clone).  Provide more error checking and clean up on failure.

Signed-off-by: Paul Smith p...@mad-scientist.net
---

Hopefully this doesn't contain unwanted stylistic changes.  There's a
kind of gross thing about the behavior here: because we cd much earlier
than in my previous version we could actually trap and rm -rf while
our current working directory is inside the removed location (inside
$cleandir).  On a traditional UNIX filesystem this is fine, of course,
but it's possible that some filesystems don't like this in some way.  In
Windows for example this will definitely fail (but new-workdir doesn't
support Windows anyway).  The only options are (a) don't worry about it
(that's what this patch does), (b) put the cd after the trap reset
again, (c) move the trap reset earlier and don't clean up if we get
failures after we cd, or (d) cd again inside the cleanup function.
Of these, (d) is the most complex and hence my least favorite.

 contrib/workdir/git-new-workdir | 53 +
 1 file changed, 38 insertions(+), 15 deletions(-)

diff --git a/contrib/workdir/git-new-workdir b/contrib/workdir/git-new-workdir
index 75e8b25..d1d7f32 100755
--- a/contrib/workdir/git-new-workdir
+++ b/contrib/workdir/git-new-workdir
@@ -10,6 +10,10 @@ die () {
exit 128
 }
 
+failed () {
+   die unable to create new workdir \$new_workdir\!
+}
+
 if test $# -lt 2 || test $# -gt 3
 then
usage $0 repository new_workdir [branch]
@@ -35,7 +39,7 @@ esac
 
 # don't link to a configured bare repository
 isbare=$(git --git-dir=$git_dir config --bool --get core.bare)
-if test ztrue = z$isbare
+if test ztrue = z$isbare
 then
die \$git_dir\ has core.bare set to true, \
 remove from \$git_dir/config\ to use $0
@@ -48,35 +52,54 @@ then
a complete repository.
 fi
 
-# don't recreate a workdir over an existing repository
-if test -e $new_workdir
+# make sure the links in the workdir have full paths to the original repo
+git_dir=$(cd $git_dir  pwd) || exit 1
+
+# don't recreate a workdir over an existing directory, unless it's empty
+if test -d $new_workdir
 then
-   die destination directory '$new_workdir' already exists.
+   if test $(ls -a1 $new_workdir/. | wc -l) -ne 2
+   then
+   die destination directory '$new_workdir' is not empty.
+   fi
+   cleandir=$new_workdir/.git
+else
+   cleandir=$new_workdir
 fi
 
-# make sure the links use full paths
-git_dir=$(cd $git_dir; pwd)
+mkdir -p $new_workdir/.git || failed
+cleandir=$(cd $cleandir  pwd) || failed
 
-# create the workdir
-mkdir -p $new_workdir/.git || die unable to create \$new_workdir\!
+cleanup () {
+   rm -rf $cleandir
+}
+siglist=0 1 2 15
+trap cleanup $siglist
 
 # create the links to the original repo.  explicitly exclude index, HEAD and
 # logs/HEAD from the list since they are purely related to the current working
 # directory, and should not be shared.
 for x in config refs logs/refs objects info hooks packed-refs remotes rr-cache 
svn
 do
+   # create a containing directory if needed
case $x in
*/*)
-   mkdir -p $(dirname $new_workdir/.git/$x)
+   mkdir -p $new_workdir/.git/${x%/*}
;;
esac
-   ln -s $git_dir/$x $new_workdir/.git/$x
+
+   ln -s $git_dir/$x $new_workdir/.git/$x || failed
 done
 
-# now setup the workdir
-cd $new_workdir
+# commands below this are run in the context of the new workdir
+cd $new_workdir || failed
+
 # copy the HEAD from the original repository as a default branch
-cp $git_dir/HEAD .git/HEAD
-# checkout the branch (either the same as HEAD from the original repository, or
-# the one that was asked for)
+cp $git_dir/HEAD .git/HEAD || failed
+
+# the workdir is set up.  if the checkout fails, the user can fix it.
+trap - $siglist
+
+# checkout the branch (either the same as HEAD from the original repository,
+# or the one that was asked for)
 git checkout -f $branch
-- 
1.8.5.3



--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] git-new-workdir: Don't fail if the target directory is empty

2014-11-26 Thread Junio C Hamano 
Paul Smith p...@mad-scientist.net writes:

 Allow new workdirs to be created in an empty directory (similar to git
 clone).  Provide more error checking and clean up on failure.

 Signed-off-by: Paul Smith p...@mad-scientist.net
 ---

 Hopefully this doesn't contain unwanted stylistic changes.

;-)

Unwanted, no, but unrelated yes.

  
 +failed () {
 + die unable to create new workdir \$new_workdir\!
 +}

Use '$new_workdir' instead to match the existing message below?

 -# don't recreate a workdir over an existing repository
 -if test -e $new_workdir
 +# make sure the links in the workdir have full paths to the original repo
 +git_dir=$(cd $git_dir  pwd) || exit 1
 +
 +# don't recreate a workdir over an existing directory, unless it's empty
 +if test -d $new_workdir
  then
 - die destination directory '$new_workdir' already exists.
 + if test $(ls -a1 $new_workdir/. | wc -l) -ne 2
 + then
 + die destination directory '$new_workdir' is not empty.
 + fi
 + cleandir=$new_workdir/.git
 +else
 + cleandir=$new_workdir
  fi

The comment in the original is somewhat misleading, but test -e
was test -e and not test -d to stop when an existing file was
given by mistake as $new_workdir, I think.  I do not know what
happens in the new code in that case.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] git-new-workdir: Don't fail if the target directory is empty

2014-11-26 Thread Paul Smith 
On Wed, 2014-11-26 at 13:55 -0800, Junio C Hamano wrote:
 The comment in the original is somewhat misleading, but test -e
 was test -e and not test -d to stop when an existing file was
 given by mistake as $new_workdir, I think.  I do not know what
 happens in the new code in that case.

I did test that.  I have a little set of tests with a no directory,
empty directory, non-empty directory, plus various permissions issues
(existing directory without write privs, no write privs to the parent
directory), and also if the new directory name is a file, a symlink
pointing to something, a symlink pointing to nothing, etc.

This is what happens for a file:

$ rm -f foo

$ touch foo

$ ./src/git/contrib/workdir/git-new-workdir src/git foo master
mkdir: cannot create directory ‘foo’: Not a directory
unable to create new workdir foo!


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] git-new-workdir: Don't fail if the target directory is empty

2014-11-26 Thread Junio C Hamano 
Paul Smith p...@mad-scientist.net writes:

 This is what happens for a file:

 $ rm -f foo

 $ touch foo

 $ ./src/git/contrib/workdir/git-new-workdir src/git foo master
 mkdir: cannot create directory ‘foo’: Not a directory
 unable to create new workdir foo!

;-)  That comes from mkdir || fail which is indeed sufficient.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html