Re: [PATCH 01/10] string_list: print_string_list to use trace_printf

From: Stefan Beller <sbeller@google.com>
Date: Aug 9, 2018, 21:40 UTC
Message-ID: <CAGZ79kZ-0R4qnZ7LMhncenOeoec3z+kNmt2_VqBKRiDXxKm6fQ@mail.gmail.com>
In-Reply-To: <xmqq7ekzb5qj.fsf@gitster-ct.c.googlers.com>

On Thu, Aug 9, 2018 at 2:16 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 6 quoted lines

>
> Stefan Beller <sbeller@google.com> writes:
>
> > It is a debugging aid, so it should print to the debugging channel.
>
> Who says?

The comment in the header file?

  /**
   * Dump a string_list to stdout, useful mainly for debugging
   * purposes. It can take an optional header argument and it writes out
   * the string-pointer pairs of the string_list, each one in its own
   * line.
   */

>  In-tree code may say so, and I do not think any in-flight
> topic up to 'pu' uses this to produce non-debugging output, but I do
> not think it is healthy attitude to assume that you can take over an
> existing function and change what it does unilaterally.

That is reasonable, as usual, and given the recent fallout of the object store refactoring I have these concerns on my mind, too.

But for this instance, I do not see any risk of accidental collisions with other series in flight:

$ git log -S print_string_list origin/pu --oneline 4f665f2cf33 string-list.h: move documentation from Documentation/api/ into header d10cb3dfab9 help.c: rename function "pretty_print_string_list" c455c87c5cd Rename path_list to string_list 1eb056905a2 include $PATH in generating list of commands for "help -a" 70827b15bfb Split up builtin commands into separate files from git.c 27dedf0c3b7 (tag: v1.0rc3, tag: v0.99.9j) GIT 0.99.9j aka 1.0rc3 8e49d503882 C implementation of the 'git' program, take two.

Show 5 quoted lines

> As there is no in-tree or in-flight user, I think it makes sense if
> the proposed change were to rename it to trace_string_list().  If
> there weren't any print_string_list() and we were adding a debugging
> aid to use trace_printf() to dump these, that would be the name we
> would use anyway.

Good call. I'll rename and send separately (or might even drop this patch completely; this patch is not used in the patches to follow; it is more a "FYI: here is a thing that I found helpful for debugging, unlike the existing function, which I found unhelpful and actively harmful")