Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore Git LFS files #5480

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Ignore Git LFS files #5480

wants to merge 6 commits into from

Conversation

weiznich
Copy link

This PR adds support for ignoring files handled by git lfs. jj previously detected these files as changed as it expected the original file content before the git lfs checkout to be present. With this change it's possible to opt in (via the git.ignore-lfs-files setting) into just ignoring this files from jj. They then need to be manually handled via e.g. git if they are changed.

From a technical point of view this change parses all .gitattributes files in the current repository and constructs a ignore list from that. This ignore list is then applied through all subcommands.

This is still missing tests, as I'm really unsure what to test about and how to write these tests. I likely won't have the capacity to figure that out on my own, so I would appreciate if someone could point to the correct places.

Somewhat addresses #80

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

Copy link

google-cla bot commented Jan 26, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.


```tom
[git]
ignore-lfs-files = -rue

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ignore-lfs-files = -rue
ignore-lfs-files = true

changes in these files, even if they are unchanged. You can configure `jj` to ignore these
files by instructing it to parse the relevant `.gitattributes` files

```tom

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
```tom
```toml

InvalidPattern(String),
}

fn parse_gitattributes(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a gix-attributes crate, probably it's better to use it?

@PhilipMetzger
Copy link
Contributor

Please adhere to the commit style guidelines outlined here: https://github.com/jj-vcs/jj/blob/main/docs/contributing.md#code-reviews

@weiznich
Copy link
Author

I'm sorry, but it's unclear for me to which part of the guide lines you exactly refer and also how that interacts with this kind on commit structure with commits from several authors. For example I fear it's not possible to just squash contributions from other persons into my commits.

@PhilipMetzger
Copy link
Contributor

I'm sorry, but it's unclear for me to which part of the guide lines you exactly refer and also how that interacts with this kind on commit structure with commits from several authors.

I don't think matching the topic: commit description structure and explaining what your commit does is a hard ask, as the project just doesn't squash these commits.

For example I fear it's not possible to just squash contributions from other persons into my commits.

For squashing others commits you can manually add the co-authored-by: trailer to still the credit the original author. This should also be no issue as long as the original author is fine with it, which I assume. cc @bcspragu

@bcspragu
Copy link

For squashing others commits you can manually add the co-authored-by: trailer to still the credit the original author. This should also be no issue as long as the original author is fine with it, which I assume. cc @bcspragu

100%, please do!

Copy link
Contributor

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial minor nits, also cc @yuja as he knows the matcher internals better and should give the final approval.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove

@@ -1277,6 +1277,32 @@ to the current parents may contain changes from multiple commits.
self.path_converter().parse_file_path(input)
}

/// Parses the given strings as file patterns, convert it to a matcher and
/// apply GIT LFS exclusions if requested
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Its Git LFS

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: squash this into the first commit which successfully implements this.

@ilyagr
Copy link
Contributor

ilyagr commented Jan 27, 2025

Are you able to sign the CLA?

@yuja
Copy link
Contributor

yuja commented Jan 27, 2025

he knows the matcher internals better

I haven't reviewed the code, but I wonder if gitattributes could really be abstracted as a Matcher. We deal with .gitignore differently because ignore files have to be loaded from sub trees, and I assume .gitattributes are similar?

If the attributes patterns can be converted to a Matcher, it might make sense to add load_git_attributes(path) -> Result<Matcher, _>, but I wouldn't expect GitAttributesMatcher that does parsing and other heavy lifting within the matcher.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants