{"thread":{"id":"37978","subject":"[PATCH] git-new-workdir: Don't fail if the target directory is empty","startedAt":"2014-11-15T17:49:20Z","lastAt":"2014-11-19T00:57:30Z","messageCount":9,"participants":["Paul Smith","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"251946","messageId":"1416073760.9305.174.camel@homebase","threadId":"37978","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-15T17:49:20Z","receivedAt":"2014-11-15T17:49:20Z","isPatch":true,"sender":{"key":"paul@mad-scientist.net","avatar":"https://avatars.githubusercontent.com/u/109636?v=4"},"body":">From 545c0d526eaa41f9306b567275a7d53799987482 Mon Sep 17 00:00:00 2001\nFrom: Paul Smith <paul@mad-scientist.net>\nDate: Fri, 14 Nov 2014 17:11:19 -0500\nSubject: [PATCH] git-new-workdir: Don't fail if the target directory is empty\n\nAlso provide more error checking and clean up on failure.\n\nSigned-off-by: Paul Smith <paul@mad-scientist.net>\n---\n\nThanks Junio.  I've reworked the change so it will automatically succeed\nif the directory does not exists or exists but is empty, and fail\notherwise, which as far as I can tell is the behavior \"git clone\" uses\nas well.  I removed the -f flag as no longer needed.  I also added some\ncleanup that is performed if the new-workdir operation fails for any\nreason so you don't get partly-constructed workdirs.  I also added more\nerror checking so that we immediately stop if any step fails.  Some may\nsuggest \"set -e\" but that flag can be tricky... I preferred to make the\nfailure explicit.\n\n contrib/workdir/git-new-workdir | 54 +++++++++++++++++++++++++++++------------\n 1 file changed, 39 insertions(+), 15 deletions(-)\n\ndiff --git a/contrib/workdir/git-new-workdir b/contrib/workdir/git-new-workdir\nindex 75e8b25..c402000 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,55 @@ 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 use full paths\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+\twas_existing=true\n+else\n+\tmkdir -p \"$new_workdir\" || failed\n+\twas_existing=false\n fi\n \n-# make sure the links use full paths\n-git_dir=$(cd \"$git_dir\"; pwd)\n+cleanup () {\n+\tif $was_existing\n+\tthen\n+\t\trm -rf \"$new_workdir\"/* \"$new_workdir\"/.[!.] \"$new_workdir\"/.??*\n+\telse\n+\t\trm -rf \"$new_workdir\"\n+\tfi\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+cd \"$new_workdir\" || failed\n # copy the HEAD from the original repository as a default branch\n-cp \"$git_dir/HEAD\" .git/HEAD\n+cp \"$git_dir/HEAD\" .git/HEAD || failed\n+\n+# don't delete the new workdir on exit\n+trap - $siglist\n+\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-- \n1.8.5.3\n"},{"id":"251989","messageId":"xmqqy4r9yc5u.fsf@gitster.dls.corp.google.com","threadId":"37978","inReplyTo":"1416073760.9305.174.camel@homebase","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-17T17:22:53Z","receivedAt":"2014-11-17T17:22:53Z","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> From 545c0d526eaa41f9306b567275a7d53799987482 Mon Sep 17 00:00:00 2001\n> From: Paul Smith <paul@mad-scientist.net>\n> Date: Fri, 14 Nov 2014 17:11:19 -0500\n> Subject: [PATCH] git-new-workdir: Don't fail if the target directory is empty\n\nPlease do not paste these in your mail message body.  The first line\nis there only so that the output looks like traditinal mbox format\n(i.e. each of these act as a signal that a new message starts there\nin the file), the other three are there only for rare cases in which\nyou want to override what your e-mail's headers say and is only used\nwhen you are sending somebody else's patch.\n\n> Also provide more error checking and clean up on failure.\n\nAs the body of the log message is supposed to be complete by itself,\nnot a continuation of a half-sentence started on the Subject:\n(i.e. consider the subject line to be the title of an article you\nare writing), starting it with \"Also\" is strange.\n\nNo need to resend only to correct the above, even though there may\nbe comments on the patch itself from me or others that may make you\nwant to reroll this patch, in which case I'd like to see these nits\ngone.\n\nThanks.\n\n\n> diff --git a/contrib/workdir/git-new-workdir b/contrib/workdir/git-new-workdir\n> index 75e8b25..c402000 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,55 @@ 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 use full paths\n> +git_dir=$(cd \"$git_dir\"; pwd)\n\nWith this change, the comment gets much harder to understand.  \"What links?\"\nwould be the reaction from those who are reading the patch.\n\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\nI wonder if this check is portable for all platforms we care about,\nbut that is OK, as it should be so for the ones I think of and care\nabout ;-)\n\n> +\tfi\n> +\twas_existing=true\n> +else\n> +\tmkdir -p \"$new_workdir\" || failed\n> +\twas_existing=false\n>  fi\n>  \n> -# make sure the links use full paths\n> -git_dir=$(cd \"$git_dir\"; pwd)\n> +cleanup () {\n> +\tif $was_existing\n> +\tthen\n> +\t\trm -rf \"$new_workdir\"/* \"$new_workdir\"/.[!.] \"$new_workdir\"/.??*\n> +\telse\n> +\t\trm -rf \"$new_workdir\"\n> +\tfi\n\nThe script chdirs around; did you turn $new_workdir into an absolute\npath already, or given a relative $new_workdir this is attempting to\nremove a hierarchy that is different from what you created?\n\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\nWhat's this removal about?  If $new_workdir/.git/logs does not\nexist, would \"ln -s $there/logs/refs $new_workdir/.git/logs/refs\"\nsucceed without first creating $new_workdir/.git/logs directory?\n\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> +cd \"$new_workdir\" || failed\n>  # copy the HEAD from the original repository as a default branch\n> -cp \"$git_dir/HEAD\" .git/HEAD\n> +cp \"$git_dir/HEAD\" .git/HEAD || failed\n> +\n> +# don't delete the new workdir on exit\n> +trap - $siglist\n> +\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":"252109","messageId":"s934mtwo0zv.fsf@mad-scientist.net","threadId":"37978","inReplyTo":"xmqqy4r9yc5u.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-18T17:46:28Z","receivedAt":"2014-11-18T17:46:28Z","isPatch":true,"sender":{"key":"paul@mad-scientist.net","avatar":"https://avatars.githubusercontent.com/u/109636?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n> Paul Smith <paul@mad-scientist.net> writes:\n\n> No need to resend only to correct the above, even though there may\n> be comments on the patch itself from me or others that may make you\n> want to reroll this patch, in which case I'd like to see these nits\n> gone.\n\nI'll fix in the next iteration.\n\n>> -# don't recreate a workdir over an existing repository\n>> -if test -e \"$new_workdir\"\n>> +# make sure the links use full paths\n>> +git_dir=$(cd \"$git_dir\"; pwd)\n>\n> With this change, the comment gets much harder to understand.  \"What\n> links?\"  would be the reaction from those who are reading the patch.\n\nI just moved this line; I don't think it had much more context\nbeforehand, but I'll definitely rewrite the comment to be more clear.\n\n>> +\tif test $(ls -a1 \"$new_workdir/.\" | wc -l) -ne 2\n\n> I wonder if this check is portable for all platforms we care about,\n> but that is OK, as it should be so for the ones I think of and care\n> about ;-)\n\nDo you mean \".\" and \"..\" representing an empty directory?  That will\nwork on any system where /bin/sh works, for sure.\n\nOr do you mean using \"ls\" and \"wc\"?  I can easily avoid this.\n\nRecall that new-workdir itself only works on systems that support\nsymlinks; this limits its portability.  For example it doesn't work on\nWindows (unfortunately).  I spent the better part of a day a few months\nago playing with the various \"symlink-ish\" capabilities of Windows NTFS\nand couldn't find any reliable way to make this work (although I have\nvirtually no Windows fu).\n\n> The script chdirs around; did you turn $new_workdir into an absolute\n> path already, or given a relative $new_workdir this is attempting to\n> remove a hierarchy that is different from what you created?\n\nGood point, I'll fix this up\n\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\n>> working\n>>  # directory, and should not be shared.\n>>  for x in config refs logs/refs objects info hooks packed-refs remotes\n>> 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>\n> What's this removal about?  If $new_workdir/.git/logs does not\n> exist, would \"ln -s $there/logs/refs $new_workdir/.git/logs/refs\"\n> succeed without first creating $new_workdir/.git/logs directory?\n\nI split the creation of the directories from the symlinks: see the new\nloop above.  This allows us to avoid the icky dirname stuff.\n\nNew iteration will follow shortly.\n\nCheers!\n"},{"id":"252111","messageId":"xmqq8uj8wbhl.fsf@gitster.dls.corp.google.com","threadId":"37978","inReplyTo":"s934mtwo0zv.fsf@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-18T19:32:38Z","receivedAt":"2014-11-18T19:32:38Z","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>>> +\tif test $(ls -a1 \"$new_workdir/.\" | wc -l) -ne 2\n>\n>> I wonder if this check is portable for all platforms we care about,\n>> but that is OK, as it should be so for the ones I think of and care\n>> about ;-)\n>\n> Do you mean \".\" and \"..\" representing an empty directory?  That will\n> work on any system where /bin/sh works, for sure.\n\nEven on network mounts from esoteric filesystems and such?  When\n\n    http://pubs.opengroup.org/onlinepubs/9699919799/utilities/ls.html\n\nmentions the \"-A\" option, it says:\n\n    -A\n        Write out all directory entries, including those whose names\n        begin with a <period> ( '.' ) but excluding the entries dot\n        and dot-dot (if they exist).\n\nThe \"if they exist\" part suggests, at least to me, that it is valid\nfor a POSIX filesystem to lack these two (I suspect that one may be\nable to find a more definitive answer from other parts of the POSIX\nbut I didn't bother).\n"},{"id":"252127","messageId":"1416344066.3899.25.camel@mad-scientist.net","threadId":"37978","inReplyTo":"xmqq8uj8wbhl.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-18T20:54:26Z","receivedAt":"2014-11-18T20:54:26Z","isPatch":true,"sender":{"key":"paul@mad-scientist.net","avatar":"https://avatars.githubusercontent.com/u/109636?v=4"},"body":"On Tue, 2014-11-18 at 12:15 -0800, Junio C Hamano wrote:\n> Paul Smith <paul@mad-scientist.net> writes:\n> \n> > Getting rid of ls/wc was not as simple as I'd hoped,\n> \n> I didn't say ls/wc was not portable.  Assuming that a directory for\n> which the output from \"ls -a\" has two entries is empty may be.\n\nYes, I didn't get your email before I sent the latest patch.  Sorry\nabout that.\n\nOn Tue, 2014-11-18 at 11:32 -0800, Junio C Hamano wrote:\n> Even on network mounts from esoteric filesystems and such?  When\n> \n>     http://pubs.opengroup.org/onlinepubs/9699919799/utilities/ls.html\n> \n> mentions the \"-A\" option, it says:\n> \n>     -A\n>         Write out all directory entries, including those whose names\n>         begin with a <period> ( '.' ) but excluding the entries dot\n>         and dot-dot (if they exist).\n> \n> The \"if they exist\" part suggests, at least to me, that it is valid\n> for a POSIX filesystem to lack these two (I suspect that one may be\n> able to find a more definitive answer from other parts of the POSIX\n> but I didn't bother).\n\nHm.  Well, POSIX clearly reserves \".\" and \"..\" to be special and\nrequires that directories containing only those values are considered\nempty (rmdir(2) says so).  The definitions section contains special\nwording for \"dot\" and \"dot-dot\".\n\nLooking at\nhttp://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xbd_chap04.html\n\n        Each directory has exactly one parent directory which is\n        represented by the name dot-dot in the first directory.\n\nwhich implies that every directory must have \"..\".  This is in the\nRationale, I realize.\n\nThere are also various mentions to using \".\" as a synonym for the\ncurrent directory.\n\nI can't find a clear statement that both are required and that \"ls -a\"\nmust show them.  I've used a wide range of UNIX-en and filesystems for\n30 years or so and never seen one that didn't provide them.  It seems\nlike it would break quite a few scripts, at least.  Even virtual\nfilesystems like ClearCase's MVFS provide \".\" and \"..\".\n\nIf you want to allow for this possibility I can do so but it would be a\nbit crufty.  Personally I think it would be overkill but you're the\nboss: let me know :-).\n\nCheers!\n"},{"id":"252129","messageId":"xmqqegt0usy3.fsf@gitster.dls.corp.google.com","threadId":"37978","inReplyTo":"1416344066.3899.25.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:58:28Z","receivedAt":"2014-11-18T20:58:28Z","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> I can't find a clear statement that both are required and that \"ls -a\"\n> must show them.  I've used a wide range of UNIX-en and filesystems for\n> 30 years or so and never seen one that didn't provide them.  It seems\n> like it would break quite a few scripts, at least.  Even virtual\n> filesystems like ClearCase's MVFS provide \".\" and \"..\".\n>\n> If you want to allow for this possibility I can do so but it would be a\n> bit crufty.  Personally I think it would be overkill but you're the\n> boss: let me know :-).\n\nDoesn't the description of the -A option I quoted upthread hint a\nsimpler and clearer solution?  I.e. \"test $(ls -A | wc -l) = 0\"?\n"},{"id":"252139","messageId":"1416349502.3899.30.camel@mad-scientist.net","threadId":"37978","inReplyTo":"xmqqegt0usy3.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-18T22:25:02Z","receivedAt":"2014-11-18T22:25:02Z","isPatch":true,"sender":{"key":"paul@mad-scientist.net","avatar":"https://avatars.githubusercontent.com/u/109636?v=4"},"body":"On Tue, 2014-11-18 at 12:58 -0800, Junio C Hamano wrote:\n> Doesn't the description of the -A option I quoted upthread hint a\n> simpler and clearer solution?  I.e. \"test $(ls -A | wc -l) = 0\"?\n\nYes, but unfortunately for us the -A flag was added to POSIX Issue 7.\nIt's not present in the previous version of POSIX, Issue 6:\n\nhttp://pubs.opengroup.org/onlinepubs/009695399/utilities/ls.html\n\nIt came from the BSD world, so it might not be available on older\nSysV-derived systems (AIX, HP-UX, even Solaris... I don't have access to\nthese anymore so I can't say).  Ultimately it's probably more portable\nto assume \"ls -a\" always prints \".\" and \"..\" than to assume \"ls -A\" is\nsupported.\n\nCheers!\n"},{"id":"252141","messageId":"CAPc5daVy0Oe6UX3VCk_7UdabktbM29MceP9mt0ZZHad1K4HVmQ@mail.gmail.com","threadId":"37978","inReplyTo":"1416349502.3899.30.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-18T22:51:36Z","receivedAt":"2014-11-18T22:51:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"OK, thanks for digging. Let's go with this version, then.\n\n\nOn Tue, Nov 18, 2014 at 2:25 PM, Paul Smith <paul@mad-scientist.net> wrote:\n> On Tue, 2014-11-18 at 12:58 -0800, Junio C Hamano wrote:\n>> Doesn't the description of the -A option I quoted upthread hint a\n>> simpler and clearer solution?  I.e. \"test $(ls -A | wc -l) = 0\"?\n>\n> Yes, but unfortunately for us the -A flag was added to POSIX Issue 7.\n> It's not present in the previous version of POSIX, Issue 6:\n>\n> http://pubs.opengroup.org/onlinepubs/009695399/utilities/ls.html\n>\n> It came from the BSD world, so it might not be available on older\n> SysV-derived systems (AIX, HP-UX, even Solaris... I don't have access to\n> these anymore so I can't say).  Ultimately it's probably more portable\n> to assume \"ls -a\" always prints \".\" and \"..\" than to assume \"ls -A\" is\n> supported.\n>\n> Cheers!\n>\n"},{"id":"252154","messageId":"1416358650.9305.184.camel@homebase","threadId":"37978","inReplyTo":"CAPc5daVy0Oe6UX3VCk_7UdabktbM29MceP9mt0ZZHad1K4HVmQ@mail.gmail.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-19T00:57:30Z","receivedAt":"2014-11-19T00:57:30Z","isPatch":true,"sender":{"key":"paul@mad-scientist.net","avatar":"https://avatars.githubusercontent.com/u/109636?v=4"},"body":"On Tue, 2014-11-18 at 14:51 -0800, Junio C Hamano wrote:\n> OK, thanks for digging. Let's go with this version, then.\n\nThanks for your attention, Junio!\n"}]}