fix(publish): make include and exclude work #22720

dsherret · 2024-03-05T23:42:22Z

Stops deno publish using some custom include/exclude behaviour from other sub commands
Takes ancestor directories into account when resolving gitignore
Backards compatible change that adds ability to unexclude an exclude by using a negated glob at a more specific level for all sub commands (see feat: improve improve/exclude deno_config#44).

Include only deno/ folder

This respects the ignored files in the gitignore:

{
  "publish": [
    "include": ["deno/"]
  ]
}

Include all files except dist/ folder

This respects the ignored files in the gitignore:

{
  "publish": [
    "exclude": ["dist/"]
  ]
}

Include a file/directory that's excluded at the top level

{
  "publish": [
    "exclude": [
      "!dist" // unexclude for publish
    ]
  ],
  "exclude": [
    "dist"
  ]
}

Include a file/directory that's excluded in a gitignore

// gitignore
dist

{
  "publish": [
    "exclude": [
      "!dist" // unexclude for publish
    ]
  ]
}

Include all files except the dist/ folder, but do include dist/js/

{
  "publish": [
    "exclude": [
      "dist/",
      "!dist/js/", // items further down in the array win over items above
    ]
  ]
}

Closes #22648
Closes #22656
Closes #22482
Closes jsr-io/jsr#194

dsherret · 2024-03-07T17:38:43Z

I'm working on updating deno-docs with this information.

dsherret · 2024-03-07T22:19:15Z

cli/lsp/config.rs

@@ -1095,7 +1095,15 @@ impl Config {
        continue;
      };
      let settings = self.workspace_settings_for_specifier(workspace_uri);
-      if settings.enable.unwrap_or_else(|| self.has_config_file()) {


@nayeemrmn this previous condition seemed slightly wrong and caused some trouble in the tests. Is the new condition ok?

Ah good find, but if enable_paths is specified then it should completely override enable.

if settings.enable_paths.as_ref().map(|p| !p.is_empty()).unwrap_or_else(|| previous_condition) { ... }

cli/tools/bench/mod.rs

cli/tools/registry/tar.rs

cli/util/fs.rs

bartlomieju · 2024-03-07T22:46:41Z

cli/util/fs.rs

+            let dir_path = if is_dir { &c } else { c.parent()? };
+            git_ignores.get_resolved_git_ignore(dir_path)
+          });
+        if !is_pattern_matched(


This whole for loop + loop is super hard to grasp. Consider rewriting to use if + continue, but not a blocker to land

I'm going to leave the code as-is for now because doing that is a riskier refactor.

bartlomieju · 2024-03-07T22:47:51Z

ext/fs/in_memory_fs.rs

Is this whole module added just to support testing gitignore functionality? 😬 If so consider making it only available in cfg(test)

Can't make it #[cfg(test)] because then it can't be used outside this crate. I think this will just be optimized away when compiling?

Hm... Since the trait it implements is public maybe move it to the gitignore module?

I think it would be useful in the future for non-CLI crates and Deploy for testing purposes.

bartlomieju · 2024-03-07T22:48:15Z

ext/fs/in_memory_fs.rs

@@ -0,0 +1,424 @@
+// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.
+
+#![allow(clippy::disallowed_types)]


Are there many of them? Maybe leave a comment about this directive usage.

I'm going to add a comment on this. It's just to use Arc, which is fine for this module.

ghostdevv · 2024-03-08T03:03:53Z

The behaviour of include respecting the .gitignore even when you specifically ask it to include a file that has been ignored feels very unintuitive to me - but perhaps I am missing something? To me the include option here feels more specific than the gitignore and should be able to overwrite it - or I should have to do the ! syntax in the include not the exclude

Regression from #22720

dsherret · 2024-03-08T05:22:41Z

@ghostdevv there are so many scenarios with what files should be matched or not. For example, someone might specify the ./src folder and expect gitignored files/directories within that folder to not be matched. Someone else might expect them to be matched. Perhaps someone does ./**/*.json in their include, but they have one .gitignored json file they didn't want included, but someone else in a similar situation did. So the behaviour implemented in this PR is to not include anything that's gitignored for people that want that behaviour, but also providing an explicit opt-out mechanism for people who want to include something that's gitignored.

Also, another point is that "include" is a closed set. The opt-out mechanism works when only specifying "exclude" as well.

That said, and as you allude to, directories or files (non-globs) in the include array should override non-glob .gitignore entries that match the full path or an ancestor directory. I thought it would be complicated to implement because of directories, but I thought of an easy way to implement it. Opened #22790

1. Stops `deno publish` using some custom include/exclude behaviour from other sub commands 2. Takes ancestor directories into account when resolving gitignore 3. Backards compatible change that adds ability to unexclude an exclude by using a negated glob at a more specific level for all sub commands (see denoland/deno_config#44).

Regression from #22720

magic-akari · 2024-03-08T17:42:24Z

I expect that using exclude similar to .npmignore would completely override .gitignore, but that's not the case in practice.
It seems that deno/jsr still follows the .gitignore file as a base and then applying additional exclude rules on top of it.

The situation I encountered is that the .gitignore file is generated by other tools in subdirectories, such as wasm-bindgen, so it is not advisable to modify it. Additionally, the negation exclude: ["!*"] does not take effect.

Is there a way to completely override .gitignore to only enable exclude, making it behave more like .npmignore?

cd pkg
npx jsr publish --dry-run

# pkg/.gitignore
*

# pkg/.npmignore
*.tgz

{
  "exclude": []
}

dsherret · 2024-03-08T17:50:03Z

Oh, that's strange. The negation should work, but it looks like it doesn't. Seems like it only works if I explicitly specify the files or directories at the top level. I'm looking into it.

dsherret · 2024-03-08T17:56:37Z

@magic-akari ok, so the reason it occurs in that scenario is because the .gitignore has * and so it ignores the parent directory. It looks like that's a bug and * in a gitignore should not ignore the parent directory (seems to only occur for the root folder). Real life scenarios that don't use * in a .gitignore should work.

Also, you'll probably want to use !** (matches all files including in descendent directories) and not just !* (only matches stuff in the cwd)

#22805) This is an unrealistic scenario, but it's still a good thing to fix and have a test for because it probably fixes some other underlying issues with how the gitignore was being resolved for the root directory. From #22720 (comment)

ghostdevv · 2024-03-08T21:49:12Z

@dsherret that makes a lot of sense thanks! It's for sure a complicated problem and this is probably the nicest solution

#22805) This is an unrealistic scenario, but it's still a good thing to fix and have a test for because it probably fixes some other underlying issues with how the gitignore was being resolved for the root directory. From #22720 (comment)

dsherret added 9 commits March 5, 2024 18:40

fix(publish): make include and exclude work

5310e44

Merge branch 'main' into fix_publish_include_exclude

19dc874

Update to latest deno_config and fix some publish tests

0f68e2f

Improve gitignore code

746de4e

Add in-memory fs, more tests, and upgrade deno_config

186c513

Lint

7409da9

Latest changes

8c5bd3e

Merge branch 'main' into fix_publish_include_exclude

7ae0a11

Update deno_config

826df37

Merge branch 'main' into fix_publish_include_exclude

c87aa06

dsherret marked this pull request as ready for review March 7, 2024 17:38

dsherret and others added 3 commits March 7, 2024 13:19

Remove out of date comment

2f626ba

Better errors for shadowed negations

2270674

Merge branch 'main' into fix_publish_include_exclude

9e52752

dsherret requested a review from bartlomieju March 7, 2024 21:57

dsherret commented Mar 7, 2024

View reviewed changes

bartlomieju reviewed Mar 7, 2024

View reviewed changes

Review updates.

7c96fde

dsherret requested a review from bartlomieju March 7, 2024 23:16

bartlomieju approved these changes Mar 7, 2024

View reviewed changes

dsherret merged commit 2dfc0ac into denoland:main Mar 8, 2024
17 checks passed

dsherret deleted the fix_publish_include_exclude branch March 8, 2024 01:16

dsherret mentioned this pull request Mar 8, 2024

fix(publish): do not include .gitignore #22789

Merged

dsherret added a commit that referenced this pull request Mar 8, 2024

fix(publish): do not include .gitignore (#22789)

2c6e910

Regression from #22720

littledivy pushed a commit that referenced this pull request Mar 8, 2024

fix(publish): do not include .gitignore (#22789)

4c51fb0

Regression from #22720

dsherret mentioned this pull request Mar 8, 2024

fix(publish): ability to un-exclude when .gitignore ignores everything #22805

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(publish): make include and exclude work #22720

fix(publish): make include and exclude work #22720

dsherret commented Mar 5, 2024 •

edited

Loading

dsherret commented Mar 7, 2024

dsherret Mar 7, 2024

nayeemrmn Mar 7, 2024

bartlomieju Mar 7, 2024

dsherret Mar 7, 2024

bartlomieju Mar 7, 2024

dsherret Mar 7, 2024

bartlomieju Mar 7, 2024

dsherret Mar 7, 2024

bartlomieju Mar 7, 2024

dsherret Mar 7, 2024

ghostdevv commented Mar 8, 2024 •

edited

Loading

dsherret commented Mar 8, 2024 •

edited

Loading

magic-akari commented Mar 8, 2024 •

edited

Loading

dsherret commented Mar 8, 2024

dsherret commented Mar 8, 2024 •

edited

Loading

ghostdevv commented Mar 8, 2024

		@@ -0,0 +1,424 @@
		// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.

		#![allow(clippy::disallowed_types)]

fix(publish): make include and exclude work #22720

fix(publish): make include and exclude work #22720

Conversation

dsherret commented Mar 5, 2024 • edited Loading

dsherret commented Mar 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghostdevv commented Mar 8, 2024 • edited Loading

dsherret commented Mar 8, 2024 • edited Loading

magic-akari commented Mar 8, 2024 • edited Loading

dsherret commented Mar 8, 2024

dsherret commented Mar 8, 2024 • edited Loading

ghostdevv commented Mar 8, 2024

dsherret commented Mar 5, 2024 •

edited

Loading

ghostdevv commented Mar 8, 2024 •

edited

Loading

dsherret commented Mar 8, 2024 •

edited

Loading

magic-akari commented Mar 8, 2024 •

edited

Loading

dsherret commented Mar 8, 2024 •

edited

Loading