| Message ID | 20180910215831.17608-1-sbeller@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] trace: add trace_print_string_list_key | expand |
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.
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
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.
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
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
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, ...);
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(+)