Re: [PATCH 1/2] Simplify tracing code by removing trace key normalization concept

[Junio C Hamano]

Junio C Hamano  writes:

> The thought behind the change flows much better in the above
> explanation than your four-bullet list (which a reader would often
> assume are parallel and orthogonal).  "Remove this, because it is
> not used" is the primary thing for this step, and the fact that the

Make that "because it is not strictly necessary", as that seems to
be much closer to what is going on, and I agree that it is a good
change to remove such "convenience for callers" feature from
codepaths that could be performance critical.  The callers can pass
the "" thing themselves just as easily as they pass NULL
and have the library code to do the defaulting, and that will remove
the need to incur extra cost for a subroutine call.

Re: [PATCH 1/2] Simplify tracing code by removing trace key normalization concept

[Junio C Hamano]

Gennady Kupava  writes:

>> The usual style comment on the subject applies here.
>
> Oh sure, 50 characters. 'Remove trace key normalization concept' would
> be better?

I was referring to #summary-section of Documentation/SubmittingPatches

The first line of the commit message should be a short description (50
characters is the soft limit, see DISCUSSION in linkgit:git-commit[1]),
and should skip the full stop.  It is also conventional in most cases to
prefix the first line with "area: " where the area is a filename or
identifier for the general area of the code being modified, e.g.

* doc: clarify distinction between sign-off and pgp-signing
* githooks.txt: improve the intro section

If in doubt which identifier to use, run `git log --no-merges` on the
files you are modifying to see the current conventions.

[[summary-section]]
It's customary to start the remainder of the first line after "area: "
with a lower-case letter. E.g. "doc: clarify...", not "doc:
Clarify...", or "githooks.txt: improve...", not "githooks.txt:
Improve...".

> So comments explaining that, while implementing trace optimization, I
> saw two options:
> 1. Move normalization function from .c file to .h file
> 2. Remove it
>
> And why I choose removal - not used, would complicate header without
> any benefit, and actually I didn't mention minor benefit of
> compilation speed. This trace.h included and used in lots of places so
> it will take compiler some time to actually eliminate the code.
>
>> Puzzled.
>
> Does it make more sense now?

The thought behind the change flows much better in the above
explanation than your four-bullet list (which a reader would often
assume are parallel and orthogonal).  "Remove this, because it is
not used" is the primary thing for this step, and the fact that the
later step benefit from that change, while it may be more important
to the person who want to see that later change, is incidental to
see if this change makes sense as a standalone patch.

And that primary thing, "remove this, because it is not used and
only complicates the code" is what we want to start the log message
with.  The first sentence ("this needs to be moved there") saying
what the patch chose not to was the source of the confusion.

Re: [PATCH 1/2] Simplify tracing code by removing trace key normalization concept

[Gennady Kupava]

> The usual style comment on the subject applies here.

Oh sure, 50 characters. 'Remove trace key normalization concept' would
be better?

> I cannot quite tell what it is trying to achive to make it a
bulleted list.  It's not like four things at the same conceptual
level is enumerated; instead it just has four sentences that talk
about random things.

With these four sentences I am describing the reasons why we need this
patch. In my previous iteration I had similar  description outside of
git comment and was told to move it into git description. So it is in
comments. Hot sure if I understand what is the problem here. I can
remove bulleted list and make it four sentences. I can remove it
altogether. I can add some text so it will be easier to understand
this without context of previous patch. What would be the best?

I reread relevant parts of
https://github.com/git/git/blob/master/Documentation/SubmittingPatches
and it seems description of the reasons and alternative discarded
solutions fits into description. So could please help me a bit to
understand what is wrong.

> More importantly, I am not sure I understand what these sentences
are trying to say.  "Should be moved to header"---so?  Does that
move something from the source to the header?  It seems to me that
the patch removes a helper function from trace.c but does not add
anything to the header.

I was told to split the patch and do removal for the normalization as
separate commit. As it is separate commit, it needs to do separate
description with the own reasons why etc. Yes it doesn't but it is
needed for the second patch in the series.

So comments explaining that, while implementing trace optimization, I
saw two options:
1. Move normalization function from .c file to .h file
2. Remove it

And why I choose removal - not used, would complicate header without
any benefit, and actually I didn't mention minor benefit of
compilation speed. This trace.h included and used in lots of places so
it will take compiler some time to actually eliminate the code.

> Puzzled.

Does it make more sense now?

Gennady


On 19 November 2017 at 02:19, Junio C Hamano <gits...@pobox.com> wrote:
> gennady.kup...@gmail.com writes:
>
>> Subject: Re: [PATCH 1/2] Simplify tracing code by removing trace key 
>> normalization concept
>
> The usual style comment on the subject applies here.
>
>> From: Gennady Kupava <gkup...@bloomberg.net>
>>
>> - to implement efficient traces with normalization, normalization
>>   implementation should be moved to header. as it seems better to not
>>   overload header file with this normalization logic, suggestion is
>>   just to remove it
>> - different macro exist specifically to handle traces with default key
>> - there is no use of normalization in current code
>> - it could be reintroduced if necessary
>
> I cannot quite tell what it is trying to achive to make it a
> bulleted list.  It's not like four things at the same conceptual
> level is enumerated; instead it just has four sentences that talk
> about random things.
>
> More importantly, I am not sure I understand what these sentences
> are trying to say.  "Should be moved to header"---so?  Does that
> move something from the source to the header?  It seems to me that
> the patch removes a helper function from trace.c but does not add
> anything to the header.
>
> Or am I wasting everybody's time by commenting on a stale comment
> that used to describe an ancient iteration of this code?
>
> Puzzled.

