From df6c8652a299ca2a0ca5c7e0ddf6678dd40df46c Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Mon, 25 May 2020 12:13:43 -0700 Subject: [PATCH] filter-repo: fix ugly bug with mixing path filtering and renaming MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- git-filter-repo | 23 +++++++++++++++-------- t/t9390-filter-repo.sh | 29 ++++++++++++++++++++++++++++- 2 files changed, 43 insertions(+), 9 deletions(-) diff --git a/git-filter-repo b/git-filter-repo index 0594140..ce8eee8 100755 --- a/git-filter-repo +++ b/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 diff --git a/t/t9390-filter-repo.sh b/t/t9390-filter-repo.sh index 4925479..88e9412 100755 --- a/t/t9390-filter-repo.sh +++ b/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 &&