Re: [PATCH 1/2] trace: add trace_print_string_list_key
- From
Stefan Beller <sbeller@google.com>
- Date
- Sep 11, 2018, 03:08 UTC
- Message-ID
- <CAGZ79kbRrPnQnPs0FX7_TeiekpmmLzDoSsQpi2fDjc-WkqBf_A@mail.gmail.com>
- In-Reply-To
- <xmqqy3c8rh3d.fsf@gitster-ct.c.googlers.com>
On Mon, Sep 10, 2018 at 5:52 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 38 quoted lines
> > 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.
Show 25 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.Will send a patch with better reasons tomorrow,
Thanks, Stefan