Re: [PATCH 1/2] Simplify tracing code by removing trace key normalization concept

[Junio C Hamano]

gennady.kup...@gmail.com writes:

> Subject: Re: [PATCH 1/2] Simplify tracing code by removing trace key 
> normalization concept

The usual style comment on the subject applies here.

> From: Gennady Kupava <gkup...@bloomberg.net>
>
> - to implement efficient traces with normalization, normalization
>   implementation should be moved to header. as it seems better to not
>   overload header file with this normalization logic, suggestion is
>   just to remove it
> - different macro exist specifically to handle traces with default key
> - there is no use of normalization in current code
> - it could be reintroduced if necessary

I cannot quite tell what it is trying to achive to make it a
bulleted list.  It's not like four things at the same conceptual
level is enumerated; instead it just has four sentences that talk
about random things.

More importantly, I am not sure I understand what these sentences
are trying to say.  "Should be moved to header"---so?  Does that
move something from the source to the header?  It seems to me that
the patch removes a helper function from trace.c but does not add
anything to the header.

Or am I wasting everybody's time by commenting on a stale comment
that used to describe an ancient iteration of this code?

Puzzled.

[PATCH 1/2] Simplify tracing code by removing trace key normalization concept

[gennady . kupava]

From: Gennady Kupava 

- to implement efficient traces with normalization, normalization
  implementation should be moved to header. as it seems better to not
  overload header file with this normalization logic, suggestion is
  just to remove it
- different macro exist specifically to handle traces with default key
- there is no use of normalization in current code
- it could be reintroduced if necessary

Signed-off-by: Gennady Kupava 
---
 trace.c | 24 
 trace.h |  4 +++-
 2 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/trace.c b/trace.c
index cb1293ed3..d47ea28e8 100644
--- a/trace.c
+++ b/trace.c
@@ -24,26 +24,13 @@
 #include "cache.h"
 #include "quote.h"
 
-/*
- * "Normalize" a key argument by converting NULL to our trace_default,
- * and otherwise passing through the value. All caller-facing functions
- * should normalize their inputs in this way, though most get it
- * for free by calling get_trace_fd() (directly or indirectly).
- */
-static void normalize_trace_key(struct trace_key **key)
-{
-   static struct trace_key trace_default = { "GIT_TRACE" };
-   if (!*key)
-   *key = _default;
-}
+struct trace_key trace_default_key = { "GIT_TRACE", 0, 0, 0 };
 
 /* Get a trace file descriptor from "key" env variable. */
 static int get_trace_fd(struct trace_key *key)
 {
const char *trace;
 
-   normalize_trace_key();
-
/* don't open twice */
if (key->initialized)
return key->fd;
@@ -81,8 +68,6 @@ static int get_trace_fd(struct trace_key *key)
 
 void trace_disable(struct trace_key *key)
 {
-   normalize_trace_key();
-
if (key->need_close)
close(key->fd);
key->fd = 0;
@@ -128,7 +113,6 @@ static int prepare_trace_line(const char *file, int line,
 static void trace_write(struct trace_key *key, const void *buf, unsigned len)
 {
if (write_in_full(get_trace_fd(key), buf, len) < 0) {
-   normalize_trace_key();
warning("unable to write trace for %s: %s",
key->key, strerror(errno));
trace_disable(key);
@@ -167,13 +151,13 @@ static void trace_argv_vprintf_fl(const char *file, int 
line,
 {
struct strbuf buf = STRBUF_INIT;
 
-   if (!prepare_trace_line(file, line, NULL, ))
+   if (!prepare_trace_line(file, line, _default_key, ))
return;
 
strbuf_vaddf(, format, ap);
 
sq_quote_argv(, argv, 0);
-   print_trace_line(NULL, );
+   print_trace_line(_default_key, );
 }
 
 void trace_strbuf_fl(const char *file, int line, struct trace_key *key,
@@ -215,7 +199,7 @@ void trace_printf(const char *format, ...)
 {
va_list ap;
va_start(ap, format);
-   trace_vprintf_fl(NULL, 0, NULL, format, ap);
+   trace_vprintf_fl(NULL, 0, _default_key, format, ap);
va_end(ap);
 }
 
diff --git a/trace.h b/trace.h
index 179b249c5..24b32f8f4 100644
--- a/trace.h
+++ b/trace.h
@@ -11,6 +11,8 @@ struct trace_key {
unsigned int  need_close : 1;
 };
 
+extern struct trace_key trace_default_key;
+
 #define TRACE_KEY_INIT(name) { "GIT_TRACE_" #name, 0, 0, 0 }
 
 extern void trace_repo_setup(const char *prefix);
@@ -78,7 +80,7 @@ extern void trace_performance_since(uint64_t start, const 
char *format, ...);
  */
 
 #define trace_printf(...) \
-   trace_printf_key_fl(TRACE_CONTEXT, __LINE__, NULL, __VA_ARGS__)
+   trace_printf_key_fl(TRACE_CONTEXT, __LINE__, _default_key, 
__VA_ARGS__)
 
 #define trace_printf_key(key, ...) \
trace_printf_key_fl(TRACE_CONTEXT, __LINE__, key, __VA_ARGS__)
-- 
2.14.1