Csaba Henk <csaba@xxxxxxxxxxx> writes:
> We can be clever and know by ourselves when we need the behavior
> implied by "--remap-to-ancestor". No need to encumber users by having
> them exposed to it as a tunable.
> ---
Sign-off?
> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> index 88fb0f0..fd5caaa 100755
> --- a/git-filter-branch.sh
> +++ b/git-filter-branch.sh
> @@ -138,11 +138,6 @@ do
> force=t
> continue
> ;;
> - --remap-to-ancestor)
> - shift
> - remap_to_ancestor=t
> - continue
> - ;;
This is not friendly to people who already wrote their own scripts and/or
trained their fingers to use this option. It would be better to just
accept the option silently and do nothing. Even better, do not touch this
part at all, so that people can ask for this option when there is no
pathspec (as far as I can see, there is no harm in doing so).
> @@ -265,7 +260,14 @@ mkdir ../map || die "Could not create map/ directory"
>
> # we need "--" only if there are no path arguments in $@
> nonrevs=$(git rev-parse --no-revs "$@") || exit
> -test -z "$nonrevs" && dashdash=-- || dashdash=
> +if test -z "$nonrevs"
> +then
> + dashdash=--
> +else
> + dashdash=
> + remap_to_ancestor=t
> +fi
The above is fine.
If you were paranoid, you could make this default assignment conditional,
i.e.
: ${remap_to_ancestor:=t}
which would then allow people to say --no-remap-to-ancestor when using
pathspecs (for whatever reason), if you add:
--no-remap-to-ancestor)
shift
remap_to_ancestor=f
continue
;;
in the command line parser loop. You would need to change the remapping
logic to trigger when $remap_to_ancestor is set to 't' if you did so.
But I don't think the paranoia is necessary here.
> diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
> index 2c55801..486c453 100755
> --- a/t/t7003-filter-branch.sh
> +++ b/t/t7003-filter-branch.sh
> @@ -289,7 +289,7 @@ test_expect_success 'Prune empty commits' '
> test_cmp expect actual
> '
>
> -test_expect_success '--remap-to-ancestor with filename filters' '
> +test_expect_success 'filename filters work even if the given files are not changed in branch head' '
> git checkout master &&
> git reset --hard A &&
> test_commit add-foo foo 1 &&
> @@ -299,7 +299,7 @@ test_expect_success '--remap-to-ancestor with filename filters' '
> orig_invariant=$(git rev-parse invariant) &&
> git branch moved-bar &&
> test_commit change-foo foo 2 &&
> - git filter-branch -f --remap-to-ancestor \
> + git filter-branch -f \
> moved-foo moved-bar A..master \
> -- -- foo &&
> test $(git rev-parse moved-foo) = $(git rev-parse moved-bar) &&
This is good, but I think it is better if the same is tested with and
without the option (iow, add a test without --remap that expects the same
result, keeping the existing one with --remap intact).
Thanks.
--
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