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

From: Gennady Kupava <gennady.kupava@gmail.com>
Date: Nov 19, 2017, 13:16 UTC
Message-ID: <CAPu-DQoapb=cgxYEOEVcpZ4nQeh+FuOE6VF=m3NaqRcr2p8Nrg@mail.gmail.com>
In-Reply-To: <xmqqzi7jnh6a.fsf@gitster.mtv.corp.google.com>

> 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 <gitster@pobox.com> wrote:

Show 31 quoted lines

> gennady.kupava@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 <gkupava@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.