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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 11, 2018, 00:52 UTC
Message-ID: <xmqqy3c8rh3d.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CAGZ79kY2XpB4nJOJc0LLueqsutpncgwdN8uC=Wj0Uxu9Yjgd8A@mail.gmail.com>

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.

Show 19 quoted lines

>> 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.

Thanks.