# [PATCH] git-new-workdir: Don't fail if the target directory is empty 9 messages from 2014-11-15 to 2014-11-19. Participants: Paul Smith, Junio C Hamano. Thread: https://gitlist.dev/t/37978 ## Paul Smith, 2014-11-15 17:49 Subject: [PATCH] git-new-workdir: Don't fail if the target directory is empty Message-ID: <1416073760.9305.174.camel@homebase> URL: https://gitlist.dev/e/1416073760.9305.174.camel%40homebase ``` >From 545c0d526eaa41f9306b567275a7d53799987482 Mon Sep 17 00:00:00 2001 From: Paul Smith 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 --- 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 []" @@ -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 ``` ## Junio C Hamano, 2014-11-17 17:22 Subject: Re: [PATCH] git-new-workdir: Don't fail if the target directory is empty Message-ID: URL: https://gitlist.dev/e/xmqqy4r9yc5u.fsf%40gitster.dls.corp.google.com In-Reply-To: <1416073760.9305.174.camel@homebase> ``` Paul Smith writes: > From 545c0d526eaa41f9306b567275a7d53799987482 Mon Sep 17 00:00:00 2001 > From: Paul Smith > 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 []" > @@ -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 ``` ## Paul Smith, 2014-11-18 17:46 Subject: Re: [PATCH] git-new-workdir: Don't fail if the target directory is empty Message-ID: URL: https://gitlist.dev/e/s934mtwo0zv.fsf%40mad-scientist.net In-Reply-To: ``` Junio C Hamano writes: > Paul Smith 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! ``` ## Junio C Hamano, 2014-11-18 19:32 Subject: Re: [PATCH] git-new-workdir: Don't fail if the target directory is empty Message-ID: URL: https://gitlist.dev/e/xmqq8uj8wbhl.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Paul Smith 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 ( '.' ) 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). ``` ## Paul Smith, 2014-11-18 20:54 Subject: Re: [PATCH] git-new-workdir: Don't fail if the target directory is empty Message-ID: <1416344066.3899.25.camel@mad-scientist.net> URL: https://gitlist.dev/e/1416344066.3899.25.camel%40mad-scientist.net In-Reply-To: ``` On Tue, 2014-11-18 at 12:15 -0800, Junio C Hamano wrote: > Paul Smith 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 ( '.' ) 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! ``` ## Junio C Hamano, 2014-11-18 20:58 Subject: Re: [PATCH] git-new-workdir: Don't fail if the target directory is empty Message-ID: URL: https://gitlist.dev/e/xmqqegt0usy3.fsf%40gitster.dls.corp.google.com In-Reply-To: <1416344066.3899.25.camel@mad-scientist.net> ``` Paul Smith 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"? ``` ## Paul Smith, 2014-11-18 22:25 Subject: Re: [PATCH] git-new-workdir: Don't fail if the target directory is empty Message-ID: <1416349502.3899.30.camel@mad-scientist.net> URL: https://gitlist.dev/e/1416349502.3899.30.camel%40mad-scientist.net In-Reply-To: ``` 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! ``` ## Junio C Hamano, 2014-11-18 22:51 Subject: Re: [PATCH] git-new-workdir: Don't fail if the target directory is empty Message-ID: URL: https://gitlist.dev/e/CAPc5daVy0Oe6UX3VCk_7UdabktbM29MceP9mt0ZZHad1K4HVmQ%40mail.gmail.com In-Reply-To: <1416349502.3899.30.camel@mad-scientist.net> ``` OK, thanks for digging. Let's go with this version, then. On Tue, Nov 18, 2014 at 2:25 PM, Paul Smith 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! > ``` ## Paul Smith, 2014-11-19 00:57 Subject: Re: [PATCH] git-new-workdir: Don't fail if the target directory is empty Message-ID: <1416358650.9305.184.camel@homebase> URL: https://gitlist.dev/e/1416358650.9305.184.camel%40homebase In-Reply-To: ``` 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! ```