Return result code when process is stopping [M147]

[jeschu1]

Return result code instead of null, fixing a null reference

There will be a further change (not needed for this release) to better log / audit return codes consistently across maintenance jobs.

[wilbaker]

Why isn't ExitDueToShutDownCode considered a failure?

[wilbaker]

To add more context, my concern is that callers might think that the git command has succeeded when it hasn't and then react incorrectly.

Did you check how callers are treating ExitCodeIsFailure?

[kewillford]

We were looking at the code in PackfileMaintenanceStep where it would log "multi-pack-index is corrupt after write. Deleting and rewriting." on a failure. Which was not the case if we never even invoked git to run.

if (this.Stopping)
{
	this.Context.Tracer.RelatedWarning(
		metadata: null,
		message: this.Area + ": Not launching Git process because the mount is stopping",
		keywords: Keywords.Telemetry);
	return null;
}

GitProcess.Result result = work.Invoke(this.GitProcess);

So it seems that this exit code is not a failure of git but an explicit choice or not running the git command. I understand your concern about assuming success and we should audit where this is checked to make sure. But we also have the ExitCodeIsSuccess which if we really care about it succeeding should be checking that. I would be good to know if we have discrepancies in our logic though.

[jeschu1]

Exactly what @kewillford said :-)

Further that the only place we have a result of ExitDueToShutDownCode is in MaintenanceJobs and there we'd only want to log if we have a failure. We already log (as shown above) when the process isn't launced due to stopping.

[wilbaker]

@kewillford good point thanks!

@jeschu1 and I discussed this offline and agreed that the changes in this PR are good to go into releases/shipped, but that we'll want to take some time to see if there is a better solution before merging to master.

One specific concern we discussed was that ExitDueToShutDownCode is a GitMaintenanceStep specific type of result, and so it probably should not be added to GitProcess.Result (which is used in lots of places, not just in GitMaintenanceStep).