public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Junio C Hamano <gitster@pobox•com>
To: Paul Smith <paul@mad-scientist•net>
Cc: git@vger•kernel.org
Subject: Re: [PATCH] git-new-workdir: Don't fail if the target directory is empty
Date: Wed, 19 Nov 2014 09:32:32 -0800	[thread overview]
Message-ID: <xmqqd28jt7tb.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1416339408.3899.22.camel@mad-scientist.net> (Paul Smith's message of "Tue, 18 Nov 2014 14:36:48 -0500")

Paul Smith <paul@mad-scientist•net> writes:

> Allow new workdirs to be created in an empty directory (similar to "git
> clone").  Provide more error checking and clean up on failure.
>
> Signed-off-by: Paul Smith <paul@mad-scientist•net>
> ---
>
> Getting rid of ls/wc was not as simple as I'd hoped, due to glob
> pathname expansion (can't rely on nullglob e.g.)  If you want this let
> me know; it will require some yucky code to do the whole thing in native
> shell.  Since new-workdir only works on systems with "ln -s" I think we
> can feel confident requiring "ls" and "wc" as well.
>
>  contrib/workdir/git-new-workdir | 55 +++++++++++++++++++++++++++--------------
>  1 file changed, 36 insertions(+), 19 deletions(-)

I took a look at this again, and I do not agree with one design
decision it makes, namely:

>> I split the creation of the directories from the symlinks: see the new
>> loop above.  This allows us to avoid the icky dirname stuff.

which forces those who maintain the script to make sure that these
two loops

	for dir in log
        do
        	make leading directory $dir
	done
	for path in refs logs/refs objects ...
	do
        	make symlink, assuming the leading directory exists
	done

kept consistent with each other.  If you forget to add frotz to the
upper loop when adding frotz/nitfol to the latter, you are breaking
it.

I find it much more icky than computing what is necessary on the fly.

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

I wondered if -gt instead of -ne is safer just in case what the
description of "-A" alludes to, i.e. "ls -a" may not have ".." in
its output, happens.

But that would not matter.  If you are on such a filesystem and have
a directory with a single file, you will see two entries in "ls -a"
(one "." and the other the sole file) and mistake it as an empty
directory whether you use -ne or -gt anyway, so -gt is not making it
safer anyway.  So let's go with this version.

> +	then
> +		die "destination directory '$new_workdir' is not empty."
> +	fi
> +	cleandir="$new_workdir"/.git
> +else
> +	mkdir -p "$new_workdir" || failed
> +	cleandir="$new_workdir"
>  fi
>  
> -# make sure the links use full paths
> -git_dir=$(cd "$git_dir"; pwd)
> +cleanup () {
> +	rm -rf "$cleandir"
> +}

Aversion to turning $cleandir to an absolute path?  Why?

You may have avoided a problem by removing "cd" below in the current
code, but I do not think it is a good future-proofing.  If you make
sure $cleandir is absolute, then you would not have to be even
worried about other people breaking that assumption "this script
will never let the trap hit while it cd to other places".

> -# 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
> +cp "$git_dir/HEAD" "$new_workdir"/.git/HEAD || failed
> +
> +# the workdir is set up.  if the checkout fails, the user can fix it.
> +trap - $siglist
> +
> +# checkout the branch (either the same as HEAD from the original repository,
> +# or the one that was asked for)
> +git --git-dir="$new_workdir"/.git --work-tree="$new_workdir" checkout -f $branch

These uses of --git-dir/--work-tree look somewhat funny.  You want
to say "I want to run checkout in that $new_workdir", so say it in a
more direct way, i.e.

    git -C "$new_workdir" checkout -f "$branch"

perhaps?

Thanks.

next prev parent reply	other threads:[~2014-11-19 17:32 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-11-18 19:36 [PATCH] git-new-workdir: Don't fail if the target directory is empty Paul Smith
2014-11-18 20:15 ` Junio C Hamano
2014-11-19 17:32 ` Junio C Hamano [this message]
2014-11-20 15:41   ` Paul Smith
  -- strict thread matches above, loose matches on Subject: below --
2014-11-20 15:46 Paul Smith
2014-11-20 17:13 ` Junio C Hamano
2014-11-21 15:08   ` Paul Smith
2014-11-21 17:33     ` Junio C Hamano
2014-11-15 17:49 Paul Smith
2014-11-17 17:22 ` Junio C Hamano
2014-11-18 17:46   ` Paul Smith
2014-11-18 19:32     ` Junio C Hamano
2014-11-18 20:54       ` Paul Smith
2014-11-18 20:58         ` Junio C Hamano
2014-11-18 22:25           ` Paul Smith
2014-11-18 22:51             ` Junio C Hamano
2014-11-19  0:57               ` Paul Smith

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqd28jt7tb.fsf@gitster.dls.corp.google.com \
    --to=gitster@pobox$(echo .)com \
    --cc=git@vger$(echo .)kernel.org \
    --cc=paul@mad-scientist$(echo .)net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox