Re: [PATCH 10/11] builtin/gc: avoid global state in `gc_before_repack()`

[Patrick Steinhardt <ps@pks.im>]

On Fri, May 30, 2025 at 08:56:36AM -0400, Ben Knoble wrote:
> > @@ -965,7 +957,8 @@ int cmd_gc(int argc,
> >          goto out;
> >      }
> > 
> > -        gc_before_repack(&opts, &cfg); /* dies on failure */
> > +        if (gc_before_repack(&opts, &cfg) < 0)
> > +            exit(127);
> 
> If I (a relative novice to this part of the code) am reading
> correctly, we trade an implicit die in a private helper for explicit
> exit in a « main » function, which I find much easier to reason about.
> Nice!
> 
> What I don’t see (being away from the rest of the source at the
> moment) is where 127 comes from. I don’t intend a crusade against
> magic numbers :) and I’ve certainly seen enough exit-codes of 127 to
> guess what this means, but reading only the patch the number does
> appear out of thin air.

The funny thing is that 127 isn't even correct -- it should be 128.
Maybe we can adapt `die_builtin()` so that it knows to not write
anything when the first argument is a NULL pointer?

Patrick