Re: [RFC PATCH 3/3] filter-branch: support --submodule-filter

[Johannes Sixt <j6t@kdbg.org>]

On Freitag, 31. Dezember 2010, Thomas Rast wrote:
> This new filter gets each submodule's current sha1 and path on stdin,
> separated by a tab.  It can then emit a new submodule sha1 and/or
> path, and filter-branch will:
>
> * if the path differs, remove the submodule at the old path;
>
> * add/replace the new sha1 at the new path.
>
> Additionally, returning an empty new sha1 deletes the submodule.
>
> You can tie this together with the last two commits to filter the
> super-project after a subproject filtering as follows:
>
>   ( cd sub1 && git filter-branch --dump-map map <filters|args> )
>   ( cd sub2 && git filter-branch --dump-map map <filters|args> )
>   cat sub1/map sub2/map > map
>   git filter-branch --load-map map \
>   	--submodule-filter "map $(cut -f1)" \
> 	<args>
>
> Other use-cases should also be covered since we also pass through the
> path.

As I said, I'm not particularly fond of a new --submodule-filter because it is 
just a special --index-filter.

Your implementation is highly problematic:

> +	if [ "$filter_submodule" ]; then
> +		git ls-files -s |
> +		grep '^160000' |
> +		while read mode sha1 stage path; do
> +			printf "$sha1\t$path\n" |
> +			{ eval "$filter_submodule" ||
> +				die "submodule filter failed: $filter_submodule"; } |

This 'die' will not do anything useful except to write an error message.

> +			read newsha1 newpath

This 'read' is part of a pipe, and as such many shells run it in a sub-shell; 
the values that it reads do not survive the pipe, hence, the subsequent code 
does not do what you intend it.

In this case, you can put everything from 'read' to the last 'fi' below inside 
a block { } because there are no process states that need to survive the 
pipe.

> +			if [ -z "$newsha1" ] || [ "$path" != "$newpath" ]; then
> +				git update-index --remove "$path" ||
> +					die "failed to remove submodule $path"
> +			fi
> +			if [ -n "$newsha1" ] && [ "$sha1" != "$newsha1" ]; then
> +				git update-index --add --replace --cacheinfo \
> +					160000 "$newsha1" "$newpath" ||
> +					die "failed to add submodule $newpath"
> +			fi
> +		done

The whole if-branch is a pipe, and it's parts are run in a sub-shell (although 
shells are allowed to optimize this). This means that the 'die' calls will 
only exit the pipe, but not terminate filter-branch. You at least need to 
have '|| exit' behind the last 'fi' and &&-join the if-statements.

> +	fi
> +
>  	parentstr=
>  	for parent in $parents; do
>  		for reparent in $(map "$parent"); do

-- Hannes