Re: [PATCH] git-new-workdir: Don't fail if the target directory is empty
- From
Paul Smith <paul@mad-scientist.net>
- Date
- Nov 21, 2014, 15:08 UTC
- Message-ID
- <1416582517.23953.18.camel@mad-scientist.net>
- In-Reply-To
- <xmqqioi94wy0.fsf@gitster.dls.corp.google.com>
On Thu, 2014-11-20 at 09:13 -0800, Junio C Hamano wrote:
Show 12 quoted lines
> Paul Smith <paul@mad-scientist.net> writes: > > +# 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 > > You used to quote this as > > if test $(ls -a1 "$new_workdir/." | wc -l) -ne 2 > > and I think that made a lot more sense than the new quoting.
Just personal script coding style. I tend to quote just variables, in case I want to add globbing or whatever later. I've run into people who think that "$foo/bar" needs quotes because it concatenates two strings, but you don't need to quote $foo by itself; quoting just the variable helps avoid that impression.
But this doesn't matter much to me.
> Case arms come at the same indentation level as "case/esac".
OK.
> Why these changes? I thought you are making sure $cleandir is > absolute so that you do not have to do this and can just "cd" into > the new working tree, the same way the user would do once it is set > up.
I made cleandir absolute mainly because you asked me to :-). I didn't realize that the implication behind that request was that I'd put back the original "cd" as well.
I personally distrust and avoid raw cd in shell scripts. It's action-at-a-distance and can cause all sorts of problems, sometimes disastrous ones. If I need to change directories I localize it, e.g.:
(cd "$somedir" && do some command)
so it's clear that it's in effect only for that command.
I understand that in this particular situation the "cd" is right before the end so this doesn't hurt (as the script stands today). If you prefer the "cd" explicitly I'll add it back. You're the one that needs to be happy with it :-).
What if I move the cd down to right before the checkout command, and use $new_workdir in the copy of HEAD? At least it won't be in effect until after the workdir is completely set up. I would feel better about it then :-).
Show 7 quoted lines
> > -# now setup the workdir > > -cd "$new_workdir" > > # copy the HEAD from the original repository as a default branch > > -cp "$git_dir/HEAD" .git/HEAD > > -# checkout the branch (either the same as HEAD from the original repository, or > > -# the one that was asked for) > > -git checkout -f $branch