Re: [PATCH v5 4/6] list-objects: filter objects in traverse_commit_list

From: Jeff King <peff@peff.net>
To: Jeff Hostetler <git@jeffhostetler.com>
Cc: Stefan Beller <sbeller@google.com>, git <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>,
	Jonathan Tan <jonathantanmy@google.com>,
	Jeff Hostetler <jeffhost@microsoft.com>
Subject: Re: [PATCH v5 4/6] list-objects: filter objects in traverse_commit_list
Date: Thu, 30 Nov 2017 17:03:15 -0500	[thread overview]
Message-ID: <20171130220315.GA13784@sigill.intra.peff.net> (raw)
In-Reply-To: <21fbd685-2c96-91b9-700e-8902a6e99c08@jeffhostetler.com>

On Mon, Nov 27, 2017 at 02:39:43PM -0500, Jeff Hostetler wrote:

> On 11/22/2017 5:56 PM, Stefan Beller wrote:
> > On Tue, Nov 21, 2017 at 12:58 PM, Jeff Hostetler <git@jeffhostetler.com> wrote:
> > > +       assert(arg);
> > > +       assert(!unset);
> > 
> > I count 16 asserts in this patch. Is that really needed?
> > Either omit them or use BUG if we want to rely on user
> > bug reports when these conditions trigger, as assert is unreliable
> > due to its dependence on the NDEBUG flag.
> 
> Yes, there are a few asserts in the code.  Old habits....
> 
> I could remove some/all of them, but personally I feel they
> have merit and hint to the mindset of the author for future
> readers of the code.  Are there other opinions?

I think I'd prefer in general to see assertions remain in one form or
another, if only because of the documentation benefits you mention here.

I do think there's such a thing as too many asserts, but I don't think I
see that here. "Too many" would probably be something like asserting
things that are a normal part of the contract (so "assert(foo)" on
every pointer parameter coming in to make sure it's not NULL).

I thought at first that's what was happening with the ones quoted above,
but it's actually documenting that no, we do not support "--no-filter"
in opt_parse_list_objects_filter (which is really checking that we're in
sync with the PARSE_OPT_NONEG found elsewhere).

So arguably my confusion argues that this one ought to have a custom
message or a comment.

Of course, it also makes me wonder whether we ought to just support
--no-filter. Shouldn't it just set us back to FILTER_DISABLED?

> Personally, I think it might be awkward to keep repeating
> something like:
> 
>     if (!c)
>         BUG(msg);
> 
> Do we want to think about a macro that builds on BUG() and
> does the test?
> 
> Something like:
>     #define ASSERT_OR_BUG(c) do { if (!(c)) BUG("%s", #c); } while (0)

Yeah, I think that was where the other thread[1] led to. IMHO that's
probably what BUG_ON() ought to do (though personally I'm fine with just
continuing to use assert for simple cases).

I think we can sidestep the whole variadic-macros thing mentioned in
that thread since we don't take a custom message.

-Peff

[1] https://public-inbox.org/git/20171122223827.26773-1-sbeller@google.com/