filter-repo: fix ugly bug with mixing path filtering and renaming

There's also a fix in here to make sure to throw an error if users are
trying to rename paths and use --invert-paths; it's not clear at all
what that would even mean.  But that also becomes important later...

Due to the ability to either filter wanted paths (default), or to just
specify unwanted paths (with --invert-paths), I keep a special
args.inclusive variable to track whether a "match" means we want the
path or not.  There are some special cases, notably when there are no
filters present (meaning e.g. no --path specifications, at most there
are some --path-rename values provided).  When there are no filters
present, that means we should keep paths even if we don't "find a match"
against any of the filters.

Now, since the rename code was embedded in the same loop as the filter
checks, it unfortunately was also being checked against the
args.inclusive setting despite never setting whether it found a match.
That happened to work in the special case that there were no filtering
paths but only because of the special logic for that case.  Since
renaming only makes sense if --invert-paths is not specified, any path
we rename is one we always want to keep.  Make sure we do.

Reported-by: Nadège (@nagreme on GitHub)
Signed-off-by: Elijah Newren <newren@gmail.com>

git-filter-repo

@@ -2025,16 +2025,20 @@ EXAMPLES
       args.path_changes = []
       args.inclusive = False
     else:
-      # Similarly, if we have no filtering paths, then no path should be
-      # filtered out.  Based on how newname() works, the easiest way to
-      # achieve that is setting args.inclusive to False.
+      # Check for incompatible --path-rename (or equivalent specification
+      # from --paths-from-file) used together with either
+      # --use-base-name or --invert-paths.
+      if args.use_base_name or not args.inclusive:
+        if any(x[0] == 'rename' for x in args.path_changes):
+          raise SystemExit(_("Error: path renaming is incompatible with "
+                             "both --use-base-name and --invert-paths"))
+
+      # If we only have renaming paths (i.e. we have no filtering paths),
+      # then no path should be filtered out.  Based on how newname() works,
+      # the easiest way to achieve that is setting args.inclusive to False.
       if not any(x[0] == 'filter' for x in args.path_changes):
         args.inclusive = False
-      # Also check for incompatible --use-base-name and --path-rename flags.
-      if args.use_base_name:
-        if any(x[0] == 'rename' for x in args.path_changes):
-          raise SystemExit(_("Error: --use-base-name and --path-rename are "
-                             "incompatible."))
+
     # Also throw some sanity checks on git version here;
     # PERF: remove these checks once new enough git versions are common
     p = subproc.Popen('git fast-export -h'.split(),
@@ -3270,8 +3274,11 @@ class RepoFilter(object):
           assert match_type in ('match','regex') # glob was translated to regex
           if match_type == 'match' and filename_matches(match, full_pathname):
             full_pathname = full_pathname.replace(match, repl, 1)
+            wanted = filtering_is_inclusive
           if match_type == 'regex':
             full_pathname = match.sub(repl, full_pathname)
+            if full_pathname != pathname:
+              wanted = filtering_is_inclusive
       return full_pathname if (wanted == filtering_is_inclusive) else None
 
     args = self._args

t/t9390-filter-repo.sh

@@ -178,6 +178,33 @@ test_expect_success '--paths-from-file' '
 	)
 '
 
+test_expect_success 'Mixing filtering and renaming paths' '
+	test_create_repo path_filtering_and_renaming &&
+	(
+		cd path_filtering_and_renaming &&
+
+		>.gitignore &&
+		mkdir -p src/main/java/com/org/{foo,bar} &&
+		mkdir -p src/main/resources &&
+		test_seq  1 10 >src/main/java/com/org/foo/uptoten &&
+		test_seq 11 20 >src/main/java/com/org/bar/uptotwenty &&
+		test_seq  1  7 >src/main/java/com/org/uptoseven &&
+		test_seq  1  5 >src/main/resources/uptofive &&
+		git add . &&
+		git commit -m Initial &&
+
+		git filter-repo --path .gitignore --path src/main/resources --path-rename src/main/java/com/org/foo/:src/main/java/com/org/ --force &&
+
+		cat <<-EOF >expect &&
+		.gitignore
+		src/main/java/com/org/uptoten
+		src/main/resources/uptofive
+		EOF
+		git ls-files >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_expect_success 'setup metasyntactic repo' '
 	test_create_repo metasyntactic &&
 	(
@@ -1093,7 +1120,7 @@ test_expect_success 'other startup error cases and requests for help' '
 		test_i18ngrep ": --analyze is incompatible with --stdin" err &&
 
 		test_must_fail git filter-repo --path-rename foo:bar --use-base-name 2>err &&
-		test_i18ngrep ": --use-base-name and --path-rename are incompatible" err &&
+		test_i18ngrep ": path renaming is incompatible with both --use-base-name and --invert-paths" err &&
 
 		test_must_fail git filter-repo --path-rename foo:bar/ 2>err &&
 		test_i18ngrep "either ends with a slash then both must." err &&