api: avoid unnecessary file write operation

[Bo98]

Fixes the HOMEBREW_NO_AUTO_UPDATE part of #14663.

File.touch always modifies the file, even if it already exists, because it refreshes mtime etc.

[BrewTestBot]

Review period skipped due to critical label.

[MikeMcQuaid]

This will break the (Time.now - Homebrew::EnvConfig.api_auto_update_secs.to_i) < target.mtime) handling because we use the time from the server.

It also feels very weird to deliberately pass an invalid JSON string to use that parse as flow control.

[Bo98]

It also feels very weird to deliberately pass an invalid JSON string to use that parse as flow control.

We already do this by touching, but can catch ENOENT instead if you reckon nothing else here will trigger it.

[MikeMcQuaid]

We already do this by touching, but can catch ENOENT instead if you reckon nothing else here will trigger it.

I think we should avoid attempting to parse the JSON if the file doesn't exist. If the file does exist: we need to touch it every time we've checked for a download, even if we didn't download anything new.

[MikeMcQuaid]

Suggested change

	File.touch(target)
	end
	end
	File.touch(target)

Are you sure we don't need to still touch if we skipped the download?

[Bo98]

That would just cause it to drift to 450 seconds after the last brew command rather than 450 seconds after the last update attempt, no?

[Bo98]

I think we should avoid attempting to parse the JSON if the file doesn't exist

The retry flow sounds messy for that scenario, but I suppose it should maybe always exist at that point so probably safe to ignore, yeah.

If the file does exist: we need to touch it every time we've checked for a download, even if we didn't download anything new.

Sounds like we want FileUtils.touch(target) unless skip_download?

[MikeMcQuaid]

Sounds like we want File.touch(target) unless skip_download?

Yeh, something like that. We're using the target.mtime as the timestamp "we checked with the server and there was no updates". As long as you're testing that flow locally: I trust you to come up with the right logic here.

[Bo98]

We're using the target.mtime as the timestamp "we checked with the server and there was no updates".

Yeah it's a weird situation where using the server time would actually be better for --time-cond but worse for the update interval case.

Anyhow, this PR should be good now.

[MikeMcQuaid]

Thanks again @Bo98!