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

Fix sparse path matching #36

Merged
merged 2 commits into from
Jan 29, 2024
Merged

Fix sparse path matching #36

merged 2 commits into from
Jan 29, 2024

Conversation

cmbartschat
Copy link

@cmbartschat cmbartschat commented Jan 28, 2024

Context

There was unexpected behavior on the following sparse checkout file:

/*
!/*/
/example/a/
/example/b/

Part of the problem is that prefixes like example/ were not matching any of the include rules, so we never recursed into the example/a/ directory for example.

This prompted an investigation into no-cone vs cone and based on our reading of https://git-scm.com/docs/git-sparse-checkout, it's a better idea going forward to simply commit to cone mode.

Testing

  • Test cases in monorepo pass
  • New unit tests pass:

make libgit2_tests && ./libgit2_tests -ssparse::paths

  • When the sparse config contains invalid paths, g8 will reject:

Screenshot 2024-01-27 at 7 03 59 PM

  • ⚠️ Certain sparse configs have unexpected behavior, though git does not complain about the format.

With:

/*
!/*/
/A/B/
/C/D/E/

The "A" folder becomes present in the workdir, with the "B" folder inside that, but "C" doesn't appear.

It seems like git operates as if

/A/B/

is equivalent to

/A/
!/A/*/
/A/B/

but

/C/D/E/

by itself is ignored altogether.

Or maybe it's a path matching bug that intersects weirdly with the "always checkout top level files" rule.

Potentially some more experimentation needed, but bottom line, we now have a sane mental model on sparse matching as long as parent folders are included. I can also follow up to add a check for that.

}

if (match->length == 1 && match->pattern[0] == '*') {
// NOTE(christoph): "/*" and "!/*/" both parse to "*", either to:

Choose a reason for hiding this comment

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

You can remove your name from the note

Copy link
Author

Choose a reason for hiding this comment

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

Done

int *checkout,
git_attr_file *file,
git_attr_path *path)
{
size_t j;
git_attr_fnmatch *match;

int path_length = strlen(path->path);
if (is_top_level_file(path) ) {
*checkout =GIT_SPARSE_CHECKOUT;

Choose a reason for hiding this comment

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

Looks like there is some tabs vs. spaces thing going on here leading to non-consistent whitespace.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

Copy link

@erikmchut erikmchut left a comment

Choose a reason for hiding this comment

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

Nice work

@cmbartschat cmbartschat merged commit 46b4b53 into main Jan 29, 2024
10 of 26 checks passed
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.

2 participants