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:
signature.asc
Description: PGP signature
