Fix empty changesets being uploaded #313
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
LawnGnome
approved these changes
Sep 9, 2020
LawnGnome
added a commit
that referenced
this pull request
Nov 25, 2020
It's totally valid and normal for empty diffs to be created when executing campaign specs: sometimes you just don't want anything to change, even though the repo matched the initial query. When this happens, #313 added a check that prevents the changeset spec from being created, and print a verbose mode message indicating that the repo was skipped: https://sourcegraph.com/github.com/sourcegraph/src-cli@d29ad54eff678d96fb7ebdf75ff95890dce6a1cf/-/blob/internal/campaigns/executor.go?utm_source=VSCode-1.1.0#L273-278 So far, so good. In #374, we made our empty diff handling even better by caching the empty diff: this means that we don't have to recalculate that nothing happened. Unfortunately, the check that exists in the cache miss code path to skip changeset spec creation doesn't exist in the cache hit code path, which means that on subsequent applications of the campaign, a changeset spec with an empty diff will be uploaded, and gitserver will ultimately be very grumpy. By applying the same logic to the cache hit code path, we can filter out these problematic changeset specs.
LawnGnome
added a commit
that referenced
this pull request
Nov 25, 2020
* campaigns: skip changeset spec creation for cached empty diffs It's totally valid and normal for empty diffs to be created when executing campaign specs: sometimes you just don't want anything to change, even though the repo matched the initial query. When this happens, #313 added a check that prevents the changeset spec from being created, and print a verbose mode message indicating that the repo was skipped: https://sourcegraph.com/github.com/sourcegraph/src-cli@d29ad54eff678d96fb7ebdf75ff95890dce6a1cf/-/blob/internal/campaigns/executor.go?utm_source=VSCode-1.1.0#L273-278 So far, so good. In #374, we made our empty diff handling even better by caching the empty diff: this means that we don't have to recalculate that nothing happened. Unfortunately, the check that exists in the cache miss code path to skip changeset spec creation doesn't exist in the cache hit code path, which means that on subsequent applications of the campaign, a changeset spec with an empty diff will be uploaded, and gitserver will ultimately be very grumpy. By applying the same logic to the cache hit code path, we can filter out these problematic changeset specs. * Extend integration tests to cover the empty diff bug. This also means that we run all the integration tests with cold and warm caches, which should help pick up these issues in future.
scjohns
pushed a commit
that referenced
this pull request
Apr 24, 2023
scjohns
pushed a commit
that referenced
this pull request
Apr 24, 2023
* campaigns: skip changeset spec creation for cached empty diffs It's totally valid and normal for empty diffs to be created when executing campaign specs: sometimes you just don't want anything to change, even though the repo matched the initial query. When this happens, #313 added a check that prevents the changeset spec from being created, and print a verbose mode message indicating that the repo was skipped: https://sourcegraph.com/github.com/sourcegraph/src-cli@d29ad54eff678d96fb7ebdf75ff95890dce6a1cf/-/blob/internal/campaigns/executor.go?utm_source=VSCode-1.1.0#L273-278 So far, so good. In #374, we made our empty diff handling even better by caching the empty diff: this means that we don't have to recalculate that nothing happened. Unfortunately, the check that exists in the cache miss code path to skip changeset spec creation doesn't exist in the cache hit code path, which means that on subsequent applications of the campaign, a changeset spec with an empty diff will be uploaded, and gitserver will ultimately be very grumpy. By applying the same logic to the cache hit code path, we can filter out these problematic changeset specs. * Extend integration tests to cover the empty diff bug. This also means that we run all the integration tests with cold and warm caches, which should help pick up these issues in future.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before, the changesets would still be uploaded, even if the diff was empty. This fixes it.
@LawnGnome is this correct usage of the progress API, or am I working around something here?
Closes #307