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

Import refs with filter #1326

Merged
merged 5 commits into from
Mar 2, 2023

Conversation

samueltardieu
Copy link
Contributor

@samueltardieu samueltardieu commented Mar 1, 2023

This patch series implements what is necessary to selectively import git references into jj. The first part was implemented by @ilyagr one week ago (avoid fetching uninteresting heads from a remote repo), and this is the second part, which also fixes some bugs:

  • jujutsu_lib::git::import_some_refs() does the same thing as git/import_refs() but ignores all references not matched by a filter. Ignored references will never cause a branch to be added, moved, or deleted in the jj repo. We could merge the functions, but that will be a backwards incompatible change.
  • jujutsu_lib::git::fetch() uses import_some_refs() when filtering is required. A trace has been let in if at some points the regex built from the globs must be analyzed/optimized.

The CHANGELOG.md file was not updated, as jj git fetch --branch is the sole user-facing change and it has been documented by @ilyagr aready.

Checklist

If applicable:

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

Fixes #1300

Using &[String] forces the caller to materalize owned strings if they
have only references, which is costly. Using &[&str] makes it cheap
if the caller owns strings as well.
@samueltardieu samueltardieu force-pushed the import-refs-with-filter branch from a14e96c to 649e431 Compare March 1, 2023 16:36
lib/src/git.rs Outdated Show resolved Hide resolved
lib/src/git.rs Outdated Show resolved Hide resolved
lib/src/git.rs Outdated Show resolved Hide resolved
lib/src/git.rs Outdated Show resolved Hide resolved
lib/src/git.rs Outdated Show resolved Hide resolved
lib/src/git.rs Outdated Show resolved Hide resolved
In `git_fetch()`, any glob present in `globs` is an "allow" mark. Using
`&[]` to represent an "allow-all" may be misleading, as it could
indicate that no branch (only the git HEAD) should be fetched.

By using an `Option<&[&str]>`, it is clearer that `None` means that
all branches are fetched.
@samueltardieu samueltardieu force-pushed the import-refs-with-filter branch 2 times, most recently from 61444df to ebc380e Compare March 1, 2023 22:47
Copy link
Member

@martinvonz martinvonz left a comment

Choose a reason for hiding this comment

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

Thanks for fixing!

lib/tests/test_git.rs Outdated Show resolved Hide resolved
tests/test_git_fetch.rs Outdated Show resolved Hide resolved
lib/src/git.rs Outdated Show resolved Hide resolved
lib/src/git.rs Show resolved Hide resolved
@ilyagr
Copy link
Contributor

ilyagr commented Mar 2, 2023

Thank you so much! This looks great to me, and I'm very glad this bug is fixed.

While it looks good to me, I'm not super-comfortable with this part of jj to say it does the right thing in every case. So I'll let Martin's review be the approving one.

@samueltardieu samueltardieu force-pushed the import-refs-with-filter branch from ebc380e to be46e48 Compare March 2, 2023 08:49
@samueltardieu samueltardieu merged commit d4b13d7 into jj-vcs:main Mar 2, 2023
@samueltardieu samueltardieu deleted the import-refs-with-filter branch March 2, 2023 09:09
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.

Bug when jj undo-ing jj git fetch --branch
3 participants