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

Thomas Rast schrieb:

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.

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