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

Bug fix: no-op dolt_pull() was leaving working set dirty #7882

Merged
merged 3 commits into from
May 21, 2024
Merged

Conversation

zachmu
Copy link
Member

@zachmu zachmu commented May 21, 2024

Customer-reported bug. Two dolt_pull() operations on two branches in the same session when local branches are already up to date, with @@autocommit off, leave the session unable to commit because two branch heads are considered dirty. See new bats test for details on reproducing.

The issue is that DoltSession.SetWorkingSet() marks that branch head dirty until the transaction is committed. Most merge code paths used by pull involve performing a dolt_commit(), which has the side effect of zeroing out the current transaction, meaning the next statement would get a new transaction and fresh working sets loaded from disk, avoiding the dirty state problem. Only the code path where the branch head is already up to date is affected by this bug. All the merge library code that actually needs to call DoltSession.SetWorkingSet() (only necessary before a dolt_commit happens, or in the case of a squash where changes should remain in the working set) already does so, making the additional call in dolt_pull.go redundant and leading to this buggy behavior in the no-change case.

There are probably still related bugs for session state management during pull and merge operations, but I want to keep this fix narrow to address the customer issue while I build up more robust (non-bats) tests for pull.

@zachmu zachmu requested a review from fulghum May 21, 2024 01:17
@coffeegoddd
Copy link
Contributor

@zachmu DOLT

comparing_percentages
100.000000 to 100.000000
version result total
f28be4c ok 5937457
version total_tests
f28be4c 5937457
correctness_percentage
100.0

Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

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

Nice fix! 🚢

@zachmu zachmu merged commit 4cd8fb7 into main May 21, 2024
33 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants