From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 31 Aug 2026 08:13:58 GMT
Subject: Re: [PATCH 5/8] builtin/clone: move setup of alternates for non-shared local clones
Message-ID: <apU3xpOZf0b9P6wT@pks.im>
In-Reply-To: <874igeuwja.fsf@emacs.iotcl.com>

On Fri, Aug 28, 2026 at 04:52:57PM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/builtin/clone.c b/builtin/clone.c
> > index 08c8f5a94f..2e3473fddf 100644
> > --- a/builtin/clone.c
> > +++ b/builtin/clone.c
> > @@ -195,29 +195,41 @@ static void copy_alternates(struct strbuf *src, const char *src_repo)
> >  	 * to turn entries with paths relative to the original
> >  	 * absolute, so that they can be used in the new repository.
> >  	 */
> > -	FILE *in = xfopen(src->buf, "r");
> > +	FILE *in;
> > +	struct strbuf path = STRBUF_INIT;
> >  	struct strbuf line = STRBUF_INIT;
> >  
> > +	strbuf_addf(&path, "%s/objects/info/alternates", src_repo);
> > +
> > +	in = fopen(path.buf, "r");
> > +	if (!in) {
> > +		if (errno == ENOENT)
> > +			goto out;
> > +		die_errno("could not read alternates file '%s'", path.buf);
> > +	}
> > +
> >  	while (strbuf_getline(&line, in) != EOF) {
> >  		char *abs_path;
> >  		if (!line.len || line.buf[0] == '#')
> >  			continue;
> >  		if (is_absolute_path(line.buf)) {
> > -			odb_add_to_alternates_file(the_repository->objects,
> > -						   line.buf);
> > +			strvec_push(alternates, line.buf);
> >  			continue;
> >  		}
> >  		abs_path = mkpathdup("%s/objects/%s", src_repo, line.buf);
> >  		if (!normalize_path_copy(abs_path, abs_path))
> > -			odb_add_to_alternates_file(the_repository->objects,
> > -						   abs_path);
> > +			strvec_push(alternates, abs_path);
> >  		else
> >  			warning("skipping invalid relative alternate: %s/%s",
> >  				src_repo, line.buf);
> >  		free(abs_path);
> >  	}
> > +
> > +out:
> > +	strbuf_release(&path);
> >  	strbuf_release(&line);
> > -	fclose(in);
> > +	if (in)
> > +		fclose(in);
> 
> Why not put this before the `out` label and remove the if?

Mostly because it feels fragile to me. If we were to ever extend this
function to have another `goto out` it's easy to miss that we don't
close `in` anymore.

Patrick