[PATCH] filter-branch: retire --remap-to-ancestor

[PATCH] filter-branch: retire --remap-to-ancestor

[Csaba Henk]

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.
---
 git-filter-branch.sh     |   14 ++++++++------
 t/t7003-filter-branch.sh |    4 ++--
 2 files changed, 10 insertions(+), 8 deletions(-)

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
-		;;
 	--prune-empty)
 		shift
 		prune_empty=t
@@ -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
+
 rev_args=$(git rev-parse --revs-only "$@")
 
 case "$filter_subdir" in
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) &&
-- 
1.7.2.2

Re: [PATCH] filter-branch: retire --remap-to-ancestor

[Junio C Hamano]

Csaba Henk <csaba@gluster.com> 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.

[PATCH v2] filter-branch: retire --remap-to-ancestor

[Csaba Henk]

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. (Option kept for backward compatibility,
but it's now a no-op.)

Signed-off-by: Csaba Henk <csaba@gluster.com>
---
 Documentation/git-filter-branch.txt |   26 +++++++++++++-------------
 git-filter-branch.sh                |   10 +++++++++-
 t/t7003-filter-branch.sh            |   18 ++++++++++++++++++
 3 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-filter-branch.txt b/Documentation/git-filter-branch.txt
index 020028c..7357c88 100644
--- a/Documentation/git-filter-branch.txt
+++ b/Documentation/git-filter-branch.txt
@@ -159,18 +159,7 @@ to other tags will be rewritten to point to the underlying commit.
 --subdirectory-filter <directory>::
 	Only look at the history which touches the given subdirectory.
 	The result will contain that directory (and only that) as its
-	project root.  Implies --remap-to-ancestor.
-
---remap-to-ancestor::
-	Rewrite refs to the nearest rewritten ancestor instead of
-	ignoring them.
-+
-Normally, positive refs on the command line are only changed if the
-commit they point to was rewritten.  However, you can limit the extent
-of this rewriting by using linkgit:rev-list[1] arguments, e.g., path
-limiters.  Refs pointing to such excluded commits would then normally
-be ignored.  With this option, they are instead rewritten to point at
-the nearest ancestor that was not excluded.
+	project root. Implies <<Remap_to_ancestor>>.
 
 --prune-empty::
 	Some kind of filters will generate empty commits, that left the tree
@@ -204,7 +193,18 @@ the nearest ancestor that was not excluded.
 	Arguments for 'git rev-list'.  All positive refs included by
 	these options are rewritten.  You may also specify options
 	such as '--all', but you must use '--' to separate them from
-	the 'git filter-branch' options.
+	the 'git filter-branch' options. Implies <<Remap_to_ancestor>>.
+
+
+[[Remap_to_ancestor]]
+Remap to ancestor
+~~~~~~~~~~~~~~~~~
+
+By using linkgit:rev-list[1] arguments, e.g., path limiters, you can limit the
+set of revisions which get rewritten. However, positive refs on the command
+line are distinguished: we don't let them be excluded by such limiters. For
+this purpose, they are instead rewritten to point at the nearest ancestor that
+was not excluded.
 
 
 Examples
diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 88fb0f0..962a93b 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -139,6 +139,7 @@ do
 		continue
 		;;
 	--remap-to-ancestor)
+		# deprecated ($remap_to_ancestor is set now automatically)
 		shift
 		remap_to_ancestor=t
 		continue
@@ -265,7 +266,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
+
 rev_args=$(git rev-parse --revs-only "$@")
 
 case "$filter_subdir" in
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 2c55801..12aa63e 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -307,6 +307,24 @@ test_expect_success '--remap-to-ancestor with filename filters' '
 	test $orig_invariant = $(git rev-parse invariant)
 '
 
+test_expect_success 'automatic remapping to ancestor with filename filters' '
+	git checkout master &&
+	git reset --hard A &&
+	test_commit add-foo2 foo 1 &&
+	git branch moved-foo2 &&
+	test_commit add-bar2 bar a &&
+	git branch invariant2 &&
+	orig_invariant=$(git rev-parse invariant2) &&
+	git branch moved-bar2 &&
+	test_commit change-foo2 foo 2 &&
+	git filter-branch -f \
+		moved-foo2 moved-bar2 A..master \
+		-- -- foo &&
+	test $(git rev-parse moved-foo2) = $(git rev-parse moved-bar2) &&
+	test $(git rev-parse moved-foo2) = $(git rev-parse master^) &&
+	test $orig_invariant = $(git rev-parse invariant2)
+'
+
 test_expect_success 'setup submodule' '
 	rm -fr ?* .git &&
 	git init &&
-- 
1.7.2.2