threads / patch / 38002

patchgit-new-workdir: Don't fail if the target directory is empty

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

## tl;dr

4 messages between Nov 18, 2014 and Nov 20, 2014. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Paul Smith· Nov 18, 2014, 19:36 UTC · lore

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 <paul@mad-scientist.net>
---

Getting rid of ls/wc was not as simple as I'd hoped, due to glob pathname expansion (can't rely on nullglob e.g.) If you want this let me know; it will require some yucky code to do the whole thing in native shell. Since new-workdir only works on systems with "ln -s" I think we can feel confident requiring "ls" and "wc" as well.

 contrib/workdir/git-new-workdir | 55 +++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 19 deletions(-)
Show changes to contrib/workdir/git-new-workdir +36 −19
diff --git a/contrib/workdir/git-new-workdir b/contrib/workdir/git-new-workdir
index 75e8b25..aef632d 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>]"
@@ -48,35 +52,48 @@ 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)
+
+# 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
+	mkdir -p "$new_workdir" || failed
+	cleandir="$new_workdir"
 fi
 
-# make sure the links use full paths
-git_dir=$(cd "$git_dir"; pwd)
+cleanup () {
+	rm -rf "$cleandir"
+}
+siglist="0 1 2 15"
+trap cleanup $siglist
 
-# create the workdir
-mkdir -p "$new_workdir/.git" || die "unable to create \"$new_workdir\"!"
+# create embedded directories
+for x in logs
+do
+	mkdir -p "$new_workdir/.git/$x" || failed
+done
 
 # 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
-	case $x in
-	*/*)
-		mkdir -p "$(dirname "$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"
 # 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)
-git checkout -f $branch
+cp "$git_dir/HEAD" "$new_workdir"/.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 --git-dir="$new_workdir"/.git --work-tree="$new_workdir" checkout -f $branch

-- 
1.8.5.3
Junio C Hamano· Nov 18, 2014, 20:15 UTC · re: Paul Smith · lore

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

Paul Smith <paul@mad-scientist.net> writes:
> Getting rid of ls/wc was not as simple as I'd hoped,

I didn't say ls/wc was not portable. Assuming that a directory for which the output from "ls -a" has two entries is empty may be.

Junio C Hamano· Nov 19, 2014, 17:32 UTC · re: Paul Smith · lore

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

Paul Smith <paul@mad-scientist.net> writes:
Show 14 quoted lines
> 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 <paul@mad-scientist.net>
> ---
>
> Getting rid of ls/wc was not as simple as I'd hoped, due to glob
> pathname expansion (can't rely on nullglob e.g.)  If you want this let
> me know; it will require some yucky code to do the whole thing in native
> shell.  Since new-workdir only works on systems with "ln -s" I think we
> can feel confident requiring "ls" and "wc" as well.
>
>  contrib/workdir/git-new-workdir | 55 +++++++++++++++++++++++++++--------------
>  1 file changed, 36 insertions(+), 19 deletions(-)

I took a look at this again, and I do not agree with one design decision it makes, namely:

>> I split the creation of the directories from the symlinks: see the new
>> loop above.  This allows us to avoid the icky dirname stuff.

which forces those who maintain the script to make sure that these two loops

	for dir in log
        do
        	make leading directory $dir
	done
	for path in refs logs/refs objects ...
	do
        	make symlink, assuming the leading directory exists
	done

kept consistent with each other. If you forget to add frotz to the upper loop when adding frotz/nitfol to the latter, you are breaking it.

I find it much more icky than computing what is necessary on the fly.
Show 5 quoted lines
> +# 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

I wondered if -gt instead of -ne is safer just in case what the description of "-A" alludes to, i.e. "ls -a" may not have ".." in its output, happens.

But that would not matter. If you are on such a filesystem and have a directory with a single file, you will see two entries in "ls -a" (one "." and the other the sole file) and mistake it as an empty directory whether you use -ne or -gt anyway, so -gt is not making it safer anyway. So let's go with this version.

Show 14 quoted lines
> +	then
> +		die "destination directory '$new_workdir' is not empty."
> +	fi
> +	cleandir="$new_workdir"/.git
> +else
> +	mkdir -p "$new_workdir" || failed
> +	cleandir="$new_workdir"
>  fi
>  
> -# make sure the links use full paths
> -git_dir=$(cd "$git_dir"; pwd)
> +cleanup () {
> +	rm -rf "$cleandir"
> +}
Aversion to turning $cleandir to an absolute path?  Why?

You may have avoided a problem by removing "cd" below in the current code, but I do not think it is a good future-proofing. If you make sure $cleandir is absolute, then you would not have to be even worried about other people breaking that assumption "this script will never let the trap hit while it cd to other places".

Show 15 quoted lines
> -# now setup the workdir
> -cd "$new_workdir"
>  # 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)
> -git checkout -f $branch
> +cp "$git_dir/HEAD" "$new_workdir"/.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 --git-dir="$new_workdir"/.git --work-tree="$new_workdir" checkout -f $branch

These uses of --git-dir/--work-tree look somewhat funny. You want to say "I want to run checkout in that $new_workdir", so say it in a more direct way, i.e.

    git -C "$new_workdir" checkout -f "$branch"
perhaps?
Thanks.
Paul Smith· Nov 20, 2014, 15:41 UTC · re: Junio C Hamano · lore

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

On Wed, 2014-11-19 at 09:32 -0800, Junio C Hamano wrote:
Show 12 quoted lines
> Paul Smith <paul@mad-scientist.net> writes:
> I took a look at this again, and I do not agree with one design
> decision it makes, namely:
> 
> >> I split the creation of the directories from the symlinks: see the new
> >> loop above.  This allows us to avoid the icky dirname stuff.
> 
> which forces those who maintain the script to make sure that these
> two loops
> kept consistent with each other.  If you forget to add frotz to the
> upper loop when adding frotz/nitfol to the latter, you are breaking
> it.

Yes, but this mistake won't live past even a single attempt to run the script, since it will immediately cause a fatal error. So this doesn't bother me.

> I find it much more icky than computing what is necessary on the fly.
OK, I'll go back to the previous way.
> Aversion to turning $cleandir to an absolute path?  Why?

In some situations switching to absolute can cause unexpected behaviors, where relative paths are no longer where you expect them etc. However in this case it's not a problem: I'll change it.

Show 5 quoted lines
> These uses of --git-dir/--work-tree look somewhat funny.  You want
> to say "I want to run checkout in that $new_workdir", so say it in a
> more direct way, i.e.
> 
>     git -C "$new_workdir" checkout -f "$branch"
Good idea.

← back to recent threads