[1/2] trace: add trace_print_string_list_key
diff mbox series

Message ID 20180910215831.17608-1-sbeller@google.com
State New
Headers show
Series
  • [1/2] trace: add trace_print_string_list_key
Related show

Commit Message

Stefan Beller Sept. 10, 2018, 9:58 p.m. UTC
Similar to trace_strbuf or trace_argv_printf, we might want to output
a string list in tracing. Add such a function.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

I separated this from the other series, making it into 2 patches:
This first patch adds tracing for string lists and the next patch that
removes the unused function from the string list API.
That way we can decide on these two patches separately if needed.

 Documentation/technical/api-trace.txt |  8 ++++++
 trace.c                               | 39 +++++++++++++++++++++++++++
 trace.h                               | 16 +++++++++++
 3 files changed, 63 insertions(+)

Comments

Junio C Hamano Sept. 10, 2018, 10:32 p.m. UTC | #1
Stefan Beller <sbeller@google.com> writes:

> I separated this from the other series, making it into 2 patches:
> This first patch adds tracing for string lists and the next patch that
> removes the unused function from the string list API.
> That way we can decide on these two patches separately if needed.

Of course, even though these are 1/2 and 2/2, only one of them and
not both would apply.

Thanks for sticking to the topic.  

Given how simple that "dump them to standard output" code is, I am
inclined to say that anybody who needs to inspect the contents of
string list at various points in the code under development can
create one from scratch even if we did not have this implementation,
so perhaps 2/2 is a better choice between the two.

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.

I have no strong preference between #2 and #3.  The benefit of #2
compared to #3 is that, if we remove it today, there will not be
somebody else in the future to come and propose removing the
otherwise unused function, which would cost us time to review and
discuss, so unless somebody stops me, I am inclined to say we'd take
2/2 ;-)

Thanks.
Stefan Beller Sept. 10, 2018, 10:38 p.m. UTC | #2
On Mon, Sep 10, 2018 at 3:32 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Stefan Beller <sbeller@google.com> writes:
>
> > I separated this from the other series, making it into 2 patches:
> > This first patch adds tracing for string lists and the next patch that
> > removes the unused function from the string list API.
> > That way we can decide on these two patches separately if needed.
>
> 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.

> Thanks for sticking to the topic.
>
> Given how simple that "dump them to standard output" code is, I am
> inclined to say that anybody who needs to inspect the contents of
> string list at various points in the code under development can
> create one from scratch even if we did not have this implementation,
> so perhaps 2/2 is a better choice between the two.

This sounds like the consensus is not to take both but only 2/2,
which I'd be happy with, too.

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

Thanks,
Stefan
Junio C Hamano Sept. 11, 2018, 12:52 a.m. UTC | #3
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.

>> 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.
Stefan Beller Sept. 11, 2018, 3:08 a.m. UTC | #4
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
Jeff King Sept. 11, 2018, 9:03 p.m. UTC | #5
On Mon, Sep 10, 2018 at 08:08:35PM -0700, Stefan Beller wrote:

> > 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.
> [...]
> 
> 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".

I am perfectly happy with 0. But for reference, your new trace helper
looks completely reasonable from a quick view. Perhaps we can let it
live on in the list archive, and somebody may find a good use for it in
the future (though there is a significant chance that they would not
think to search the archive -- it could even be of value to commit and
revert it so that they find it in "git log", but that may be getting
pretty hypothetical).

-Peff

Patch
diff mbox series

diff --git a/Documentation/technical/api-trace.txt b/Documentation/technical/api-trace.txt
index fadb5979c48..ad0ea99d930 100644
--- a/Documentation/technical/api-trace.txt
+++ b/Documentation/technical/api-trace.txt
@@ -62,6 +62,14 @@  Functions
 	Prints the strbuf, without additional formatting (i.e. doesn't
 	choke on `%` or even `\0`).
 
