From: Jeff King <peff@peff.net>
Date: Fri, 07 Sep 2018 00:04:24 GMT
Subject: Re: [PATCH 01/11] string_list: print_string_list to use trace_printf
Message-ID: <20180907000423.GA16764@sigill.intra.peff.net>
In-Reply-To: <CAGZ79kbRyVZ_1QpX0pPigN+2Pt2dV1eYvLyoWeSWejbLxko4YQ@mail.gmail.com>

On Thu, Sep 06, 2018 at 03:16:21PM -0700, Stefan Beller wrote:

> > It seems funny that we'd iterate through the list checking over and over
> > whether tracing is enabled.
> >
> > Should this do:
> >
> >   if (!trace_want(&trace_default_key))
> >         return;
> >
> > at the top? (Or possibly even take a trace key from the caller, so that
> > it can use whatever context makes sense for this particular list?)
> 
> I added this check as well as rewording the commit message
> to recite Junios understanding of the patch as well.
> 
> However I would want to not derail this patch any further.
> This function was used as an aid by me some time ago, so I
> am willing to share the modifications needed for efficient
> printf debugging here, but I do not want to be dragged into
> a rabbit hole.
> 
> For example taking the trace key is much overkill IMHO
> for a pure debugging aid (and so is the shortcutting return
> that you proposed, but I added that already for the resend),
> so if anyone needs this function outside of printf-debugging,
> I would recommend patches on top.

I guess the question is: is this a thing we would want to make available
to code to leave in all the time? Or is it just for sticking in
temporarily for a quick dump?

If the former, then I think it needs the early-return at the least (and
probably _should_ have the key parameter).

But I get the feeling that you really just want the latter, and are only
grudgingly being pushed into the former by Junio's suggestion.

-Peff