Subject: Re: [PATCH v2 02/16] odb: move list of packfiles into `struct packfile_store`

From: Patrick Steinhardt <[email-masked]>
Date: Wed, 3 Sep 2025 07:58:31 +0200

On Tue, Sep 02, 2025 at 10:42:25AM -0700, Junio C Hamano wrote:
> Taylor Blau <[email-masked]> writes:
> 
> > So I think in this case, the CodingGuidelines would suggest that we
> > write this as:
> >
> >     for (p = store->packs; p; p = p->next) {
> >         if (p->do_not_close)
> >             BUG("want to close pack marked 'do-not-close'");
> >         else
> >             close_pack(p);
> >     }
> >
> > , which from our discussion here seems like something that we both find
> > more readable than the original.
> 
> Yes.  Technically the "if...else..." is still a single statement, so
> a rule like "do not use {} only if you would place a single
> statement in it", though.
> 
> I would actually write it more like this, though.
> 
>      for (p = store->packs; p; p = p->next) {
>          if (p->do_not_close)
>              BUG("want to close pack marked 'do-not-close'");
> 
>          close_pack(p);
>      }
> 
> The first two lines in that block is a glorified assert(), and
> without a programming bug, what the loop wants to do is only to call
> close_pack() on eacn and every pack on the list.  Not using "else"
> conveys that much clearer.

That reads even better, agreed. Will queue this change locally and send
it out with the next revision of this patch series.

Patrick