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

From: Johannes Sixt <j.sixt@viscovery.net>
Date: Oct 29, 2009, 07:35 UTC
Message-ID: <4AE945D0.5030403@viscovery.net>
In-Reply-To: <6e01558f719f4bfcd12f3c6dc5657790e86c874d.1256770377.git.trast@student.ethz.ch>

Thomas Rast schrieb:

Show 10 quoted lines

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

Show 12 quoted lines

>  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")"

Show 6 quoted lines

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

Show 7 quoted lines

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