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.

Sign up for GitHub

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

Jump to bottom

sparse-index: fix crash in status #395

Merged
merged 1 commit into from
Jul 2, 2021
Merged

sparse-index: fix crash in status #395

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions sparse-index.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,7 @@ void ensure_full_index(struct index_state *istate)

/* Copy back into original index. */
memcpy(&istate->name_hash, &full->name_hash, sizeof(full->name_hash));
memcpy(&istate->dir_hash, &full->dir_hash, sizeof(full->dir_hash));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wow. Thanks for finding this missing memory copy. This applies to the version of sparse-index in upstream's master, so we should send it up as its own bugfix (independent of other enhancements).

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder whether it would be worth running t1091, t1092, t7519 and t7817 through valgrind on Linux and macOS, which may have caught this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to set up a GitHub workflow to do this: https://github.com/dscho/git/runs/2998707560?check_suite_focus=true

What a rabbit hole. Apparently, brew install valgrind fails because it says it requires Linux... but then I found an unofficial fork that builds on macOS, except it fails because of a missing syscall (I reported this).

On the Linux side, it's just really slow. I also suspect that it runs the tests over and over again because it is still running, at the time of writing it ran for over 45 minutes (granted, some 4-6 minutes of that is the build, not the test).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, so it took ~25 minutes to run the tests (in parallel, t1092 took this long on its own), and it ran things twice because linux-gcc runs the test suite twice, once with default options, and then with a ton of options overridden (such as testing SHA-256 instead of SHA-1). But it did finish eventually.

I'm not sure what to do about testing on macOS, though.

istate->sparse_index = 0;
free(istate->cache);
istate->cache = full->cache;
Expand Down