Fix paths when running clang-tidy on git diffs #161496

flar · 2025-01-13T00:34:25Z

The working directory was engine/src/flutter but all of the file names already had those parent directories on them so we'd be trying to find build commands for engine/src/flutter/engine/src/flutter. This change tells the git commands to make the file names relative to the working directory (which is already engine/src/flutter).

Someone more familiar with the --lint-head option should double check its operation since I wasn't sure exactly what it was supposed to do, but the list of files it generated looked correct.

flutter-dashboard · 2025-01-13T00:34:28Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

flar · 2025-01-13T04:05:44Z

It looks like there are tests (written for the tool itself?) that expect repo-relative results when the utility is used from within a sub-directory of the repo. So, this should likely be made an option.

flar · 2025-01-13T04:23:28Z

It looks like an internal test for the git_repo_tools is using a fake process manager to double-check the git commands being executed and the addition of --relative breaks its expectations. I should probably modify the utility to make the relative option be optional and modify the test to check both conditions, but that is proving not very straightforward. Another option is to teach clang-tidy to not duplicate the sub-directory paths.

jtmcdole · 2025-01-13T16:35:48Z

I just want to make sure this is the expected output:

If I edit two files (framework and engine) in the same tree and don't commit them:

from the root of the tree I get:

git diff --name-only --diff-filter=ACMRT upstream/master --relative:

engine/src/flutter/impeller/core/buffer_view.cc
packages/flutter/lib/cupertino.dart

git diff --name-only --diff-filter=ACMRT upstream/master:

engine/src/flutter/impeller/core/buffer_view.cc
packages/flutter/lib/cupertino.dart

from the engine folder:

git diff --name-only --diff-filter=ACMRT upstream/master --relative

impeller/core/buffer_view.cc

git diff --name-only --diff-filter=ACMRT upstream/master

engine/src/flutter/impeller/core/buffer_view.cc
packages/flutter/lib/cupertino.dart

If I commit them:

 ~/src/flutter/engine/src/flutter ❯ git diff-tree --no-commit-id --name-only --diff-filter=ACMRT -r HEAD
engine/src/flutter/impeller/core/buffer_view.cc
packages/flutter/lib/cupertino.dart

 ~/src/flutter/engine/src/flutter ❯ git diff-tree --no-commit-id --name-only --diff-filter=ACMRT --relative  -r HEAD
impeller/core/buffer_view.cc

~/src/flutter ❯ git diff-tree --no-commit-id --name-only --diff-filter=ACMRT -r HEAD
engine/src/flutter/impeller/core/buffer_view.cc
packages/flutter/lib/cupertino.dart

~/src/flutter ❯ git diff-tree --no-commit-id --name-only --diff-filter=ACMRT --relative  -r HEAD
engine/src/flutter/impeller/core/buffer_view.cc
packages/flutter/lib/cupertino.dart

flar · 2025-01-13T18:41:06Z

I think it is setting the working directory to the root of the engine tree which means it is only seeing files in the engine subtree in its git commands. But, even though the focus is on just the local files, they are listed as relative to the git root.

Adding --relative makes them relative to the current dir, which means the next step of prepending them with the "root" field will compose the correct absolute paths. There are 2 ways to fix this:

Use --relative as I did here, but then fix the tests so that they expect that option (and possibly make it optional and possibly then add extra tests that test it both ways)
Just let the paths be relative to the git root, but have the clang_tidy class know that and tell the git command to be relative to the engine root, but do its post-fixing with the path of just the git root.

flar · 2025-01-13T19:45:47Z

Since the prepending of the git root happens in the same file as the git commands I decided that always using the --relative option was the way to go and fixed the tests to expect it as an option on the command lines being run.

…er#161496)

