Re: [PATCH] git-new-workdir: Don't fail if the target directory is empty

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 2014-11-20 at 09:13 -0800, Junio C Hamano wrote:
> Paul Smith <paul@xxxxxxxxxxxxxxxxx> 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 :-).

> > -# 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]