{"thread":{"id":"38017","subject":"[PATCH] git-new-workdir: Don't fail if the target directory is empty","startedAt":"2014-11-20T15:46:35Z","lastAt":"2014-11-21T17:33:08Z","messageCount":4,"participants":["Paul Smith","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"252260","messageId":"1416498395.23953.8.camel@mad-scientist.net","threadId":"38017","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-20T15:46:35Z","receivedAt":"2014-11-20T15:46:35Z","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 contrib/workdir/git-new-workdir | 54 +++++++++++++++++++++++++++--------------\n 1 file changed, 36 insertions(+), 18 deletions(-)\n\ndiff --git a/contrib/workdir/git-new-workdir b/contrib/workdir/git-new-workdir\nindex 75e8b25..7334720 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,49 @@ 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\t;;\n+\t\t*/*) mkdir -p \"$new_workdir/.git/${x%/*}\" ;;\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 # 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 -C \"$new_workdir\" checkout -f $branch\n-- \n1.8.5.3\n"},{"id":"252265","messageId":"xmqqioi94wy0.fsf@gitster.dls.corp.google.com","threadId":"38017","inReplyTo":"1416498395.23953.8.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-20T17:13:27Z","receivedAt":"2014-11-20T17:13:27Z","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>  contrib/workdir/git-new-workdir | 54 +++++++++++++++++++++++++++--------------\n>  1 file changed, 36 insertions(+), 18 deletions(-)\n\nThanks.  I would have liked [PATCH v$n] on the subject line but\nthat's minor.\n\nI also see you made a few small improvements here and there, like...\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\n... this bit, which I think is a good idea.  We are asking \"--bool\"\nso we know we will never get anything other than true/false to cause\ntrouble by not quoting, but the dq-pair there does not cost anything\nand it is a good discipline.\n\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... so is this bit, which used to just go even if cd failed.\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\nYou used to quote this as\n\n\tif test $(ls -a1 \"$new_workdir/.\" | wc -l) -ne 2\n\nand I think that made a lot more sense than the new quoting.\n\nThe ultimate reason we quote here is because \"$new_workdir/.\" is the\nsingle directory path we want to give \"ls\", and quoting the whole\nthing shows that intention far clearer than the new one, which says\n\"there may be an $IFS character in $new_workdir so I am quoting to\nkeep them together as a single string and then appending /. to that\nsingle string will still be a single string\", which is not wrong\nper-se in that it is saying the same thing, but it is a roundabout\nway to do so.\n\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\t;;\n> +\t\t*/*) mkdir -p \"$new_workdir/.git/${x%/*}\" ;;\n\nCase arms come at the same indentation level as \"case/esac\".\n\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>  # 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\nWhy these changes?  I thought you are making sure $cleandir is\nabsolute so that you do not have to do this and can just \"cd\" into\nthe new working tree, the same way the user would do once it is set\nup.\n\nPuzzled...  Before seeing this bit, I was about to say \"but all the\nabove nits are minor so no need to resend---I'll locally fix them up\nwhile queueing\", but I suspect that I may be missing some obvious\nreason why you avoid \"cd\" and I am reluctant to \"fix\" this part up\nmyself without seeing a response, so I'd swallow that \"no need to\nresend\" X-<.\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 -C \"$new_workdir\" checkout -f $branch\n"},{"id":"252342","messageId":"1416582517.23953.18.camel@mad-scientist.net","threadId":"38017","inReplyTo":"xmqqioi94wy0.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-21T15:08:37Z","receivedAt":"2014-11-21T15:08:37Z","isPatch":true,"sender":{"key":"paul@mad-scientist.net","avatar":"https://avatars.githubusercontent.com/u/109636?v=4"},"body":"On Thu, 2014-11-20 at 09:13 -0800, Junio C Hamano wrote:\n> Paul Smith <paul@mad-scientist.net> writes:\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> \n> You used to quote this as\n> \n> \tif test $(ls -a1 \"$new_workdir/.\" | wc -l) -ne 2\n> \n> and I think that made a lot more sense than the new quoting.\n\nJust personal script coding style.  I tend to quote just variables, in\ncase I want to add globbing or whatever later.  I've run into people who\nthink that \"$foo/bar\" needs quotes because it concatenates two strings,\nbut you don't need to quote $foo by itself; quoting just the variable\nhelps avoid that impression.\n\nBut this doesn't matter much to me.\n\n> Case arms come at the same indentation level as \"case/esac\".\n\nOK.\n\n> Why these changes?  I thought you are making sure $cleandir is\n> absolute so that you do not have to do this and can just \"cd\" into\n> the new working tree, the same way the user would do once it is set\n> up.\n\nI made cleandir absolute mainly because you asked me to :-).  I didn't\nrealize that the implication behind that request was that I'd put back\nthe original \"cd\" as well.\n\nI personally distrust and avoid raw cd in shell scripts.  It's\naction-at-a-distance and can cause all sorts of problems, sometimes\ndisastrous ones.  If I need to change directories I localize it, e.g.:\n\n  (cd \"$somedir\" && do some command)\n\nso it's clear that it's in effect only for that command.\n\nI understand that in this particular situation the \"cd\" is right before\nthe end so this doesn't hurt (as the script stands today).  If you\nprefer the \"cd\" explicitly I'll add it back.  You're the one that needs\nto be happy with it :-).\n\nWhat if I move the cd down to right before the checkout command, and use\n$new_workdir in the copy of HEAD?  At least it won't be in effect until\nafter the workdir is completely set up.  I would feel better about it\nthen :-).\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"},{"id":"252348","messageId":"xmqqegsw1msr.fsf@gitster.dls.corp.google.com","threadId":"38017","inReplyTo":"1416582517.23953.18.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-21T17:33:08Z","receivedAt":"2014-11-21T17:33:08Z","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>> Why these changes?  I thought you are making sure $cleandir is\n>> absolute so that you do not have to do this and can just \"cd\" into\n>> the new working tree, the same way the user would do once it is set\n>> up.\n>\n> I made cleandir absolute mainly because you asked me to :-).  I didn't\n> realize that the implication behind that request was that I'd put back\n> the original \"cd\" as well.\n\nThis is about not changing unnecessarily the original that has been\nbattle-tested and keeping the result resistant to future mistakes\n(i.e. somebody going elsewhere without realizing a relative cleandir\nwill cause problems).  Once $cleandir is fixed, there no longer is a\ngood reason to change how HEAD is pointed in the $new_workdir or its\nworking tree populated; the change in the earlier round in that area\nwas necessary when $cleandir was not absolute, but not anymore.\n"}]}