Skip to content

Commit 2c9892b

Browse files
authored
Do not create ChangesetSpecs on server with empty diff (#287)
I ran into this yesterday with Chris: we ran a command across multiple repositories and in one repository it didn't produce a diff. That's totally to be expected, since you don't know which command will produce which diff in which repository. The problem is that src-cli then sends up the changeset spec, even though the diff is empty and we then try to apply the diff in gitserver, which is where it fails (bonus: our error message looked really good!) This commit here fixes it by not adding a spec with an empty diff to the "list of completed specs". I've looked at a few places where we could add this check (for example: right before we make the GraphQL request we could do an early exit, or before we add it to the cache, ...) and I'm still not 100% whether that's the best place. But I think the requirements are these: - We should not send up empty diffs (because it looks weird in the UI if we say "we will create this changeset", even though it has no diff). - We should still cache the result, even though the diff is empty. - We should be able to report progress when executing it (we did do _something_, but we just didn't produce a diff) - We shouldn't say "sending up 1 changeset spec" if we don't do that That's why I added it where it is, which also required to fix the progress bar for the case where "bar.Max == 0" so we don't run into a panic. I think this is the least invasive option of all the ones I considered (I could also, for example, blast a few `if len(specs) > 0` around the codebase...). Let me know what you think, especially you @LawnGnome!
1 parent f9ea964 commit 2c9892b

File tree

1 file changed

+12
-4
lines changed

1 file changed

+12
-4
lines changed

internal/campaigns/executor.go

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -200,17 +200,25 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) {
200200
status.ChangesetSpec = spec
201201
x.updateTaskStatus(task, status)
202202

203-
// Add the spec to the executor's list of completed specs.
204-
x.specsMu.Lock()
205-
x.specs = append(x.specs, spec)
206-
x.specsMu.Unlock()
203+
// Add it to the status
204+
status.ChangesetSpec = spec
207205

208206
// Add to the cache. We don't use runCtx here because we want to write to
209207
// the cache even if we've now reached the timeout.
210208
if err = x.cache.Set(ctx, cacheKey, spec); err != nil {
211209
err = errors.Wrapf(err, "caching result for %q", task.Repository.Name)
212210
}
213211

212+
// If the steps didn't result in any diff, we don't need to add it to the
213+
// list of specs that are displayed to the user and send to the server.
214+
if len(diff) == 0 {
215+
return
216+
}
217+
218+
// Add the spec to the executor's list of completed specs.
219+
x.specsMu.Lock()
220+
x.specs = append(x.specs, spec)
221+
x.specsMu.Unlock()
214222
return
215223
}
216224

0 commit comments

Comments
 (0)