campaigns: skip changeset spec creation for cached empty diffs #397
Conversation
This file contains 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
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.
eseliger
approved these changes
Nov 25, 2020
| // send to the server. Instead, we can just report that the task is | ||
| // complete and move on. | ||
| if len(diff) == 0 { | ||
| status.FinishedAt = time.Now() |
There was a problem hiding this comment.
I think this line could also be moved up and deduped
There was a problem hiding this comment.
I thought about it, but kept it there because createChangesetSpec() really marks the end of executing the task in the normal case, and I'd like to account for that time as well.
This also means that we run all the integration tests with cold and warm caches, which should help pick up these issues in future.
|
Just asking for a quick re-review here to glance over the test changes, since they're moderately significant. |
eseliger
approved these changes
Nov 25, 2020
Comment on lines
+133
to
+134
| // No changesets should be generated. | ||
| wantFilesChanged: map[string][]string{}, |
There was a problem hiding this comment.
I don't 100% understand how this tests the behavior is right but I trust you that it does what we look for. Regardless this is a good test.
I assume this would have failed before the fix because the hot run is going to yield a change? Even though it was empty? Not sure why it would show any files changed when there was an empty diff.
There was a problem hiding this comment.
Each entry in wantFilesChanged represents a changeset spec. In this case, since true won't change anything, the test should hit the empty diff code paths and not generate changeset specs, even though the cache is populated.
This does indeed fail on the old main:
~/trees/sourcegraph/src-cli on main via 🐹 v1.15.5 ❯ go test -race ./...
ok github.com/sourcegraph/src-cli/cmd/src 0.224s
ok github.com/sourcegraph/src-cli/internal/api (cached)
--- FAIL: TestExecutor_Integration (0.46s)
--- FAIL: TestExecutor_Integration/empty (0.04s)
executor_test.go:189: wrong number of changeset specs. want=0, have=1
--- FAIL: TestExecutor_Integration/empty/warm_cache (0.00s)
testing.go:1048: test executed panic(nil) or runtime.Goexit: subtest may have called FailNow on a parent test
FAIL
FAIL github.com/sourcegraph/src-cli/internal/campaigns 0.565s
? github.com/sourcegraph/src-cli/internal/campaigns/graphql [no test files]
ok github.com/sourcegraph/src-cli/internal/codeintel (cached)
? github.com/sourcegraph/src-cli/internal/output [no test files]
ok github.com/sourcegraph/src-cli/internal/servegit (cached)
? github.com/sourcegraph/src-cli/schema [no test files]
FAIL
There was a problem hiding this comment.
Ahh okay makes sense. Thanks for confirming!
LawnGnome
added a commit
to sourcegraph/sourcegraph-public-snapshot
that referenced
this pull request
Nov 26, 2020
The previous recommended version suffered from the bug fixed by sourcegraph/src-cli#397, so we'll want to move our users past that.
|
Ah man, what a bug! Thanks for fixing this so diligently! |
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.
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.