Fix: .gitignored dirs should be excluded from workspace, pedanticPack…

…ages should handle gitignore patterns with leading slash (#1222) * /.spago in .gitignore caused pedanticPackages to spuriously fail * respect .gitignore patterns individually, rather than the whole file
purescript · May 29, 2024 · 6c9e1af · 6c9e1af
1 parent c68808a
commit 6c9e1af
Show file tree

Hide file tree

Showing 11 changed files with 145 additions and 184 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,6 +11,7 @@ Other improvements:
 - builds with Cabal successfully
 - update to latest `versions` dependency: https://hackage.haskell.org/package/versions-6.0.1/changelog
 - Fix output truncation with `--json-errors`, many warnings and build failure (#1199)
+- Fixed globbing issue where `/.spago` behaves differently than `.spago` in `.gitignore`
 
 ## [0.21.0] - 2023-05-04
 

diff --git a/spago.lock b/spago.lock
@@ -356,148 +356,6 @@ workspace:
         - unsafe-coerce
         - variant
         - versions
-    perftest:
-      path: perftest
-      dependencies:
-        - aff
-        - arrays
-        - codec-argonaut
-        - console
-        - js-date
-        - lists
-        - node-fs
-        - node-path
-        - node-process
-        - optparse
-        - ordered-collections
-        - record
-        - refs
-        - registry-lib
-        - spago
-        - spago-core
-        - unsafe-coerce
-      test_dependencies: []
-      build_plan:
-        - aff
-        - aff-promise
-        - affjax
-        - affjax-node
-        - ansi
-        - argonaut-core
-        - arraybuffer-types
-        - arrays
-        - assert
-        - avar
-        - bifunctors
-        - catenable-lists
-        - codec
-        - codec-argonaut
-        - console
-        - const
-        - contravariant
-        - control
-        - datetime
-        - distributive
-        - docs-search-common
-        - docs-search-index
-        - dodo-printer
-        - effect
-        - either
-        - enums
-        - exceptions
-        - exists
-        - exitcodes
-        - filterable
-        - fixed-points
-        - foldable-traversable
-        - foreign
-        - foreign-object
-        - fork
-        - form-urlencoded
-        - formatters
-        - free
-        - functions
-        - functors
-        - gen
-        - graphs
-        - http-methods
-        - identity
-        - integers
-        - invariant
-        - js-date
-        - js-timers
-        - js-uri
-        - json-codecs
-        - language-cst-parser
-        - language-purescript
-        - lazy
-        - lcg
-        - lists
-        - maybe
-        - media-types
-        - mmorph
-        - newtype
-        - node-buffer
-        - node-child-process
-        - node-event-emitter
-        - node-execa
-        - node-fs
-        - node-human-signals
-        - node-os
-        - node-path
-        - node-process
-        - node-streams
-        - nonempty
-        - now
-        - nullable
-        - numbers
-        - open-memoize
-        - optparse
-        - ordered-collections
-        - orders
-        - parallel
-        - parsing
-        - partial
-        - pipes
-        - posix-types
-        - prelude
-        - profunctor
-        - profunctor-lenses
-        - psci-support
-        - quickcheck
-        - quickcheck-laws
-        - random
-        - record
-        - refs
-        - registry-lib
-        - routing-duplex
-        - safe-coerce
-        - search-trie
-        - spago
-        - spago-core
-        - spec
-        - st
-        - string-parsers
-        - strings
-        - stringutils
-        - tailrec
-        - these
-        - transformers
-        - tuples
-        - type-equality
-        - typelevel-prelude
-        - unfoldable
-        - unicode
-        - unsafe-coerce
-        - unsafe-reference
-        - variant
-        - versions
-        - web-dom
-        - web-events
-        - web-file
-        - web-html
-        - web-storage
-        - web-xhr
     spago:
       path: ./
       dependencies:

diff --git a/src/Spago/Glob.purs b/src/Spago/Glob.purs
@@ -3,8 +3,10 @@ module Spago.Glob (gitignoringGlob) where
 import Spago.Prelude
 
 import Data.Array as Array
-import Data.String as String
+import Data.Filterable (filter)
 import Data.Foldable (any)
+import Data.String as String
+import Data.String.CodePoints as String.CodePoint
 import Effect.Aff as Aff
 import Effect.Ref as Ref
 import Node.FS.Sync as SyncFS
@@ -48,15 +50,14 @@ gitignoreToMicromatchPatterns base =
   dropSuffixSlash str = fromMaybe str $ String.stripSuffix (String.Pattern "/") str
   dropPrefixSlash str = fromMaybe str $ String.stripPrefix (String.Pattern "/") str
 
+  leadingSlash str = String.codePointAt 0 str == Just (String.CodePoint.codePointFromChar '/')
+  trailingSlash str = String.codePointAt (String.length str - 1) str == Just (String.CodePoint.codePointFromChar '/')
+
   gitignorePatternToMicromatch :: String -> String
   gitignorePatternToMicromatch pattern
-    -- Git matches every pattern that does not include a `/` by basename.
-    | not $ String.contains (String.Pattern "/") pattern = "**/" <> pattern <> "/**"
-    | otherwise =
-        -- Micromatch treats every pattern like git treats those starting with '/'.
-        dropPrefixSlash pattern
-          -- ".spago/" in a .gitignore is the same as ".spago". Micromatch does interpret them differently.
-          # dropSuffixSlash
+    | trailingSlash pattern = gitignorePatternToMicromatch $ dropSuffixSlash pattern
+    | leadingSlash pattern = dropPrefixSlash pattern <> "/**"
+    | otherwise = "**/" <> pattern <> "/**"
 
 fsWalk :: String -> Array String -> Array String -> Aff (Array Entry)
 fsWalk cwd ignorePatterns includePatterns = Aff.makeAff \cb -> do
@@ -92,20 +93,20 @@ fsWalk cwd ignorePatterns includePatterns = Aff.makeAff \cb -> do
           Left _ -> pure unit
           Right gitignore -> do
             let { ignore, patterns } = gitignoreToMicromatchPatterns base gitignore
-            let matcherForThisGitignore = micromatch { ignore } patterns
-
-            -- Does the .gitignore contain a pattern which would ignore the very
-            -- thing we are asked to look for?
-            -- For example, the .gitignore might have the pattern ".spago" but
-            -- `includePatterns` contains a glob like .spago/p/foo-3.1.4/**/*.purs
-            let anyIncludePatternWouldBeIgnored = any matcherForThisGitignore includePatterns
-
-            -- In such a case, do not use this .gitignore for pruning the search.
-            when (not anyIncludePatternWouldBeIgnored) do
+            let gitignored = (micromatch { ignore } <<< pure) <$> patterns
+            let wouldConflictWithSearch m = any m includePatterns
+
+            -- Do not add `.gitignore` patterns that explicitly ignore the files
+            -- we're searching for;
+            --
+            -- ex. if `includePatterns` is [".spago/p/aff-1.0.0/**/*.purs"],
+            -- and `gitignored` is ["node_modules", ".spago"],
+            -- then add "node_modules" to `ignoreMatcher` but not ".spago"
+            for_ (filter (not <<< wouldConflictWithSearch) gitignored) \pat -> do
               -- Instead of composing the matcher functions, we could also keep a growing array of
               -- patterns and regenerate the matcher on every append. I don't know which option is
               -- more performant, but composing functions is more convenient.
-              let addMatcher currentMatcher = or [ currentMatcher, matcherForThisGitignore ]
+              let addMatcher currentMatcher = or [ currentMatcher, pat ]
               void $ Ref.modify addMatcher ignoreMatcherRef
 
       ignoreMatcher <- Ref.read ignoreMatcherRef

diff --git a/test-fixtures/pedantic/follow-instructions/.gitignore b/test-fixtures/pedantic/follow-instructions/.gitignore
@@ -0,0 +1,12 @@
+
+bower_components/
+node_modules/
+.pulp-cache/
+output/
+output-es/
+generated-docs/
+.psc-package/
+.psc*
+.purs*
+.psa*
+.spago
diff --git a/test-fixtures/pedantic/follow-instructions/spago.yaml b/test-fixtures/pedantic/follow-instructions/spago.yaml
@@ -0,0 +1,12 @@
+package:
+  name: follow-instructions
+  run:
+    main: Test.Pedantic.FollowInstructions.Main
+  dependencies:
+    - console
+    - effect
+    - prelude
+workspace:
+  package_set:
+    registry: 0.0.1
+  extra_packages: {}
diff --git a/test-fixtures/pedantic/follow-instructions/src/Main.purs b/test-fixtures/pedantic/follow-instructions/src/Main.purs
@@ -0,0 +1,8 @@
+module Test.Pedantic.FollowInstructions.Main where
+import Prelude
+
+import Effect
+import Effect.Console as Console
+
+main :: Effect Unit
+main = Console.log "wat"
diff --git a/test-fixtures/pedantic/pedantic-instructions-initial-failure.txt b/test-fixtures/pedantic/pedantic-instructions-initial-failure.txt
@@ -1,6 +1,6 @@
 Reading Spago workspace configuration...
 
-✅ Selecting package to build: pedantic
+✅ Selecting package to build: follow-instructions
 
 Downloading dependencies...
 Building...
@@ -14,10 +14,10 @@ Looking for unused and undeclared transitive dependencies...
 
 ❌ Found unused and/or undeclared transitive dependencies:
 
-Sources for package 'pedantic' import the following transitive dependencies - please add them to the project dependencies, or remove the imports:
+Sources for package 'follow-instructions' import the following transitive dependencies - please add them to the project dependencies, or remove the imports:
   effect
-    from `Main`, which imports:
+    from `Test.Pedantic.FollowInstructions.Main`, which imports:
       Effect
 
 These errors can be fixed by running the below command(s):
-spago install -p pedantic effect
+spago install -p follow-instructions effect
diff --git a/test-fixtures/pedantic/pedantic-instructions-installation-result.txt b/test-fixtures/pedantic/pedantic-instructions-installation-result.txt
@@ -1,6 +1,6 @@
 Reading Spago workspace configuration...
 
-✅ Selecting package to build: pedantic
+✅ Selecting package to build: follow-instructions
 
 Adding 1 package to the config in spago.yaml
 Downloading dependencies...

diff --git a/test/Spago.purs b/test/Spago.purs
@@ -8,6 +8,7 @@ import Data.Newtype (un)
 import Effect (Effect)
 import Effect.Aff (Milliseconds(..))
 import Effect.Aff as Aff
+import Test.Spago.Glob as Glob
 import Test.Spago.Build as Build
 import Test.Spago.Bundle as Bundle
 import Test.Spago.Docs as Docs
@@ -56,4 +57,4 @@ main = Aff.launchAff_ $ void $ un Identity $ Spec.Runner.runSpecT testConfig [ S
     Spec.describe "miscellaneous" do
       Lock.spec
       Unit.spec
-
+      Glob.spec
diff --git a/test/Spago/Build/Pedantic.purs b/test/Spago/Build/Pedantic.purs
@@ -192,28 +192,28 @@ spec =
     -- where the fix is to install that missing package.
     -- Following those instructions shouldn't cause an error.
     Spec.it "following installation instructions does not fail with an unrelated pedantic error" \{ spago, fixture } -> do
-      setup spago
-        ( defaultSetupConfig
-            { installSourcePackages = [ "prelude", "console" ]
-            , installTestPackages = []
-            , main = Just
-                [ "import Prelude"
-                , ""
-                , "import Effect"
-                , "import Effect.Console as Console"
-                , ""
-                , "main :: Effect Unit"
-                , "main = Console.log \"wat\""
-                ]
-            , testMain = Nothing
-            }
-        )
+      FS.copyTree { src: fixture "pedantic/follow-instructions", dst: "." }
       spago [ "uninstall", "effect" ] >>= shouldBeSuccess
       -- Get rid of "Compiling..." messages
       spago [ "build" ] >>= shouldBeSuccess
       editSpagoYaml (addPedanticFlagToSrc)
       spago [ "build" ] >>= shouldBeFailureErr (fixture "pedantic/pedantic-instructions-initial-failure.txt")
-      spago [ "install", "-p", "pedantic", "effect" ] >>= shouldBeSuccessErr (fixture "pedantic/pedantic-instructions-installation-result.txt")
+      spago [ "install", "-p", "follow-instructions", "effect" ] >>= shouldBeSuccessErr (fixture "pedantic/pedantic-instructions-installation-result.txt")
+
+    -- Regression test for https://github.com/purescript/spago/pull/1222
+    let gitignores = [".spago", "/.spago", ".spago/**"]
+    for_ gitignores \gitignore ->
+      Spec.it
+        (".gitignore does not affect discovery of transitive deps (" <> gitignore <> ")")
+        \{ spago, fixture } -> do
+          FS.copyTree { src: fixture "pedantic/follow-instructions", dst: "." }
+          FS.writeTextFile ".gitignore" gitignore
+          spago [ "uninstall", "effect" ] >>= shouldBeSuccess
+          -- Get rid of "Compiling..." messages
+          spago [ "build" ] >>= shouldBeSuccess
+          editSpagoYaml (addPedanticFlagToSrc)
+          spago [ "build" ] >>= shouldBeFailureErr (fixture "pedantic/pedantic-instructions-initial-failure.txt")
+          spago [ "install", "-p", "follow-instructions", "effect" ] >>= shouldBeSuccessErr (fixture "pedantic/pedantic-instructions-installation-result.txt")
 
 addPedanticFlagToSrc :: Config -> Config
 addPedanticFlagToSrc config = config