{"thread":{"id":"38058","subject":"[PATCH v4] git-new-workdir: Don't fail if the target directory is empty","startedAt":"2014-11-26T20:38:28Z","lastAt":"2014-12-10T17:16:57Z","messageCount":6,"participants":["Paul Smith","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"252601","messageId":"1417034308.23650.51.camel@homebase","threadId":"38058","inReplyTo":null,"subject":"[PATCH v4] git-new-workdir: Don't fail if the target directory is empty","fromName":"Paul Smith","fromEmail":"paul@mad-scientist.net","sentAt":"2014-11-26T20:38:28Z","receivedAt":"2014-11-26T20:38:28Z","isPatch":true,"sender":{"key":"paul@mad-scientist.net","avatar":"https://avatars.githubusercontent.com/u/109636?v=4"},"body":"Allow new workdirs to be created in an empty directory (similar to \"git\nclone\").  Provide more error checking and clean up on failure.\n\nSigned-off-by: Paul Smith <paul@mad-scientist.net>\n---\n\nHopefully this doesn't contain unwanted stylistic changes.  There's a\nkind of gross thing about the behavior here: because we cd much earlier\nthan in my previous version we could actually trap and \"rm -rf\" while\nour current working directory is inside the removed location (inside\n$cleandir).  On a traditional UNIX filesystem this is fine, of course,\nbut it's possible that some filesystems don't like this in some way.  In\nWindows for example this will definitely fail (but new-workdir doesn't\nsupport Windows anyway).  The only options are (a) don't worry about it\n(that's what this patch does), (b) put the \"cd\" after the trap reset\nagain, (c) move the trap reset earlier and don't clean up if we get\nfailures after we \"cd\", or (d) \"cd\" again inside the cleanup function.\nOf these, (d) is the most complex and hence my least favorite.\n\n contrib/workdir/git-new-workdir | 53 +++++++++++++++++++++++++++++------------\n 1 file changed, 38 insertions(+), 15 deletions(-)\n\ndiff --git a/contrib/workdir/git-new-workdir b/contrib/workdir/git-new-workdir\nindex 75e8b25..d1d7f32 100755\n--- a/contrib/workdir/git-new-workdir\n+++ b/contrib/workdir/git-new-workdir\n@@ -10,6 +10,10 @@ die () {\n \texit 128\n }\n \n+failed () {\n+\tdie \"unable to create new workdir \\\"$new_workdir\\\"!\"\n+}\n+\n if test $# -lt 2 || test $# -gt 3\n then\n \tusage \"$0 <repository> <new_workdir> [<branch>]\"\n@@ -35,7 +39,7 @@ esac\n \n # don't link to a configured bare repository\n isbare=$(git --git-dir=\"$git_dir\" config --bool --get core.bare)\n-if test ztrue = z$isbare\n+if test ztrue = \"z$isbare\"\n then\n \tdie \"\\\"$git_dir\\\" has core.bare set to true,\" \\\n \t\t\" remove from \\\"$git_dir/config\\\" to use $0\"\n@@ -48,35 +52,54 @@ then\n \t\t\"a complete repository.\"\n fi\n \n-# don't recreate a workdir over an existing repository\n-if test -e \"$new_workdir\"\n+# make sure the links in the workdir have full paths to the original repo\n+git_dir=$(cd \"$git_dir\" && pwd) || exit 1\n+\n+# don't recreate a workdir over an existing directory, unless it's empty\n+if test -d \"$new_workdir\"\n then\n-\tdie \"destination directory '$new_workdir' already exists.\"\n+\tif test $(ls -a1 \"$new_workdir/.\" | wc -l) -ne 2\n+\tthen\n+\t\tdie \"destination directory '$new_workdir' is not empty.\"\n+\tfi\n+\tcleandir=\"$new_workdir/.git\"\n+else\n+\tcleandir=\"$new_workdir\"\n fi\n \n-# make sure the links use full paths\n-git_dir=$(cd \"$git_dir\"; pwd)\n+mkdir -p \"$new_workdir/.git\" || failed\n+cleandir=$(cd \"$cleandir\" && pwd) || failed\n \n-# create the workdir\n-mkdir -p \"$new_workdir/.git\" || die \"unable to create \\\"$new_workdir\\\"!\"\n+cleanup () {\n+\trm -rf \"$cleandir\"\n+}\n+siglist=\"0 1 2 15\"\n+trap cleanup $siglist\n \n # create the links to the original repo.  explicitly exclude index, HEAD and\n # logs/HEAD from the list since they are purely related to the current working\n # directory, and should not be shared.\n for x in config refs logs/refs objects info hooks packed-refs remotes rr-cache svn\n do\n+\t# create a containing directory if needed\n \tcase $x in\n \t*/*)\n-\t\tmkdir -p \"$(dirname \"$new_workdir/.git/$x\")\"\n+\t\tmkdir -p \"$new_workdir/.git/${x%/*}\"\n \t\t;;\n \tesac\n-\tln -s \"$git_dir/$x\" \"$new_workdir/.git/$x\"\n+\n+\tln -s \"$git_dir/$x\" \"$new_workdir/.git/$x\" || failed\n done\n \n-# now setup the workdir\n-cd \"$new_workdir\"\n+# commands below this are run in the context of the new workdir\n+cd \"$new_workdir\" || failed\n+\n # copy the HEAD from the original repository as a default branch\n-cp \"$git_dir/HEAD\" .git/HEAD\n-# checkout the branch (either the same as HEAD from the original repository, or\n-# the one that was asked for)\n+cp \"$git_dir/HEAD\" .git/HEAD || failed\n+\n+# the workdir is set up.  if the checkout fails, the user can fix it.\n+trap - $siglist\n+\n+# checkout the branch (either the same as HEAD from the original repository,\n+# or the one that was asked for)\n git checkout -f $branch\n-- \n1.8.5.3\n"},{"id":"252609","messageId":"xmqq8uixpqyx.fsf@gitster.dls.corp.google.com","threadId":"38058","inReplyTo":"1417034308.23650.51.camel@homebase","subject":"Re: [PATCH v4] git-new-workdir: Don't fail if the target directory is empty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-26T21:55:02Z","receivedAt":"2014-11-26T21:55:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Smith <paul@mad-scientist.net> writes:\n\n> Allow new workdirs to be created in an empty directory (similar to \"git\n> clone\").  Provide more error checking and clean up on failure.\n>\n> Signed-off-by: Paul Smith <paul@mad-scientist.net>\n> ---\n>\n> Hopefully this doesn't contain unwanted stylistic changes.\n\n;-)\n\nUnwanted, no, but unrelated yes.\n\n>  \n> +failed () {\n> +\tdie \"unable to create new workdir \\\"$new_workdir\\\"!\"\n> +}\n\nUse '$new_workdir' instead to match the existing message below?\n\n> -# don't recreate a workdir over an existing repository\n> -if test -e \"$new_workdir\"\n> +# make sure the links in the workdir have full paths to the original repo\n> +git_dir=$(cd \"$git_dir\" && pwd) || exit 1\n> +\n> +# don't recreate a workdir over an existing directory, unless it's empty\n> +if test -d \"$new_workdir\"\n>  then\n> -\tdie \"destination directory '$new_workdir' already exists.\"\n> +\tif test $(ls -a1 \"$new_workdir/.\" | wc -l) -ne 2\n> +\tthen\n> +\t\tdie \"destination directory '$new_workdir' is not empty.\"\n> +\tfi\n> +\tcleandir=\"$new_workdir/.git\"\n> +else\n> +\tcleandir=\"$new_workdir\"\n>  fi\n\nThe comment in the original is somewhat misleading, but \"test -e\"\nwas \"test -e\" and not \"test -d\" to stop when an existing file was\ngiven by mistake as $new_workdir, I think.  I do not know what\nhappens in the new code in that case.\n"},{"id":"252614","messageId":"1417041115.23650.69.camel@homebase","threadId":"38058","inReplyTo":"xmqq8uixpqyx.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4] git-new-workdir: Don't fail if the target directory is empty","fromName":"Paul Smith","fromEmail":"paul@mad-scientist.net","sentAt":"2014-11-26T22:31:55Z","receivedAt":"2014-11-26T22:31:55Z","isPatch":true,"sender":{"key":"paul@mad-scientist.net","avatar":"https://avatars.githubusercontent.com/u/109636?v=4"},"body":"On Wed, 2014-11-26 at 13:55 -0800, Junio C Hamano wrote:\n> The comment in the original is somewhat misleading, but \"test -e\"\n> was \"test -e\" and not \"test -d\" to stop when an existing file was\n> given by mistake as $new_workdir, I think.  I do not know what\n> happens in the new code in that case.\n\nI did test that.  I have a little set of tests with a no directory,\nempty directory, non-empty directory, plus various permissions issues\n(existing directory without write privs, no write privs to the parent\ndirectory), and also if the new directory name is a file, a symlink\npointing to something, a symlink pointing to nothing, etc.\n\nThis is what happens for a file:\n\n$ rm -f foo\n\n$ touch foo\n\n$ ./src/git/contrib/workdir/git-new-workdir src/git foo master\nmkdir: cannot create directory ‘foo’: Not a directory\nunable to create new workdir \"foo\"!\n"},{"id":"252621","messageId":"xmqqk32ho8mc.fsf@gitster.dls.corp.google.com","threadId":"38058","inReplyTo":"1417041115.23650.69.camel@homebase","subject":"Re: [PATCH v4] git-new-workdir: Don't fail if the target directory is empty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-26T23:16:43Z","receivedAt":"2014-11-26T23:16:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Smith <paul@mad-scientist.net> writes:\n\n> This is what happens for a file:\n>\n> $ rm -f foo\n>\n> $ touch foo\n>\n> $ ./src/git/contrib/workdir/git-new-workdir src/git foo master\n> mkdir: cannot create directory ‘foo’: Not a directory\n> unable to create new workdir \"foo\"!\n\n;-)  That comes from mkdir || fail which is indeed sufficient.\n"},{"id":"252671","messageId":"1417199645.3562.6.camel@homebase","threadId":"38058","inReplyTo":"xmqqk32ho8mc.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4] git-new-workdir: Don't fail if the target directory is empty","fromName":"Paul Smith","fromEmail":"paul@mad-scientist.net","sentAt":"2014-11-28T18:34:05Z","receivedAt":"2014-11-28T18:34:05Z","isPatch":true,"sender":{"key":"paul@mad-scientist.net","avatar":"https://avatars.githubusercontent.com/u/109636?v=4"},"body":"On Wed, 2014-11-26 at 15:16 -0800, Junio C Hamano wrote:\n> > $ ./src/git/contrib/workdir/git-new-workdir src/git foo master\n> > mkdir: cannot create directory ‘foo’: Not a directory\n> > unable to create new workdir \"foo\"!\n> \n> ;-)  That comes from mkdir || fail which is indeed sufficient.\n\nRight.  Often I find it simpler/clearer to let the underlying commands\ngive the errors: they use perror() and can often provide more specific\nerror messages than my script can, unless I spend a lot of effort trying\nto determine exactly what the problem is (permissions, disk space, bad\nsymlink, existing file, whatever).\n\nShould I respin this with the \\\"$new_workdir\\\" -> '$new_workdir' change\n(I actually prefer the latter myself but the former was used somewhere\nso I kept it)?\n"},{"id":"253512","messageId":"1418231817.3947.4.camel@mad-scientist.net","threadId":"38058","inReplyTo":"xmqqk32ho8mc.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4] git-new-workdir: Don't fail if the target directory is empty","fromName":"Paul Smith","fromEmail":"paul@mad-scientist.net","sentAt":"2014-12-10T17:16:57Z","receivedAt":"2014-12-10T17:16:57Z","isPatch":true,"sender":{"key":"paul@mad-scientist.net","avatar":"https://avatars.githubusercontent.com/u/109636?v=4"},"body":"On Wed, 2014-11-26 at 15:16 -0800, Junio C Hamano wrote:\n> Paul Smith <paul@mad-scientist.net> writes:\n> \n> > This is what happens for a file:\n> >\n> > $ rm -f foo\n> >\n> > $ touch foo\n> >\n> > $ ./src/git/contrib/workdir/git-new-workdir src/git foo master\n> > mkdir: cannot create directory ‘foo’: Not a directory\n> > unable to create new workdir \"foo\"!\n> \n> ;-)  That comes from mkdir || fail which is indeed sufficient.\n\nHi Junio;\n\nIs this all set to be applied or is there more to do?\n\nCheers!\n"}]}