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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 4, 2011, 19:18 UTC
Message-ID: <7vfwt8fpfb.fsf@alter.siamese.dyndns.org>
In-Reply-To: <201101041414.19587.trast@student.ethz.ch>

Thomas Rast <trast@student.ethz.ch> writes:

Show 6 quoted lines

> Subject: TOY PATCH: filter-branch --split-submodule
>
> Sometimes it makes sense to split out a path not as a subdirectory
> (that would be merged by subtree-merge), but as a submodule.  Since
> git objects are just shaped in the right way, this is actually quite
> easy to do in a way that maintains the correct history relations:

The patch from a cursory look feels sane.

Show 8 quoted lines

> @@ -349,6 +352,43 @@ while read commit parents; do
>  	eval "$filter_index" < /dev/null ||
>  		die "index filter failed: $filter_index"
>  
> +	if test -n "$split_submodule"; then
> +		sub_differs=
> +		sub_parents=
> +		sub_commit=

Just a style, but I find

	if test -n "$split_submodule"
        then
        	sub_differs= sub_parents= sub_commit=

easier to read. Not a biggie, as the neighbourhood in the script already is infested in the other style, but I thought I'd mention it.

> +		submodule="$(git rev-parse --verify $commit:$split_submodule 2>/dev/null)"

Do we need double quotes around it?

Show 5 quoted lines

> +		if test -z "$parents"; then
> +			if test -n "$submodule"; then
> +				sub_differs=t
> +			fi
> +		fi

	if test -z "$parents" && test -n "$submodule"
        then
        	sub_differs=t
	fi

> +		for parent in $parents; do
> +			if ! test "$(git rev-parse --verify $parent:$split_submodule 2>/dev/null)" = "$submodule"; then
> +				sub_differs=t
> +			fi

If even one of the parents is different, we say "differs"...

Show 7 quoted lines

> +		if test -n "$sub_differs"; then
> +			sub_commit="$(sed -e '1,/^$/d' <../commit |
> +				      git commit-tree $submodule $sub_parents)" || exit
> +		else
> +			for parent in $parents; do
> +				sub_commit="$(git rev-parse --verify "$(map "$parent")":$split_submodule 2>/dev/null)"
> +				break

... so we can just pick from the first parent and know all of them are the same (could be empty which also is fine). Good.