Re: [PATCHv2 1/2] Add git alternate command

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 29, 2010, 07:32 UTC
Message-ID: <7vr5n37fci.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1269497251-13103-2-git-send-email-judge.packham@gmail.com>

Chris Packham <judge.packham@gmail.com> writes:

Show 6 quoted lines

> +abspath()
> +{
> +	cd "$1"
> +	pwd
> +	cd - > /dev/null
> +}

I do not think "cd -" is all that portable. As you will be always using this in this form:

	somevariable=$(abspath "$it")

you are in a subshell and won't affect the caller anyway. Why not drop the "go back to where we came from"?

Also a shell built-in "pwd" tends to be fooled by $PWD especially since you are running "cd" without -P.

Show 6 quoted lines

> +#
> +# Runs through the alternates file calling the callback function $1
> +# with the name of the alternate as the first argument to the callback
> +# any additional arguments are passed to the callback function.
> +#
> +walk_alternates()

This is more like "for_each_alternates"; "walk" can be mistaken as if you are recursively looking at alternates defined in alternate repositories of the repository you start from.

Show 11 quoted lines

> +{
> +	alternates=$GIT_DIR/objects/info/alternates
> +	callback=$1
> +	shift
> +
> +	if test -f "$alternates"
> +	then
> +		while read line
> +		do
> +			$callback "$line" "$@"
> +		done< "$alternates"

	done <"$alternates"

How well does this handle relative alternate object stores? Shouldn't it be more like this?

	while read altdir
        do
        	case "$altdir" in
                /*) ;; # full path
                *) altdir="$GIT_DIR/objects/$altdir" ;;
                esac &&
                $callback "$altdir" "$@"
	done <"$alternates"

Show 10 quoted lines

> +# Walk function to display one alternate object store and, if the user
> +# has specified -r, recursively call show_alternates on the git 
> +# repository that the object store belongs to.
> +#
> +show_alternates_walk()
> +{
> +	say "Object store $1"
> +	say "    referenced via $GIT_DIR"
> +
> +	new_git_dir=${line%%/objects}

Do you need double-% here, not a single one?

Show 12 quoted lines

> +# Add a new alternate
> +add_alternate()
> +{
> +	if test ! -d "$dir"; then
> +		die "fatal: $dir is not a directory"
> +	fi
> +
> +	abs_dir=$(abspath "$dir")
> +	walk_alternates check_current_alternate_walk "$abs_dir"
> +
> +	# At this point we know that $dir is a directory that exists
> +	# and that its not already being used as an alternate. We could

s/its/(it's|it is)/;

But I don't think it is true that you have verified that it is not used. You are running abspath on the input from the end user, but you are using existing entries in the alternates file that may not be absolute. They can be relative to $GIT_DIR/objects/.

> +	say "    use 'git repack -adl' to remove duplicate objects"

Good.

Show 9 quoted lines

> +# Deletes the name alternate from the alternates file.
> +# If there are no more alternates the alternates file will be removed
> +del_alternate()
> +{
> +	if test ! $force = "true"; then
> +		say "Not forced, use"
> +		say "    'git repack -a' to fetch missing objects, then "
> +		say "    '$dashless -f -d $dir' to remove the alternate"
> +		die

Hmm, I am afraid that this will end up training users to always say -f without even thinking. Shouldn't this code be doing whatever necessary steps to make sure this repository has all the necessary objects without the named alternates and then removing the file? An easiest might be to temporarily remove the entry and run fsck, perhaps?