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

fcm make: extract: fix target status on conflict #219

Merged

Conversation

matthewrmshin
Copy link
Member

On conflict, the target status was recorded incorrectly as unchanged.
This caused incremental extract to succeed in error. This change fixes
the problem.

@benfitzpatrick @scwhitehouse please review.

On conflict, the target status was recorded incorrectly as unchanged.
This caused incremental extract to succeed in error. This change fixes
the problem.
@@ -1080,7 +1080,6 @@ sub _target_update {
) {
return _target_delete($attrib_ref, $m_ctx, $ctx, $target);
}
$target->set_status($target->ST_UNCHANGED);
my $path = _target_update_source($attrib_ref, $m_ctx, $ctx, $target);
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: the new test passes when just the target->set_status(...) and my $path = _target_update_source(...) lines are swapped.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. The change is to ensure that the target status is ST_UNKNOWN until the very last moment, for safety.

@benfitzpatrick
Copy link
Contributor

Looks fine, tests fine.

@scwhitehouse
Copy link
Contributor

Looks sensible to me.

scwhitehouse added a commit that referenced this pull request Feb 9, 2016
fcm make: extract: fix target status on conflict
@scwhitehouse scwhitehouse merged commit 1f9ff68 into metomi:master Feb 9, 2016
@matthewrmshin matthewrmshin deleted the fix-extract-conflict-incr branch February 9, 2016 15:36
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

3 participants