Re: [PATCH v2 1/2] filter-branch: stop special-casing $filter_subdir argument

From: Johannes Sixt <j.sixt@viscovery.net>
Date: Oct 22, 2009, 06:06 UTC
Message-ID: <4ADFF66F.1080005@viscovery.net>
In-Reply-To: <95535b01e2181d321190c6d93b2834188612a389.1256149428.git.trast@student.ethz.ch>

Thomas Rast schrieb:

Show 9 quoted lines

> Handling $filter_subdir in the usual way requires a separate case at
> every use, because the variable is empty when unused.  Furthermore,
> the case for --subdirectory-filter supplies its own --, so the user
> cannot provide one himself (though there is also very little point in
> doing so).
> 
> Instead, tack the $filter_subdir onto $@ in the right place
> automatically, and only use a -- if it was not already provided by the
> user.

I understand that this is a preparatory patch, but you seem to argue that even without the follow-up patch there is a problem. But from your explanation I do not understand what it is. An example invocation that shows the problem would be very helpful.

Show 7 quoted lines

> @@ -257,15 +257,29 @@ git read-tree || die "Could not seed the index"
>  # map old->new commit ids for rewriting parents
>  mkdir ../map || die "Could not create map/ directory"
>  
> +non_ref_args=$(git rev-parse --no-revs --sq "$@")
> +dashdash=--
> +for arg in "$non_ref_args"; do

At this point $non_ref_args might contain one or more IFS-separatable words, but if you say "$non_ref_args" here, this loop will be entered exactly once. But even if you drop the dquotes, the --sq quoting that you requested from rev-parse bought you nothing.

> +	if test arg = --; then

Did you mean $arg here? Even then this test will succeed only if $non_ref_args contains exactly one word and that word is '--'. Is that what you mean?

Show 38 quoted lines

> +		dashdash=
> +		break
> +	fi
> +done
> +
>  case "$filter_subdir" in
>  "")
> -	git rev-list --reverse --topo-order --default HEAD \
> -		--parents --simplify-merges "$@"
> +	filter_subdir_sq=
>  	;;
>  *)
> -	git rev-list --reverse --topo-order --default HEAD \
> -		--parents --simplify-merges "$@" -- "$filter_subdir"
> -esac > ../revs || die "Could not get the commits"
> +	filter_subdir_sq=$(git rev-parse --sq-quote "$filter_subdir")
> +esac
> +
> +eval "set -- \"\$@\" $dashdash $filter_subdir_sq"
> +non_ref_args=$(git rev-parse --no-revs --sq "$@")
> +
> +git rev-list --reverse --topo-order --default HEAD \
> +	--parents --simplify-merges "$@" \
> +	> ../revs || die "Could not get the commits"
>  commits=$(wc -l <../revs | tr -d " ")
>  
>  test $commits -eq 0 && die "Found nothing to rewrite"
> @@ -356,8 +370,8 @@ then
>  	do
>  		sha1=$(git rev-parse "$ref"^0)
>  		test -f "$workdir"/../map/$sha1 && continue
> -		ancestor=$(git rev-list --simplify-merges -1 \
> -				$ref -- "$filter_subdir")
> +		ancestor=$(eval "git rev-list --simplify-merges " \
> +				"-1 \"$ref\" $non_ref_args")
>  		test "$ancestor" && echo $(map $ancestor) >> "$workdir"/../map/$sha1
>  	done < "$tempdir"/heads
>  fi

This looks so convoluted; there must be a simpler way to achieve your goal.

-- Hannes