git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

Previous: Junio C HamanoNext: Jeff King
Message 14 of 39 in “fetch: make sure submodule oids are fetched”
  1. 00/11 fetch: make sure submodule oids are fetchedStefan Beller, Sep 4, 2018
  2. 01/11 string_list: print_string_list to use trace_printfStefan Beller, Sep 4, 2018
  3. Junio C HamanoSep 6, 2018
  4. Jeff KingSep 6, 2018
  5. Stefan BellerSep 6, 2018
  6. Jeff KingSep 7, 2018
  7. Junio C HamanoSep 7, 2018
  8. Stefan BellerSep 7, 2018
  9. 1/2 trace: add trace_print_string_list_keyStefan Beller, Sep 10, 2018
  10. 2/2 string-list: remove unused function print_string_listStefan Beller, Sep 10, 2018
  11. Junio C HamanoSep 10, 2018
  12. Stefan BellerSep 10, 2018
  13. Junio C HamanoSep 11, 2018
  14. Stefan BellerSep 11, 2018
  15. Jeff KingSep 11, 2018
  16. string-list: remove unused function print_string_listStefan Beller, Sep 11, 2018
  17. Junio C HamanoSep 11, 2018
  18. Junio C HamanoSep 11, 2018
  19. Stefan BellerSep 11, 2018
  20. Junio C HamanoSep 11, 2018
  21. 02/11 string-list.h: add string_list_{pop, last} functionsStefan Beller, Sep 4, 2018
  22. Junio C HamanoSep 6, 2018
  23. Stefan BellerSep 6, 2018
  24. 03/11 sha1-array: provide oid_array_filterStefan Beller, Sep 4, 2018
  25. Junio C HamanoSep 6, 2018
  26. 04/11 submodule.c: convert submodule_move_head new argument to object idStefan Beller, Sep 4, 2018
  27. Junio C HamanoSep 6, 2018
  28. 05/11 submodule.c: fix indentationStefan Beller, Sep 4, 2018
  29. Junio C HamanoSep 6, 2018
  30. 06/11 submodule.c: sort changed_submodule_names before searching itStefan Beller, Sep 4, 2018
  31. Junio C HamanoSep 6, 2018
  32. Stefan BellerSep 11, 2018
  33. 07/11 submodule: move global changed_submodule_names into fetch submodule structStefan Beller, Sep 4, 2018
  34. Junio C HamanoSep 6, 2018
  35. 08/11 submodule.c: do not copy around submodule listStefan Beller, Sep 4, 2018
  36. Junio C HamanoSep 6, 2018
  37. 09/11 submodule: fetch in submodules git directory instead of in worktreeStefan Beller, Sep 4, 2018
  38. 10/11 fetch: retry fetching submodules if sha1 were not fetchedStefan Beller, Sep 4, 2018
  39. 11/11 builtin/fetch: check for submodule updates for non branch fetchesStefan Beller, Sep 4, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.