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

From: Johannes Sixt <j6t@kdbg.org>
Date: Dec 31, 2010, 17:31 UTC
Message-ID: <201012311831.24057.j6t@kdbg.org>
In-Reply-To: <44e6104ba28c80a6befe0f39fa4e2d6eeec56aa9.1293809100.git.trast@student.ethz.ch>

On Freitag, 31. Dezember 2010, Thomas Rast wrote:

Show 22 quoted lines

> 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:

Show 7 quoted lines

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

Show 10 quoted lines

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

Show 5 quoted lines

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

-- Hannes