Re: [PATCH 1/2] trace: add trace_print_string_list_key

[Stefan Beller <sbeller@google.com>]

On Mon, Sep 10, 2018 at 5:52 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Stefan Beller <sbeller@google.com> writes:
>
> >> Of course, even though these are 1/2 and 2/2, only one of them and
> >> not both would apply.
> >
> > Or you could squash them once we reach consensus that both are good.
>
> Ah, sorry, I completely misread the first one.  I thought that it
> was extending the implementation of existing unused function by
> using trace API, which explains why I mistook them as an either-or
> choice.  I did not realize 1/2 was adding yet another unused one
> without doing anything to the existing unused one.
>
> So the choice being offered are:
>
>  (0) take 2/2 only, keeping zero unused helper.
>  (1) take 1/2 only, keeping two unused helpers.
>  (2) do nothing, keeping the simple unused helper we had from the
>      beginning of time.
>  (3) take 1/2 and 2/2, replacing one simple unused helper with
>      another unused helper that is more complex and capable.
>
> Are you planning to, or do you know somebody who plans to, use one
> or the other if available in a near future?  If so, it would be OK
> to take choice (2) or (3), and it probably is preferrable to take
> (3) between them.  A more complex and capable one would require
> maintenance over time (trace API is being updated with the trace2 in
> another topic that will start flying soon, so it would be expected a
> user of trace API may need update), but as long as that is actually
> used and help developers, that maintenance cost is worth paying.
>
> If not, I would say that the option (1) or (3) are worse choices
> than either (0) or (2).  It would be better to minimize maintenance
> cost for unused helper(s), and the simpler one is likely to stay
> maintenance free for longer than the more complex and capable one,
> so (1) and (3) do not make much sense unless we plan to start using
> these real soon.

Yes, I think (0) is the way to go, actually.

I wrote patch 1/2 to show Peff and you to prove otherwise that I am
not contributing "only grudgingly".

If the current unused function would be actually helpful in debugging
I would not remove it, but actually use it.

>
> >> It is not costing us much to leave it in the code.  It's not like
> >> the function costed a lot of maintenance burden since it was added
> >> in 8fd2cb40 ("Extract helper bits from c-merge-recursive work",
> >> 2006-07-25), so the alternative #3 might be to do nothing.
> >
> > True, but ...
> >
> >> somebody else in the future to propose removing
> >
> > is what is actually happening here already, see
> >
> > https://public-inbox.org/git/1421343725-3973-1-git-send-email-kuleshovmail@gmail.com/
> >
> >> I am inclined to say we'd take
> >> 2/2 ;-)
> >>
> >> Thanks.
> >
> > Feel free to take Alexanders patch from 2015 instead.
>
> I prefer to take 2/2 over the one from 2015, especially if we can
> explain the removal better.  We had three extra years that the
> helper stayed unused and unchanged, which gives us a better
> indication that it won't be missed.

Will send a patch with better reasons tomorrow,

Thanks,
Stefan