From 31bfd519956b011b822769f227fd7dcf679f2f43 Mon Sep 17 00:00:00 2001
From: Bruce Dawson <brucedawson@chromium.org>
Date: Tue, 10 May 2022 23:19:39 +0000
Subject: [PATCH] Actually add a trailing slash to dir_with_slash

AffectedFiles carefully added a slash to dir_with_slash, but then called
normpath - which trims trailing path separators. This meant that
ui/views/PRESUBMIT.py would analyze files in ui\views_content_client,
leading to spooky action at a distance. This was noticed when this
command triggered a presubmit error:

    git cl presubmit "--files=ui/views_content_client/*.cc;ui/views/readme.md"

when running presubmit on either file individually found nothing.

This change was tested by getting CheckChangeLintsClean to print the
files that it was asked to analyze, making the behavior quite obvious.

This bug appears to have existed for about thirteen years, but only
triggers in rare cases, and even then the incorrect behavior was almost
impossible to notice.

One of the tests was inadvertently testing the broken AffectedFiles
behavior and another seemed to be requiring it to handle '.'
correctly which I'm not sure we want to support.

Bug: 1309977
Change-Id: Ibdc39981d69664b03448acb228d4ab05b49436f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3632034
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
---
 presubmit_support.py        |  8 +++++---
 tests/presubmit_unittest.py | 28 +++++++++++++++++-----------
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/presubmit_support.py b/presubmit_support.py
index fb3cb51ed0..dbfcc80af8 100755
--- a/presubmit_support.py
+++ b/presubmit_support.py
@@ -710,9 +710,11 @@ class InputApi(object):
     script, or subdirectories thereof. Note that files are listed using the OS
     path separator, so backslashes are used as separators on Windows.
     """
-    dir_with_slash = normpath('%s/' % self.PresubmitLocalPath())
-    if len(dir_with_slash) == 1:
-      dir_with_slash = ''
+    dir_with_slash = normpath(self.PresubmitLocalPath())
+    # normpath strips trailing path separators, so the trailing separator has to
+    # be added after the normpath call.
+    if len(dir_with_slash) > 0:
+      dir_with_slash += os.path.sep
 
     return list(filter(
         lambda x: normpath(x.AbsoluteLocalPath()).startswith(dir_with_slash),
diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py
index 0fd02fb7ca..088f592d69 100755
--- a/tests/presubmit_unittest.py
+++ b/tests/presubmit_unittest.py
@@ -1449,10 +1449,12 @@ class InputApiUnittest(PresubmitTestsBase):
     change = presubmit.GitChange(
         'mychange', '', self.fake_root_dir, files, 0, 0, None)
     input_api = presubmit.InputApi(
-        change, './PRESUBMIT.py', False, None, False)
+        change, os.path.join(self.fake_root_dir, 'PRESUBMIT.py'), False, None,
+        False)
     # Sample usage of overriding the default white and black lists.
     got_files = input_api.AffectedSourceFiles(
         lambda x: input_api.FilterSourceFile(x, files_to_check, files_to_skip))
+
     self.assertEqual(len(got_files), 2)
     self.assertEqual(got_files[0].LocalPath(), 'eeaee')
     self.assertEqual(got_files[1].LocalPath(), 'eecaee')
@@ -1473,6 +1475,8 @@ class InputApiUnittest(PresubmitTestsBase):
     change = presubmit.Change(
         'mychange', '', self.fake_root_dir, files, 0, 0, None)
     affected_files = change.AffectedFiles()
+    # Validate that normpath strips trailing path separators.
+    self.assertEqual('isdir', normpath('isdir/'))
     # Local paths should remain the same
     self.assertEqual(affected_files[0].LocalPath(), normpath('isdir'))
     self.assertEqual(affected_files[1].LocalPath(), normpath('isdir/blat.cc'))
@@ -1494,16 +1498,18 @@ class InputApiUnittest(PresubmitTestsBase):
         change=change, presubmit_path=presubmit_path,
         is_committing=True, gerrit_obj=None, verbose=False)
     paths_from_api = api.AbsoluteLocalPaths()
-    self.assertEqual(len(paths_from_api), 2)
-    for absolute_paths in [paths_from_change, paths_from_api]:
-      self.assertEqual(
-          absolute_paths[0],
-          presubmit.normpath(os.path.join(
-              self.fake_root_dir, 'isdir')))
-      self.assertEqual(
-          absolute_paths[1],
-          presubmit.normpath(os.path.join(
-              self.fake_root_dir, 'isdir', 'blat.cc')))
+    self.assertEqual(len(paths_from_api), 1)
+    self.assertEqual(
+        paths_from_change[0],
+        presubmit.normpath(os.path.join(self.fake_root_dir, 'isdir')))
+    self.assertEqual(
+        paths_from_change[1],
+        presubmit.normpath(os.path.join(self.fake_root_dir, 'isdir',
+                                        'blat.cc')))
+    self.assertEqual(
+        paths_from_api[0],
+        presubmit.normpath(os.path.join(self.fake_root_dir, 'isdir',
+                                        'blat.cc')))
 
   def testDeprecated(self):
     change = presubmit.Change(