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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html