Skip to content

Distiguishing "finished exec" and "built specs" in TaskStatus

Warren Gifford requested to merge mrn/fix-verbose-regression into main

Created by: mrnugget

This fixes the regression reported in https://github.com/sourcegraph/sourcegraph/issues/21230 and introduced by yours truly in https://github.com/sourcegraph/src-cli/commit/d6c876cd32397ad53f1e5df1a850fcac7076ba3e.

With the introduction of the Coordinator and the explicit steps of checking-the-cache-for and building-of ChangesetSpecs outside the executor, the TaskStatus.ChangesetSpecs field wasn't set at the time when FinishedAt was set.

The batchProgressPrinter assumed, though, that this was the case and that if FinishedAt was set that len(taskStatus.ChangesetSpecs) == 0 means "No changes".

This change here fixes the problem by distinguishing between the two states: finished execution of steps and finished building changeset specs.

The problem is that it's still a slight regression in behaviour: previously the diff stats would be printed in the status bar and in the verbose mode as tasks were finishing.

Now that we build all changeset specs at once, after all of them have been executed, we can't update the status bar to include diff stats and the verbose messages will be logged all at once.

I still think that the current code (with the Coordinator) is better than what we had before and the hard problem here is fixed (no wrong information being displayed), but longer term I think there's a solution possible in which we decouple the task execution and its status much more and make it possible to build better UIs for the status of execution.

That I think should be approached separately though.

Merge request reports

Loading