Re: [PATCH 10/11] builtin/gc: avoid global state in `gc_before_repack()`
- From
Patrick Steinhardt <ps@pks.im>
- Date
- May 30, 2025, 14:05 UTC
- Message-ID
- <aDm7KdKrjhjDToqH@pks.im>
- In-Reply-To
- <6EE3E4F8-16DB-45AB-AD67-FADE12FBFBBD@gmail.com>
On Fri, May 30, 2025 at 08:56:36AM -0400, Ben Knoble wrote:
Show 18 quoted lines
> > @@ -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