Jeff King <peff@xxxxxxxx> writes: > On Fri, May 12, 2017 at 01:46:48PM -0700, Jonathan Tan wrote: > >> Change from v5: used "ensure_tip_oids_initialized" function instead. >> This removes some of the muddiness (e.g. with newlist being modified >> after the function). > > I don't think it really improves the muddiness. You are still calling > the ensure function each time through the loop with a potentially > modified list, but it doesn't actually look at the list after the first > time. So the muddy part is still there. > > I think rather than changing the code, you'd do better with a comment > like: > > /* > * Note that this only looks at the ref lists the first time > * it's called. This works out in filter_refs() because even > * though it may add to "newlist" between calls, the additions > * will always be for oids that are already in the set. > */ > > At which point the original all-in-one function is probably fine (as it > avoids the "return 1 just to join the &&-chain" bit). Yes. I agree that the in-code comment is the best approach to the "muddy combination of 'grabbing it for the first time' and 'the thing that is grabbed mutates in the loop'" confusion.