Re: [PATCH v4 10/12] refs: implement removal of ref storages

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 1492 bytes --]

On Wed, Jun 05, 2024 at 06:12:00AM -0400, Jeff King wrote:
> On Mon, Jun 03, 2024 at 11:30:55AM +0200, Patrick Steinhardt wrote:
> 
> > +static int files_ref_store_remove_on_disk(struct ref_store *ref_store,
> > +					  struct strbuf *err)
> > +{
> > [...]
> > +	strbuf_addf(&sb, "%s/refs", refs->base.gitdir);
> > +	if (remove_dir_recursively(&sb, 0) < 0) {
> > +		strbuf_addf(err, "could not delete refs: %s",
> > +			    strerror(errno));
> > +		ret = -1;
> > +	}
> > +	strbuf_reset(&sb);
> > +
> > +	strbuf_addf(&sb, "%s/logs", refs->base.gitdir);
> > +	if (remove_dir_recursively(&sb, 0) < 0) {
> > +		strbuf_addf(err, "could not delete logs: %s",
> > +			    strerror(errno));
> > +		ret = -1;
> > +	}
> > +	strbuf_reset(&sb);
> 
> If removing either of the directories fails, we set ret to "-1". Make
> sense. But...
> 
> > +	ret = for_each_root_ref(refs, remove_one_root_ref, &data);
> > +	if (ret < 0)
> > +		ret = -1;
> 
> ...then we unconditionally overwrite it, forgetting the earlier error.
> Either we should jump to the end on the first failure, or if the goal is
> to do as much as possible, should we |= the result? I'm not clear why we
> assign "ret" and then immediately check it to assign "-1" again.
> 
> Is that a mistake, or are we normalizing other negative values? Maybe
> just:
> 
>   if (for_each_root_ref(refs, remove_one_root_ref, &data) < 0)
> 	ret = -1;
> 
> would work?

Yup, that would work, good catch.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]