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

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

[Paul Smith]

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

Also provide more error checking and clean up on failure.

Signed-off-by: Paul Smith <paul@mad-scientist.net>
---

Thanks Junio.  I've reworked the change so it will automatically succeed
if the directory does not exists or exists but is empty, and fail
otherwise, which as far as I can tell is the behavior "git clone" uses
as well.  I removed the -f flag as no longer needed.  I also added some
cleanup that is performed if the new-workdir operation fails for any
reason so you don't get partly-constructed workdirs.  I also added more
error checking so that we immediately stop if any step fails.  Some may
suggest "set -e" but that flag can be tricky... I preferred to make the
failure explicit.

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

diff --git a/contrib/workdir/git-new-workdir b/contrib/workdir/git-new-workdir
index 75e8b25..c402000 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,55 @@ then
 		"a complete repository."
 fi
 
-# 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)
+
+# 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
+	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
+}
+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"
+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
-- 
1.8.5.3

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

[Junio C Hamano]

Paul Smith <paul@mad-scientist.net> writes:

> From 545c0d526eaa41f9306b567275a7d53799987482 Mon Sep 17 00:00:00 2001
> From: Paul Smith <paul@mad-scientist.net>
> 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.


> diff --git a/contrib/workdir/git-new-workdir b/contrib/workdir/git-new-workdir
> index 75e8b25..c402000 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,55 @@ then
>  		"a complete repository."
>  fi
>  
> -# 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.

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

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

[Paul Smith]

Junio C Hamano <gitster@pobox.com> writes:
> Paul Smith <paul@mad-scientist.net> 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.

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

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

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

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

[Junio C Hamano]

Paul Smith <paul@mad-scientist.net> writes:

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

Even on network mounts from esoteric filesystems and such?  When

    http://pubs.opengroup.org/onlinepubs/9699919799/utilities/ls.html

mentions the "-A" option, it says:

    -A
        Write out all directory entries, including those whose names
        begin with a <period> ( '.' ) but excluding the entries dot
        and dot-dot (if they exist).

