From 3245632722e94c06620076ab2dd789abf1beee3c Mon Sep 17 00:00:00 2001 From: Daniel Finke Date: Sat, 17 Aug 2024 14:43:51 -0700 Subject: [PATCH 1/2] Apply `exclude_files` to `files` absolute paths When calling `erlfmt` with absolute paths for the files to format, the excludes don't apply, even if the resolved paths of the excludes would match the files to format. Resolve the paths of the excludes based on the current working directory and add them to the excludes. This issue was observed when using the [VS Code Erlang Formatter extension](https://marketplace.visualstudio.com/items?itemName=szTheory.erlang-formatter). The extension invokes `rebar3 fmt` with the full path of the file to format. --- src/erlfmt_cli.erl | 19 +++++++++++++++++- test/erlfmt_cli_SUITE.erl | 41 +++++++++++++++++++++++++++++---------- 2 files changed, 49 insertions(+), 11 deletions(-) diff --git a/src/erlfmt_cli.erl b/src/erlfmt_cli.erl index 8ba30f9..e602be1 100644 --- a/src/erlfmt_cli.erl +++ b/src/erlfmt_cli.erl @@ -107,8 +107,25 @@ with_parsed(Name, Config) -> erlang:halt(127) end. +-spec set_difference([file:name_all()], [file:name_all()]) -> [file:name_all()]. set_difference(Files, Excludes) -> - sets:to_list(sets:subtract(sets:from_list(Files), sets:from_list(Excludes))). + {ok, Dir} = file:get_cwd(), + AbsoluteExcludes = [resolve_path(Dir, E) || E <- Excludes], + AllExcludes = sets:from_list(Excludes ++ AbsoluteExcludes), + sets:to_list(sets:subtract(sets:from_list(Files), AllExcludes)). + +resolve_path(Dir, Filename) -> + case filename:pathtype(Filename) of + absolute -> Filename; + relative -> resolve_path2(lists:reverse(filename:split(Dir)), filename:split(Filename)) + end. + +resolve_path2([_H | T1], [".." | T2]) -> + resolve_path2(T1, T2); +resolve_path2([_H | T1], [<<"..">> | T2]) -> + resolve_path2(T1, T2); +resolve_path2(Dir, Filename) -> + filename:join(lists:reverse(Dir) ++ Filename). %% needed because of getopt -dialyzer({nowarn_function, [unprotected_with_config/2]}). diff --git a/test/erlfmt_cli_SUITE.erl b/test/erlfmt_cli_SUITE.erl index 8afcdc1..6866f18 100644 --- a/test/erlfmt_cli_SUITE.erl +++ b/test/erlfmt_cli_SUITE.erl @@ -51,6 +51,7 @@ noformat_pragma/1, noformat_pragma_file/1, exclude_check/1, + exclude_absolute_check/1, range_check_full/1, range_check_partial/1 ]). @@ -98,6 +99,7 @@ groups() -> noformat_pragma, noformat_pragma_file, exclude_check, + exclude_absolute_check, range_check_full, range_check_partial ]} @@ -250,16 +252,14 @@ noformat_pragma(Config) when is_list(Config) -> exclude_check(Config) when is_list(Config) -> Files = filename:join(?config(data_dir, Config), "*.erl"), Exclude = filename:join(?config(data_dir, Config), "broken.erl"), - WithBroken = os:cmd( - escript() ++ " -c " ++ Files - ), - ?assertNotMatch(nomatch, string:find(WithBroken, "[warn]")), - ?assertNotMatch(nomatch, string:find(WithBroken, "broken.erl")), - WithoutBroken = os:cmd( - escript() ++ " -c " ++ Files ++ " --exclude-files=" ++ Exclude - ), - ?assertNotMatch(nomatch, string:find(WithoutBroken, "[warn]")), - ?assertMatch(nomatch, string:find(WithoutBroken, "broken.erl")). + exclude_test(Files, Exclude). + +exclude_absolute_check(Config) when is_list(Config) -> + {ok, ProjectRoot} = file:get_cwd(), + DataDirRelPath = make_relative_path(?config(data_dir, Config), ProjectRoot), + Files = filename:join(?config(data_dir, Config), "*.erl"), + Exclude = filename:join(DataDirRelPath, "broken.erl"), + exclude_test(Files, Exclude). range_check_full(Config) when is_list(Config) -> %% Mainly check the options is properly recognized. @@ -284,6 +284,27 @@ stdio_test(FileName, Options, Config) -> {ok, Expected} = file:read_file(Path), assert_diagnostic:assert_binary_match(Expected, unicode:characters_to_binary(Formatted)). +exclude_test(Files, Exclude) -> + WithBroken = os:cmd( + escript() ++ " -c " ++ Files + ), + ?assertNotMatch(nomatch, string:find(WithBroken, "[warn]")), + ?assertNotMatch(nomatch, string:find(WithBroken, "broken.erl")), + WithoutBroken = os:cmd( + escript() ++ " -c " ++ Files ++ " --exclude-files=" ++ Exclude + ), + ?assertNotMatch(nomatch, string:find(WithoutBroken, "[warn]")), + ?assertMatch(nomatch, string:find(WithoutBroken, "broken.erl")). + escript() -> %% this relies on the _build structure rebar3 uses filename:join(code:lib_dir(erlfmt), "../../bin/erlfmt"). + +make_relative_path(Source, Target) -> + make_relative_path2(filename:split(Source), filename:split(Target)). + +make_relative_path2([H | T1], [H | T2]) -> + make_relative_path2(T1, T2); +make_relative_path2(Source, Target) -> + Base = lists:duplicate(length(Target), ".."), + filename:join(Base ++ Source). From 915d7e22605646223f12c6cfa97b9b1d76867cac Mon Sep 17 00:00:00 2001 From: Daniel Finke Date: Wed, 28 Aug 2024 12:03:21 -0700 Subject: [PATCH 2/2] Use `filename:absname/2` to resolve absolute paths This handles Windows' `volumerelative` path types by properly resolving them based on the "per-drive" working directory. Also, resolve the input file absolute paths. This is new functionality; if you were previously to use a file path like `a/../b`, that would have been directly matched against the excludes. As a result, an exclude of `b` would not have applied. This change makes sure relative path resolution is consistent between `files`/`exclude_files`. To avoid displaying different paths from what was requesting by config or CLI options (e.g. when using the verbose flag), use a map of absolute paths -> original paths and return the original paths from `erlfmt_cli:set_difference/2` after applying excludes. --- src/erlfmt_cli.erl | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/src/erlfmt_cli.erl b/src/erlfmt_cli.erl index e602be1..bea1e6c 100644 --- a/src/erlfmt_cli.erl +++ b/src/erlfmt_cli.erl @@ -109,23 +109,24 @@ with_parsed(Name, Config) -> -spec set_difference([file:name_all()], [file:name_all()]) -> [file:name_all()]. set_difference(Files, Excludes) -> - {ok, Dir} = file:get_cwd(), - AbsoluteExcludes = [resolve_path(Dir, E) || E <- Excludes], - AllExcludes = sets:from_list(Excludes ++ AbsoluteExcludes), - sets:to_list(sets:subtract(sets:from_list(Files), AllExcludes)). + {ok, Cwd} = file:get_cwd(), + AbsoluteFiles = maps:from_list([{resolve_path(Cwd, F), F} || F <- Files]), + AbsoluteExcludes = [resolve_path(Cwd, E) || E <- Excludes], + maps:values(maps:without(AbsoluteExcludes, AbsoluteFiles)). resolve_path(Dir, Filename) -> - case filename:pathtype(Filename) of - absolute -> Filename; - relative -> resolve_path2(lists:reverse(filename:split(Dir)), filename:split(Filename)) - end. - -resolve_path2([_H | T1], [".." | T2]) -> - resolve_path2(T1, T2); -resolve_path2([_H | T1], [<<"..">> | T2]) -> - resolve_path2(T1, T2); -resolve_path2(Dir, Filename) -> - filename:join(lists:reverse(Dir) ++ Filename). + resolve_path2(filename:absname(Filename, Dir)). + +resolve_path2(AbsPath) -> + [Volume | Components] = filename:split(AbsPath), + filename:join(resolve_path2(Components, [Volume])). + +resolve_path2([".." | T1], [Volume]) -> resolve_path2(T1, [Volume]); +resolve_path2([<<"..">> | T1], [Volume]) -> resolve_path2(T1, [Volume]); +resolve_path2([".." | T1], [_H2 | T2]) -> resolve_path2(T1, T2); +resolve_path2([<<"..">> | T1], [_H2 | T2]) -> resolve_path2(T1, T2); +resolve_path2([H1 | T1], Components) -> resolve_path2(T1, [H1 | Components]); +resolve_path2([], Components) -> lists:reverse(Components). %% needed because of getopt -dialyzer({nowarn_function, [unprotected_with_config/2]}).