Re: [PATCH 4/4] odb/source-files: only reload alternates when the file has changed

[Patrick Steinhardt <ps@pks.im>]

On Tue, Oct 20, 2026 at 12:59:58PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > +	if (!validity || !stat_validity_check(validity, path)) {
> > +		int fd = open(path, O_RDONLY);
> > +		if (fd < 0) {
> > +			warn_on_fopen_errors(path);
> > +			if (validity)
> > +				stat_validity_clear(validity);
> > +		} else {
> > +			if (validity)
> > +				stat_validity_update(validity, fd);
> > +			if (strbuf_read(&buf, fd, 1024) < 0)
> > +				warning_errno(_("unable to read alternates file '%s'"),
> > +					      path);
> > +			else
> > +				parse_alternates(buf.buf, '\n', object_dir, out);
> > +			close(fd);
> 
> When we update validity and then fail to strbuf_read(), say due to a
> transient I/O error or something that was caused by somebody else
> mucking with our alternates list at the same time, we warn, but do
> not roll back the validity update even though we failed to parse.  I
> wonder if it is safer to run the validity update only after
> successfully read the alternates file, and if the extra safety is
> worth it?

Good question. I doubt that the extra safety would really be worth it in
the general case as it's rather unlikely that we can open the path but
then not read from it. But that doesn't feel like a good-enough argument
to be sloppy, because "unlikely" doesn't mean "impossible".

So we could do something like this instead:

	if (!validity || !stat_validity_check(validity, path)) {
		bool successful = false;
		int fd;

		fd = open(path, O_RDONLY);
		if (fd < 0) {
			warn_on_fopen_errors(path);
		} else if (strbuf_read(&buf, fd, 1024) < 0) {
			warning_errno(_("unable to read alternates file '%s'"),
				      path);
		} else {
			parse_alternates(buf.buf, '\n', object_dir, out);
			successful = true;
		}

		if (validity) {
			if (successful)
				stat_validity_update(validity, fd);
			else
				stat_validity_clear(validity);
		}

		if (fd >= 0)
			close(fd);
	}

I've queued that change locally for now.

Thanks!

Patrick