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

[Johannes Sixt <j.sixt@viscovery.net>]

Thomas Rast schrieb:
> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> index a480d6f..da23b99 100755
> --- a/git-filter-branch.sh
> +++ b/git-filter-branch.sh
> @@ -257,15 +257,23 @@ 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"
>  
> +dashdash=
> +test -z "$(git rev-parse --no-revs "$@")" && dashdash=--

Hmm, if the user runs

   $ git filter-branch does-not-exist

then this would be the first call to rev-parse that fails with "fatal:
ambiguous argument 'does-not-exist': unknown revision or path...", but
this code wouldn't catch the failure. I suggest this instead:

# we need "--" only if there are no path arguments in $@
nonrevs=$(git rev-parse --no-revs "$@") || exit
dashdash=${nonrevs+"--"}

(including the comment; you can drop the dquotes in the dashdash
assignment if you think the result is still readable.)

I was to suggest to move these lines into the case arm below that is the
only user of $dashdash. But since this is a good error check of the
user-supplied arguments, I now prefer to keep the lines outside the case
statement.

> +ref_args=$(git rev-parse --revs-only "$@")

"ref_args"? Shouldn't it be "rev_args"? It's about "revisions", not
"references".

>  case "$filter_subdir" in
>  "")
> -	git rev-list --reverse --topo-order --default HEAD \
> -		--parents --simplify-merges "$@"
> +	eval set -- "$(git rev-parse --sq --no-revs "$@")"
>  	;;
>  *)
> -	git rev-list --reverse --topo-order --default HEAD \
> -		--parents --simplify-merges "$@" -- "$filter_subdir"
> -esac > ../revs || die "Could not get the commits"
> +	eval set -- "$(git rev-parse --sq --no-revs "$@")" \
> +	    $dashdash "$filter_subdir"

This is not correct: $filter_subdir undergoes an extra level of
evaluation. You must write it like this:

	eval set -- "$(git rev-parse --sq --no-revs "$@" \
		$dashdash "$filter_subdir")"

> +	;;
> +esac
> +
> +git rev-list --reverse --topo-order --default HEAD \
> +	--parents --simplify-merges $ref_args "$@" \
> +	> ../revs || die "Could not get the commits"

Personally, I would prefer to write this as

git rev-list --reverse --topo-order --default HEAD \
	--parents --simplify-merges $ref_args "$@" > ../revs ||
	die "Could not get the commits"

to avoid one backslash.

> @@ -356,8 +364,7 @@ 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=$(git rev-list --simplify-merges -1 "$ref" "$@")

You added dquotes around $ref: while not absolutely necessary, I agree
with this change.

>  		test "$ancestor" && echo $(map $ancestor) >> "$workdir"/../map/$sha1
>  	done < "$tempdir"/heads
>  fi

-- Hannes