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

Correct t-lock regular expression to be musl compatible #4673

Merged
merged 1 commit into from Oct 7, 2021

Conversation

fh1ch
Copy link
Contributor

@fh1ch fh1ch commented Oct 7, 2021

This MR corrects a regular expression within t/t-lock.sh to make it compatible with the musl version of libc (see https://wiki.musl-libc.org/functional-differences-from-glibc.html#regular-expressions), as it's used in e.g. Alpine Linux. There is no functional change, just a format one.

I'm currently in the process of bumping git-lfs to version 3.0.1 in Alpine Linux (see https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/26213), but running the integration tests (specifically this one regular expression) as part of the packaging process fail. This can also be reproduced locally:

% docker run -ti alpine /bin/sh
$ grep -E '\[{"id":"[^"]+","path":"a\.dat","owner":{"name":"Git LFS Tests"},"locked_at":"[^"]+"},{"id":"[^"]+","path":"b\.dat","owner":{"name":"Git LFS Tests"},"locked_at":"[^"]+"}\]' lock.json 
grep: bad regex '\[{"id":"[^"]+","path":"a\.dat","owner":{"name":"Git LFS Tests"},"locked_at":"[^"]+"},{"id":"[^"]+","path":"b\.dat","owner":{"name":"Git LFS Tests"},"locked_at":"[^"]+"}\]': Invalid contents of {}

/cc @bk2204

Copy link
Member

@bk2204 bk2204 left a comment

Choose a reason for hiding this comment

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

Hey,

Thanks for the patch. This seems obviously correct.

I'll merge this once CI passes and I'll likely do a 3.0.2 next week (although there's no guarantee). It definitely won't be Monday, since that's a holiday here, but perhaps later in the week.

@fh1ch
Copy link
Contributor Author

fh1ch commented Oct 7, 2021

Hey,

Thanks for the patch. This seems obviously correct.

I'll merge this once CI passes and I'll likely do a 3.0.2 next week (although there's no guarantee). It definitely won't be Monday, since that's a holiday here, but perhaps later in the week.

@bk2204 wow, thanks for this amazingly fast feedback 🙇

No hurries regarding the release from my end, I've already create a local patch for Alpine and there might even be some more integrations tests I have to look into to make them work for packaging.

Regarding CI, there seems to be an unrelated failure (probably just a flaky test) as well as a problem with installing Git (due to the Let's Encrypt intermediate CA expiry, see https://github.com/git-lfs/git-lfs/pull/4673/checks?check_run_id=3828861031). I guess we need to re-trigger those and hope that the second time is a charm, otherwise I'll dig into the issue a bit deeper.

@bk2204
Copy link
Member

bk2204 commented Oct 7, 2021

Regarding CI, there seems to be an unrelated failure (probably just a flaky test) as well as a problem with installing Git (due to the Let's Encrypt intermediate CA expiry, see https://github.com/git-lfs/git-lfs/pull/4673/checks?check_run_id=3828861031). I guess we need to re-trigger those and hope that the second time is a charm, otherwise I'll dig into the issue a bit deeper.

Yes, I think that's a flaky test. I've opened git-lfs/build-dockers#37 to fix the Let's Encrypt issue and will re-run CI once that merges.

@bk2204 bk2204 merged commit d277612 into git-lfs:main Oct 7, 2021
@fh1ch fh1ch deleted the test/lock-regex-musl branch October 7, 2021 19:22
bk2204 added a commit to bk2204/git-lfs that referenced this pull request Oct 25, 2021
Correct t-lock regular expression to be musl compatible
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.

None yet

2 participants