Re: [PATCH] filter-branch: retire --remap-to-ancestor
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:49:23
Csaba Henk [off-list ref] 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?
quoted hunk
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).
quoted hunk
@@ -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.
quoted hunk
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.