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

Paul Smith [off-list ref] writes:

From 545c0d526eaa41f9306b567275a7d53799987482 Mon Sep 17 00:00:00 2001
From: Paul Smith <redacted>
Date: Fri, 14 Nov 2014 17:11:19 -0500
Subject: [PATCH] git-new-workdir: Don't fail if the target directory is empty

Please do not paste these in your mail message body.  The first line
is there only so that the output looks like traditinal mbox format
(i.e. each of these act as a signal that a new message starts there
in the file), the other three are there only for rare cases in which
you want to override what your e-mail's headers say and is only used
when you are sending somebody else's patch.

Also provide more error checking and clean up on failure.

As the body of the log message is supposed to be complete by itself,
not a continuation of a half-sentence started on the Subject:
(i.e. consider the subject line to be the title of an article you
are writing), starting it with "Also" is strange.

No need to resend only to correct the above, even though there may
be comments on the patch itself from me or others that may make you
want to reroll this patch, in which case I'd like to see these nits
gone.

Thanks.

With this change, the comment gets much harder to understand.  "What links?"
would be the reaction from those who are reading the patch.

+
+# 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."

I wonder if this check is portable for all platforms we care about,
but that is OK, as it should be so for the ones I think of and care
about ;-)

+	fi
+	was_existing=true
+else
+	mkdir -p "$new_workdir" || failed
+	was_existing=false
 fi
 
-# make sure the links use full paths
-git_dir=$(cd "$git_dir"; pwd)
+cleanup () {
+	if $was_existing
+	then
+		rm -rf "$new_workdir"/* "$new_workdir"/.[!.] "$new_workdir"/.??*
+	else
+		rm -rf "$new_workdir"
+	fi

The script chdirs around; did you turn $new_workdir into an absolute
path already, or given a relative $new_workdir this is attempting to
remove a hierarchy that is different from what you created?

+}
+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

What's this removal about?  If $new_workdir/.git/logs does not
exist, would "ln -s $there/logs/refs $new_workdir/.git/logs/refs"
succeed without first creating $new_workdir/.git/logs directory?

-	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"
+cd "$new_workdir" || failed
 # copy the HEAD from the original repository as a default branch
-cp "$git_dir/HEAD" .git/HEAD
+cp "$git_dir/HEAD" .git/HEAD || failed
+
+# don't delete the new workdir on exit
+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

Junio C Hamano [off-list ref] writes:

Paul Smith [off-list ref] writes:

No need to resend only to correct the above, even though there may
be comments on the patch itself from me or others that may make you
want to reroll this patch, in which case I'd like to see these nits
gone.

I'll fix in the next iteration.

quoted

-# don't recreate a workdir over an existing repository
-if test -e "$new_workdir"
+# make sure the links use full paths
+git_dir=$(cd "$git_dir"; pwd)

With this change, the comment gets much harder to understand.  "What
links?"  would be the reaction from those who are reading the patch.

I just moved this line; I don't think it had much more context
beforehand, but I'll definitely rewrite the comment to be more clear.

quoted

+	if test $(ls -a1 "$new_workdir/." | wc -l) -ne 2

I wonder if this check is portable for all platforms we care about,
but that is OK, as it should be so for the ones I think of and care
about ;-)

Do you mean "." and ".." representing an empty directory?  That will
work on any system where /bin/sh works, for sure.

Or do you mean using "ls" and "wc"?  I can easily avoid this.

Recall that new-workdir itself only works on systems that support
symlinks; this limits its portability.  For example it doesn't work on
Windows (unfortunately).  I spent the better part of a day a few months
ago playing with the various "symlink-ish" capabilities of Windows NTFS
and couldn't find any reliable way to make this work (although I have
virtually no Windows fu).

The script chdirs around; did you turn $new_workdir into an absolute
path already, or given a relative $new_workdir this is attempting to
remove a hierarchy that is different from what you created?

Good point, I'll fix this up

quoted

+}
+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

What's this removal about?  If $new_workdir/.git/logs does not
exist, would "ln -s $there/logs/refs $new_workdir/.git/logs/refs"
succeed without first creating $new_workdir/.git/logs directory?

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

New iteration will follow shortly.

Cheers!