From: Junio C Hamano <junkio@cox.net>
Date: Tue, 27 Mar 2007 05:59:24 GMT
Subject: Re: [PATCH] contrib/workdir: add a simple script to create a working directory
Message-ID: <7vabxzrzur.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <20070327003033.4226.8413.julian@quantumfyre.co.uk>

Julian Phillips <julian@quantumfyre.co.uk> writes:

> diff --git a/contrib/workdir/git-new-workdir b/contrib/workdir/git-new-workdir
> new file mode 100755
> index 0000000..5bfd87e
> --- /dev/null
> +++ b/contrib/workdir/git-new-workdir
> @@ -0,0 +1,55 @@
> +#!/bin/bash

I do not see anything bash specific you need to do in your
script.

> +
> +function usage () {
> +	echo "usage:" $1;
> +	exit 127;
> +}
> +
> +function die () {
> +	echo $1;
> +	exit 128;
> +}

Do not add noiseword "function" in our shell scripts, please.
This is the only thing POSIX says "produces unspecified results"
I found in your script, so if you lose them you shouldn't have
to say "#!/bin/bash".

> +test $# -eq 3 || usage "$0 <original> <new_workdir> <branch>";
> +
> +orig_git=$1;
> +new_workdir=$2;
> +branch=$3;

Perhaps default branch to whatever original's HEAD points at?

> +
> +# want to make sure that what is pointed to has a .git directory ...
> +test -d ${orig_git}/.git || die "${original_git} is not a git repository!";
> +
> +# don't link to a workdir, link to the original repo the workdir is linked to
> +if test -L ${orig_git}/.git/config
> +then
> +	orig_git=$(dirname $(dirname $(readlink -f gm/.git/config)));
> +fi

"gm"?  I think it is not worth doing this, as readlink is not
all that portable.  Just see if it is a symlink and error out.

Dq all pathname values you get from the user, like "$orig_git".
They may have SP in them.

Do you need all those braces around shell variable names?

> +# vim: tabstop=8
> +# vim: noexpandtab

Lose these two lines, please.