The "if they exist" part suggests, at least to me, that it is valid
for a POSIX filesystem to lack these two (I suspect that one may be
able to find a more definitive answer from other parts of the POSIX
but I didn't bother).

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

[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 <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(-)

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

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

[Junio C Hamano]

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.

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

[Paul Smith]

On Tue, 2014-11-18 at 12:15 -0800, Junio C Hamano wrote:
> 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.

Yes, I didn't get your email before I sent the latest patch.  Sorry
about that.

On Tue, 2014-11-18 at 11:32 -0800, Junio C Hamano wrote:
> Even on network mounts from esoteric filesystems and such?  When
> 
>     http://pubs.opengroup.org/onlinepubs/9699919799/utilities/ls.html
> 
> mentions the "-A" option, it says:
> 
>     -A
>         Write out all directory entries, including those whose names
>         begin with a <period> ( '.' ) but excluding the entries dot
>         and dot-dot (if they exist).
> 
> The "if they exist" part suggests, at least to me, that it is valid
> for a POSIX filesystem to lack these two (I suspect that one may be
> able to find a more definitive answer from other parts of the POSIX
> but I didn't bother).

Hm.  Well, POSIX clearly reserves "." and ".." to be special and
requires that directories containing only those values are considered
empty (rmdir(2) says so).  The definitions section contains special
wording for "dot" and "dot-dot".

Looking at
http://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xbd_chap04.html

        Each directory has exactly one parent directory which is
        represented by the name dot-dot in the first directory.

which implies that every directory must have "..".  This is in the
Rationale, I realize.

There are also various mentions to using "." as a synonym for the
current directory.

I can't find a clear statement that both are required and that "ls -a"
must show them.  I've used a wide range of UNIX-en and filesystems for
30 years or so and never seen one that didn't provide them.  It seems
like it would break quite a few scripts, at least.  Even virtual
filesystems like ClearCase's MVFS provide "." and "..".

If you want to allow for this possibility I can do so but it would be a
bit crufty.  Personally I think it would be overkill but you're the
boss: let me know :-).

Cheers!

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

[Junio C Hamano]

Paul Smith <paul@mad-scientist.net> writes:

> I can't find a clear statement that both are required and that "ls -a"
> must show them.  I've used a wide range of UNIX-en and filesystems for
> 30 years or so and never seen one that didn't provide them.  It seems
> like it would break quite a few scripts, at least.  Even virtual
> filesystems like ClearCase's MVFS provide "." and "..".
>
> If you want to allow for this possibility I can do so but it would be a
> bit crufty.  Personally I think it would be overkill but you're the
> boss: let me know :-).

Doesn't the description of the -A option I quoted upthread hint a
simpler and clearer solution?  I.e. "test $(ls -A | wc -l) = 0"?

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

[Paul Smith]

On Tue, 2014-11-18 at 12:58 -0800, Junio C Hamano wrote:
> Doesn't the description of the -A option I quoted upthread hint a
> simpler and clearer solution?  I.e. "test $(ls -A | wc -l) = 0"?

Yes, but unfortunately for us the -A flag was added to POSIX Issue 7.
It's not present in the previous version of POSIX, Issue 6:

http://pubs.opengroup.org/onlinepubs/009695399/utilities/ls.html

It came from the BSD world, so it might not be available on older
SysV-derived systems (AIX, HP-UX, even Solaris... I don't have access to
these anymore so I can't say).  Ultimately it's probably more portable
to assume "ls -a" always prints "." and ".." than to assume "ls -A" is
supported.

Cheers!

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

[Junio C Hamano]

OK, thanks for digging. Let's go with this version, then.


On Tue, Nov 18, 2014 at 2:25 PM, Paul Smith <paul@mad-scientist.net> wrote:
> On Tue, 2014-11-18 at 12:58 -0800, Junio C Hamano wrote:
>> Doesn't the description of the -A option I quoted upthread hint a
>> simpler and clearer solution?  I.e. "test $(ls -A | wc -l) = 0"?
>
> Yes, but unfortunately for us the -A flag was added to POSIX Issue 7.
> It's not present in the previous version of POSIX, Issue 6:
>
> http://pubs.opengroup.org/onlinepubs/009695399/utilities/ls.html
>
> It came from the BSD world, so it might not be available on older
> SysV-derived systems (AIX, HP-UX, even Solaris... I don't have access to
> these anymore so I can't say).  Ultimately it's probably more portable
> to assume "ls -a" always prints "." and ".." than to assume "ls -A" is
> supported.
>
> Cheers!
>

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

[Paul Smith]

On Tue, 2014-11-18 at 14:51 -0800, Junio C Hamano wrote:
> OK, thanks for digging. Let's go with this version, then.

Thanks for your attention, Junio!

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

[Junio C Hamano]

Paul Smith <paul@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 <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.

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

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

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

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

[Paul Smith]

On Wed, 2014-11-19 at 09:32 -0800, Junio C Hamano wrote:
> 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.

> 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.

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

[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 <paul@mad-scientist.net>
---
 contrib/workdir/git-new-workdir | 54 +++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 18 deletions(-)

diff --git a/contrib/workdir/git-new-workdir b/contrib/workdir/git-new-workdir
index 75e8b25..7334720 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,49 @@ 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"
 # 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 -C "$new_workdir" checkout -f $branch
-- 
1.8.5.3

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

[Junio C Hamano]

Paul Smith <paul@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 <paul@mad-scientist.net>
> ---
>  contrib/workdir/git-new-workdir | 54 +++++++++++++++++++++++++++--------------
>  1 file changed, 36 insertions(+), 18 deletions(-)

Thanks.  I would have liked [PATCH v$n] on the subject line but
that's minor.

I also see you made a few small improvements here and there, like...

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

... this bit, which I think is a good idea.  We are asking "--bool"
so we know we will never get anything other than true/false to cause
trouble by not quoting, but the dq-pair there does not cost anything
and it is a good discipline.

> +# make sure the links in the workdir have full paths to the original repo
> +git_dir=$(cd "$git_dir" && pwd) || exit 1

... so is this bit, which used to just go even if cd failed.

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

You used to quote this as

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

and I think that made a lot more sense than the new quoting.

The ultimate reason we quote here is because "$new_workdir/." is the
single directory path we want to give "ls", and quoting the whole
thing shows that intention far clearer than the new one, which says
"there may be an $IFS character in $new_workdir so I am quoting to
keep them together as a single string and then appending /. to that
single string will still be a single string", which is not wrong
per-se in that it is saying the same thing, but it is a roundabout
way to do so.

> +	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%/*}" ;;

Case arms come at the same indentation level as "case/esac".

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

Why these changes?  I thought you are making sure $cleandir is
absolute so that you do not have to do this and can just "cd" into
the new working tree, the same way the user would do once it is set
up.

Puzzled...  Before seeing this bit, I was about to say "but all the
above nits are minor so no need to resend---I'll locally fix them up
while queueing", but I suspect that I may be missing some obvious
reason why you avoid "cd" and I am reluctant to "fix" this part up
myself without seeing a response, so I'd swallow that "no need to
resend" X-<.

> +# 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 -C "$new_workdir" checkout -f $branch

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

[Paul Smith]

On Thu, 2014-11-20 at 09:13 -0800, Junio C Hamano wrote:
> Paul Smith <paul@mad-scientist.net> writes:
> > +# 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
> 
> You used to quote this as
> 
> 	if test $(ls -a1 "$new_workdir/." | wc -l) -ne 2
> 
> and I think that made a lot more sense than the new quoting.

Just personal script coding style.  I tend to quote just variables, in
case I want to add globbing or whatever later.  I've run into people who
think that "$foo/bar" needs quotes because it concatenates two strings,
but you don't need to quote $foo by itself; quoting just the variable
helps avoid that impression.

But this doesn't matter much to me.

> Case arms come at the same indentation level as "case/esac".

OK.

> Why these changes?  I thought you are making sure $cleandir is
> absolute so that you do not have to do this and can just "cd" into
> the new working tree, the same way the user would do once it is set
> up.

I made cleandir absolute mainly because you asked me to :-).  I didn't
realize that the implication behind that request was that I'd put back
the original "cd" as well.

I personally distrust and avoid raw cd in shell scripts.  It's
action-at-a-distance and can cause all sorts of problems, sometimes
disastrous ones.  If I need to change directories I localize it, e.g.:

  (cd "$somedir" && do some command)

so it's clear that it's in effect only for that command.

I understand that in this particular situation the "cd" is right before
the end so this doesn't hurt (as the script stands today).  If you
prefer the "cd" explicitly I'll add it back.  You're the one that needs
to be happy with it :-).

What if I move the cd down to right before the checkout command, and use
$new_workdir in the copy of HEAD?  At least it won't be in effect until
after the workdir is completely set up.  I would feel better about it
then :-).

> > -# 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

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

[Junio C Hamano]

Paul Smith <paul@mad-scientist.net> writes:

>> Why these changes?  I thought you are making sure $cleandir is
>> absolute so that you do not have to do this and can just "cd" into
>> the new working tree, the same way the user would do once it is set
>> up.
>
> I made cleandir absolute mainly because you asked me to :-).  I didn't
> realize that the implication behind that request was that I'd put back
> the original "cd" as well.

This is about not changing unnecessarily the original that has been
battle-tested and keeping the result resistant to future mistakes
(i.e. somebody going elsewhere without realizing a relative cleandir
will cause problems).  Once $cleandir is fixed, there no longer is a
good reason to change how HEAD is pointed in the $new_workdir or its
working tree populated; the change in the earlier round in that area
was necessary when $cleandir was not absolute, but not anymore.