{"thread":{"id":"38002","subject":"[PATCH] git-new-workdir: Don't fail if the target directory is empty","startedAt":"2014-11-18T19:36:48Z","lastAt":"2014-11-20T15:41:37Z","messageCount":4,"participants":["Paul Smith","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"252113","messageId":"1416339408.3899.22.camel@mad-scientist.net","threadId":"38002","inReplyTo":null,"subject":"[PATCH] git-new-workdir: Don't fail if the target directory is empty","fromName":"Paul Smith","fromEmail":"paul@mad-scientist.net","sentAt":"2014-11-18T19:36:48Z","receivedAt":"2014-11-18T19:36:48Z","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\nGetting rid of ls/wc was not as simple as I'd hoped, due to glob\npathname expansion (can't rely on nullglob e.g.)  If you want this let\nme know; it will require some yucky code to do the whole thing in native\nshell.  Since new-workdir only works on systems with \"ln -s\" I think we\ncan feel confident requiring \"ls\" and \"wc\" as well.\n\n contrib/workdir/git-new-workdir | 55 +++++++++++++++++++++++++++--------------\n 1 file changed, 36 insertions(+), 19 deletions(-)\n\ndiff --git a/contrib/workdir/git-new-workdir b/contrib/workdir/git-new-workdir\nindex 75e8b25..aef632d 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@@ -48,35 +52,48 @@ 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)\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+\tmkdir -p \"$new_workdir\" || failed\n+\tcleandir=\"$new_workdir\"\n fi\n \n-# make sure the links use full paths\n-git_dir=$(cd \"$git_dir\"; pwd)\n+cleanup () {\n+\trm -rf \"$cleandir\"\n+}\n+siglist=\"0 1 2 15\"\n+trap cleanup $siglist\n \n-# create the workdir\n-mkdir -p \"$new_workdir/.git\" || die \"unable to create \\\"$new_workdir\\\"!\"\n+# create embedded directories\n+for x in logs\n+do\n+\tmkdir -p \"$new_workdir/.git/$x\" || failed\n+done\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-\tcase $x in\n-\t*/*)\n-\t\tmkdir -p \"$(dirname \"$new_workdir/.git/$x\")\"\n-\t\t;;\n-\tesac\n-\tln -s \"$git_dir/$x\" \"$new_workdir/.git/$x\"\n+\tln -s \"$git_dir/$x\" \"$new_workdir/.git/$x\" || failed\n done\n \n-# now setup the workdir\n-cd \"$new_workdir\"\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-git checkout -f $branch\n+cp \"$git_dir/HEAD\" \"$new_workdir\"/.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 --git-dir=\"$new_workdir\"/.git --work-tree=\"$new_workdir\" checkout -f $branch\n-- \n1.8.5.3\n"},{"id":"252119","messageId":"xmqqvbmcuuyd.fsf@gitster.dls.corp.google.com","threadId":"38002","inReplyTo":"1416339408.3899.22.camel@mad-scientist.net","subject":"Re: [PATCH] git-new-workdir: Don't fail if the target directory is empty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-18T20:15:06Z","receivedAt":"2014-11-18T20:15:06Z","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> Getting rid of ls/wc was not as simple as I'd hoped,\n\nI didn't say ls/wc was not portable.  Assuming that a directory for\nwhich the output from \"ls -a\" has two entries is empty may be.\n"},{"id":"252185","messageId":"xmqqd28jt7tb.fsf@gitster.dls.corp.google.com","threadId":"38002","inReplyTo":"1416339408.3899.22.camel@mad-scientist.net","subject":"Re: [PATCH] git-new-workdir: Don't fail if the target directory is empty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-19T17:32:32Z","receivedAt":"2014-11-19T17:32:32Z","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> Getting rid of ls/wc was not as simple as I'd hoped, due to glob\n> pathname expansion (can't rely on nullglob e.g.)  If you want this let\n> me know; it will require some yucky code to do the whole thing in native\n> shell.  Since new-workdir only works on systems with \"ln -s\" I think we\n> can feel confident requiring \"ls\" and \"wc\" as well.\n>\n>  contrib/workdir/git-new-workdir | 55 +++++++++++++++++++++++++++--------------\n>  1 file changed, 36 insertions(+), 19 deletions(-)\n\nI took a look at this again, and I do not agree with one design\ndecision it makes, namely:\n\n>> I split the creation of the directories from the symlinks: see the new\n>> loop above.  This allows us to avoid the icky dirname stuff.\n\nwhich forces those who maintain the script to make sure that these\ntwo loops\n\n\tfor dir in log\n        do\n        \tmake leading directory $dir\n\tdone\n\tfor path in refs logs/refs objects ...\n\tdo\n        \tmake symlink, assuming the leading directory exists\n\tdone\n\nkept consistent with each other.  If you forget to add frotz to the\nupper loop when adding frotz/nitfol to the latter, you are breaking\nit.\n\nI find it much more icky than computing what is necessary on the fly.\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\nI wondered if -gt instead of -ne is safer just in case what the\ndescription of \"-A\" alludes to, i.e. \"ls -a\" may not have \"..\" in\nits output, happens.\n\nBut that would not matter.  If you are on such a filesystem and have\na directory with a single file, you will see two entries in \"ls -a\"\n(one \".\" and the other the sole file) and mistake it as an empty\ndirectory whether you use -ne or -gt anyway, so -gt is not making it\nsafer anyway.  So let's go with this version.\n\n> +\tthen\n> +\t\tdie \"destination directory '$new_workdir' is not empty.\"\n> +\tfi\n> +\tcleandir=\"$new_workdir\"/.git\n> +else\n> +\tmkdir -p \"$new_workdir\" || failed\n> +\tcleandir=\"$new_workdir\"\n>  fi\n>  \n> -# make sure the links use full paths\n> -git_dir=$(cd \"$git_dir\"; pwd)\n> +cleanup () {\n> +\trm -rf \"$cleandir\"\n> +}\n\nAversion to turning $cleandir to an absolute path?  Why?\n\nYou may have avoided a problem by removing \"cd\" below in the current\ncode, but I do not think it is a good future-proofing.  If you make\nsure $cleandir is absolute, then you would not have to be even\nworried about other people breaking that assumption \"this script\nwill never let the trap hit while it cd to other places\".\n\n> -# now setup the workdir\n> -cd \"$new_workdir\"\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> -git checkout -f $branch\n> +cp \"$git_dir/HEAD\" \"$new_workdir\"/.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 --git-dir=\"$new_workdir\"/.git --work-tree=\"$new_workdir\" checkout -f $branch\n\nThese uses of --git-dir/--work-tree look somewhat funny.  You want\nto say \"I want to run checkout in that $new_workdir\", so say it in a\nmore direct way, i.e.\n\n    git -C \"$new_workdir\" checkout -f \"$branch\"\n\nperhaps?\n\nThanks.\n"},{"id":"252259","messageId":"1416498097.23953.5.camel@mad-scientist.net","threadId":"38002","inReplyTo":"xmqqd28jt7tb.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-new-workdir: Don't fail if the target directory is empty","fromName":"Paul Smith","fromEmail":"paul@mad-scientist.net","sentAt":"2014-11-20T15:41:37Z","receivedAt":"2014-11-20T15:41:37Z","isPatch":true,"sender":{"key":"paul@mad-scientist.net","avatar":"https://avatars.githubusercontent.com/u/109636?v=4"},"body":"On Wed, 2014-11-19 at 09:32 -0800, Junio C Hamano wrote:\n> Paul Smith <paul@mad-scientist.net> writes:\n> I took a look at this again, and I do not agree with one design\n> decision it makes, namely:\n> \n> >> I split the creation of the directories from the symlinks: see the new\n> >> loop above.  This allows us to avoid the icky dirname stuff.\n> \n> which forces those who maintain the script to make sure that these\n> two loops\n> kept consistent with each other.  If you forget to add frotz to the\n> upper loop when adding frotz/nitfol to the latter, you are breaking\n> it.\n\nYes, but this mistake won't live past even a single attempt to run the\nscript, since it will immediately cause a fatal error.  So this doesn't\nbother me.\n\n> I find it much more icky than computing what is necessary on the fly.\n\nOK, I'll go back to the previous way.\n\n> Aversion to turning $cleandir to an absolute path?  Why?\n\nIn some situations switching to absolute can cause unexpected behaviors,\nwhere relative paths are no longer where you expect them etc.  However\nin this case it's not a problem: I'll change it.\n\n> These uses of --git-dir/--work-tree look somewhat funny.  You want\n> to say \"I want to run checkout in that $new_workdir\", so say it in a\n> more direct way, i.e.\n> \n>     git -C \"$new_workdir\" checkout -f \"$branch\"\n\nGood idea.\n"}]}