+`void trace_print_string_list_key(struct trace_key *key, const struct string_list *p, const char *text)::
+
+	Print the string-pointer pairs of the string_list,
+	each one in its own line.
+	It takes an optional key and header argument.
+	If the tracing key is not given, use the default key.
+	The header text is printed before the list itself.
+
 `uint64_t getnanotime(void)`::
 
 	Returns nanoseconds since the epoch (01/01/1970), typically used
diff --git a/trace.c b/trace.c
index fc623e91fdd..e3a12a092f9 100644
--- a/trace.c
+++ b/trace.c
@@ -176,6 +176,38 @@  void trace_strbuf_fl(const char *file, int line, struct trace_key *key,
 	strbuf_release(&buf);
 }
 
+void trace_print_string_list_key_fl(const char *file, int line,
+				    struct trace_key *key,
+				    const struct string_list *p,
+				    const char *text)
+{
+	int i, buf_prefix;
+	struct strbuf buf = STRBUF_INIT;
+
+	if (!key)
+		key = &trace_default_key;
+
+	if (!prepare_trace_line(file, line, key, &buf))
+		return;
+
+	buf_prefix = buf.len;
+
+	if (text) {
+		strbuf_addstr(&buf, text);
+		print_trace_line(key, &buf);
+	}
+
+	for (i = 0; i < p->nr; i++) {
+		strbuf_setlen(&buf, buf_prefix);
+		strbuf_addf(&buf, "%s:%p\n",
+			    p->items[i].string,
+			    p->items[i].util);
+		print_trace_line(key, &buf);
+	}
+
+	strbuf_release(&buf);
+}
+
 static void trace_performance_vprintf_fl(const char *file, int line,
 					 uint64_t nanos, const char *format,
 					 va_list ap)
@@ -227,6 +259,13 @@  void trace_strbuf(struct trace_key *key, const struct strbuf *data)
 	trace_strbuf_fl(NULL, 0, key, data);
 }
 
+void trace_print_string_list_key(struct trace_key *key,
+				 const struct string_list *p,
+				 const char *text)
+{
+	trace_print_string_list_key_fl(NULL, 0, key, p, list);
+}
+
 void trace_performance(uint64_t nanos, const char *format, ...)
 {
 	va_list ap;
diff --git a/trace.h b/trace.h
index 2b6a1bc17c2..0e083891e1f 100644
--- a/trace.h
+++ b/trace.h
@@ -37,6 +37,10 @@  extern void trace_argv_printf(const char **argv, const char *format, ...);
 
 extern void trace_strbuf(struct trace_key *key, const struct strbuf *data);
 
+extern void trace_print_string_list_key(struct trace_key *key,
+					const struct string_list *p,
+					const char *text);
+
 /* Prints elapsed time (in nanoseconds) if GIT_TRACE_PERFORMANCE is enabled. */
 __attribute__((format (printf, 2, 3)))
 extern void trace_performance(uint64_t nanos, const char *format, ...);
@@ -103,6 +107,13 @@  extern void trace_performance_since(uint64_t start, const char *format, ...);
 			trace_strbuf_fl(TRACE_CONTEXT, __LINE__, key, data);\
 	} while (0)
 
+#define trace_print_string_list_key(key, list, text) 			    \
+	do {								    \
+		if (trace_pass_fl(key))					    \
+			trace_print_string_list_key_fl(TRACE_CONTEXT,	    \
+						       __LINE__, key, data);\
+	} while (0)
+
 #define trace_performance(nanos, ...)					    \
 	do {								    \
 		if (trace_pass_fl(&trace_perf_key))			    \
@@ -127,6 +138,11 @@  extern void trace_argv_printf_fl(const char *file, int line, const char **argv,
 				 const char *format, ...);
 extern void trace_strbuf_fl(const char *file, int line, struct trace_key *key,
 			    const struct strbuf *data);
+extern void trace_print_string_list_key_fl(const char *file, int line,
+					   struct trace_key *key,
+					   const struct string_list *p,
+					   const char *text);
+
 __attribute__((format (printf, 4, 5)))
 extern void trace_performance_fl(const char *file, int line,
 				 uint64_t nanos, const char *fmt, ...);