Roll Flutter from 72db8f6 to 40c2b86 (33 revisions) flutter/flutter@72db8f6...40c2b86 2025-01-14 [email protected] update changelog for 3.27.2 release (flutter/flutter#161569) 2025-01-14 [email protected] Fix `showLicensePage` does not inherit ambient `Theme` (flutter/flutter#161599) 2025-01-14 [email protected] Added special case for fat width arcs (flutter/flutter#161255) 2025-01-14 [email protected] Replace `fetch `with `gclient sync`. (flutter/flutter#161565) 2025-01-14 [email protected] Roll Packages from 3c3bc68 to d1fd623 (4 revisions) (flutter/flutter#161597) 2025-01-14 [email protected] Remove unused method (flutter/flutter#161572) 2025-01-14 [email protected] Fix crash when closing a window with `Alt+F4` in multi-win Flutter on Windows (flutter/flutter#161375) 2025-01-14 [email protected] Update InputDecoration.border documentation (flutter/flutter#161415) 2025-01-14 [email protected] [Web] Allow specifying the strategy on when to use <img> element to display images (flutter/flutter#159917) 2025-01-14 [email protected] Roll Dart to Version 3.7.0-323.0.dev (flutter/flutter#161567) 2025-01-14 [email protected] Use wildcards (flutter/flutter#161548) 2025-01-14 [email protected] Autocomplete Options Width (flutter/flutter#143249) 2025-01-13 [email protected] Move the analyzer_benchmark to Mac arm64 devicelab bots (flutter/flutter#161405) 2025-01-13 [email protected] Remove references to `cirrus`, mostly in doc comments. (flutter/flutter#161529) 2025-01-13 [email protected] Fix paths when running clang-tidy on git diffs (flutter/flutter#161496) 2025-01-13 [email protected] [web:a11y] treat empty tappables as buttons (flutter/flutter#161360) 2025-01-13 [email protected] Add route settings to CupertinoSheetRoute (flutter/flutter#161528) 2025-01-13 [email protected] Copy `linux_host_engine` as `linux_host_engine_test`, removing `archives: [...]`. (flutter/flutter#161532) 2025-01-13 [email protected] Remove last two references to Cirrus CI. (flutter/flutter#161530) 2025-01-13 [email protected] Mark `Mac_mokey microbenchmarks` as flakey (flutter/flutter#161550) 2025-01-13 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Match CupertinoPageTransitionsBuilder animation duration to CupertinoPageRoute (#160241)" (flutter/flutter#161555) 2025-01-13 [email protected] Add validator execution times to `flutter doctor --verbose` (flutter/flutter#158124) 2025-01-13 [email protected] Explain more specifically how to use `flutter drive`/what it does (flutter/flutter#161450) 2025-01-13 [email protected] Fixed repeated strings for incompatible Gradle or AGP version in `create` command (flutter/flutter#161223) 2025-01-13 [email protected] Remove `WEB_SHARD_COUNT`, which no longer exists post-Cirrus. (flutter/flutter#161527) 2025-01-13 [email protected] [Impeller] Update guidance on prebuilt artifacts. (flutter/flutter#161251) 2025-01-13 [email protected] Migrate DisplayList unit tests to DL/Impeller geometry classes (flutter/flutter#161453) 2025-01-13 [email protected] Context menu button callback docs clarification (flutter/flutter#161451) 2025-01-13 [email protected] Match CupertinoPageTransitionsBuilder animation duration to CupertinoPageRoute (flutter/flutter#160241) 2025-01-13 [email protected] Udpate documentation on the third_party directories (flutter/flutter#161407) 2025-01-13 [email protected] Propagate environment variables when `flutter drive` is invoked. (flutter/flutter#161452) 2025-01-13 [email protected] Convert base application name handling to kotlin source (start of FGP kt conversion) (flutter/flutter#155963) 2025-01-13 [email protected] [Impeller] remove API 30 restriction for SurfaceControl testing. (flutter/flutter#161438) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: ...

The working directory was `engine/src/flutter` but all of the file names already had those parent directories on them so we'd be trying to find build commands for `engine/src/flutter/engine/src/flutter`. This change tells the git commands to make the file names relative to the working directory (which is already `engine/src/flutter`). Someone more familiar with the `--lint-head` option should double check its operation since I wasn't sure exactly what it was supposed to do, but the list of files it generated looked correct.

…er#161496)

Roll Flutter from 72db8f6 to 40c2b86 (33 revisions) flutter/flutter@72db8f6...40c2b86 2025-01-14 [email protected] update changelog for 3.27.2 release (flutter/flutter#161569) 2025-01-14 [email protected] Fix `showLicensePage` does not inherit ambient `Theme` (flutter/flutter#161599) 2025-01-14 [email protected] Added special case for fat width arcs (flutter/flutter#161255) 2025-01-14 [email protected] Replace `fetch `with `gclient sync`. (flutter/flutter#161565) 2025-01-14 [email protected] Roll Packages from 3c3bc68 to d1fd623 (4 revisions) (flutter/flutter#161597) 2025-01-14 [email protected] Remove unused method (flutter/flutter#161572) 2025-01-14 [email protected] Fix crash when closing a window with `Alt+F4` in multi-win Flutter on Windows (flutter/flutter#161375) 2025-01-14 [email protected] Update InputDecoration.border documentation (flutter/flutter#161415) 2025-01-14 [email protected] [Web] Allow specifying the strategy on when to use <img> element to display images (flutter/flutter#159917) 2025-01-14 [email protected] Roll Dart to Version 3.7.0-323.0.dev (flutter/flutter#161567) 2025-01-14 [email protected] Use wildcards (flutter/flutter#161548) 2025-01-14 [email protected] Autocomplete Options Width (flutter/flutter#143249) 2025-01-13 [email protected] Move the analyzer_benchmark to Mac arm64 devicelab bots (flutter/flutter#161405) 2025-01-13 [email protected] Remove references to `cirrus`, mostly in doc comments. (flutter/flutter#161529) 2025-01-13 [email protected] Fix paths when running clang-tidy on git diffs (flutter/flutter#161496) 2025-01-13 [email protected] [web:a11y] treat empty tappables as buttons (flutter/flutter#161360) 2025-01-13 [email protected] Add route settings to CupertinoSheetRoute (flutter/flutter#161528) 2025-01-13 [email protected] Copy `linux_host_engine` as `linux_host_engine_test`, removing `archives: [...]`. (flutter/flutter#161532) 2025-01-13 [email protected] Remove last two references to Cirrus CI. (flutter/flutter#161530) 2025-01-13 [email protected] Mark `Mac_mokey microbenchmarks` as flakey (flutter/flutter#161550) 2025-01-13 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Match CupertinoPageTransitionsBuilder animation duration to CupertinoPageRoute (#160241)" (flutter/flutter#161555) 2025-01-13 [email protected] Add validator execution times to `flutter doctor --verbose` (flutter/flutter#158124) 2025-01-13 [email protected] Explain more specifically how to use `flutter drive`/what it does (flutter/flutter#161450) 2025-01-13 [email protected] Fixed repeated strings for incompatible Gradle or AGP version in `create` command (flutter/flutter#161223) 2025-01-13 [email protected] Remove `WEB_SHARD_COUNT`, which no longer exists post-Cirrus. (flutter/flutter#161527) 2025-01-13 [email protected] [Impeller] Update guidance on prebuilt artifacts. (flutter/flutter#161251) 2025-01-13 [email protected] Migrate DisplayList unit tests to DL/Impeller geometry classes (flutter/flutter#161453) 2025-01-13 [email protected] Context menu button callback docs clarification (flutter/flutter#161451) 2025-01-13 [email protected] Match CupertinoPageTransitionsBuilder animation duration to CupertinoPageRoute (flutter/flutter#160241) 2025-01-13 [email protected] Udpate documentation on the third_party directories (flutter/flutter#161407) 2025-01-13 [email protected] Propagate environment variables when `flutter drive` is invoked. (flutter/flutter#161452) 2025-01-13 [email protected] Convert base application name handling to kotlin source (start of FGP kt conversion) (flutter/flutter#155963) 2025-01-13 [email protected] [Impeller] remove API 30 restriction for SurfaceControl testing. (flutter/flutter#161438) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: ...

Fix paths when running clang-tidy on git diffs

09c4db7

flar requested review from jtmcdole and matanlurey January 13, 2025 00:34

github-actions bot added the engine flutter/engine related. See also e: labels. label Jan 13, 2025

flar added 2 commits January 13, 2025 11:41

add comments and fix tests

dc99c38

formatting

b559aa0

jtmcdole approved these changes Jan 13, 2025

View reviewed changes

flar added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 13, 2025

auto-submit bot added this pull request to the merge queue Jan 13, 2025

Merged via the queue into flutter:master with commit 366ed7f Jan 14, 2025
178 checks passed

flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jan 14, 2025

engine-flutter-autoroll mentioned this pull request Jan 14, 2025

Roll Flutter from 72db8f69e339 to f2c15f9debb2 (27 revisions) flutter/packages#8424

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 14, 2025

366ed7f Fix paths when running clang-tidy on git diffs (flutter/flutt…

7f96f35

…er#161496)

engine-flutter-autoroll mentioned this pull request Jan 14, 2025

Roll Flutter from 72db8f69e339 to c9923ca609cc (28 revisions) flutter/packages#8425

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 14, 2025

366ed7f Fix paths when running clang-tidy on git diffs (flutter/flutt…

cf58a5a

…er#161496)

engine-flutter-autoroll mentioned this pull request Jan 14, 2025

Roll Flutter from 72db8f69e339 to ed9f9206629b (29 revisions) flutter/packages#8426

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 14, 2025

366ed7f Fix paths when running clang-tidy on git diffs (flutter/flutt…

0f58084

…er#161496)

engine-flutter-autoroll mentioned this pull request Jan 14, 2025

Roll Flutter from 72db8f69e339 to 40c2b86fdf65 (33 revisions) flutter/packages#8428

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 14, 2025

366ed7f Fix paths when running clang-tidy on git diffs (flutter/flutt…

fe6a7fe

…er#161496)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 12, 2025

366ed7f Fix paths when running clang-tidy on git diffs (flutter/flutt…

267e867

…er#161496)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025

366ed7f Fix paths when running clang-tidy on git diffs (flutter/flutt…

582d5b8

…er#161496)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025

366ed7f Fix paths when running clang-tidy on git diffs (flutter/flutt…

1c97af8

…er#161496)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 6, 2025

366ed7f Fix paths when running clang-tidy on git diffs (flutter/flutt…

0a2b087

…er#161496)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2025

366ed7f Fix paths when running clang-tidy on git diffs (flutter/flutt…

57728e7

…er#161496)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix paths when running clang-tidy on git diffs #161496

Fix paths when running clang-tidy on git diffs #161496

Uh oh!

flar commented Jan 13, 2025

Uh oh!

flutter-dashboard bot commented Jan 13, 2025

Uh oh!

flar commented Jan 13, 2025

Uh oh!

flar commented Jan 13, 2025

Uh oh!

jtmcdole commented Jan 13, 2025

Uh oh!

flar commented Jan 13, 2025

Uh oh!

flar commented Jan 13, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Fix paths when running clang-tidy on git diffs #161496

Fix paths when running clang-tidy on git diffs #161496

Uh oh!

Conversation

flar commented Jan 13, 2025

Uh oh!

flutter-dashboard bot commented Jan 13, 2025

Uh oh!

flar commented Jan 13, 2025

Uh oh!

flar commented Jan 13, 2025

Uh oh!

jtmcdole commented Jan 13, 2025

If I edit two files (framework and engine) in the same tree and don't commit them:

If I commit them:

Uh oh!

flar commented Jan 13, 2025

Uh oh!

flar commented Jan 13, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants