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

[Junio C Hamano <gitster@pobox.com>]

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

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

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

> +{
> +	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"

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

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

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