Re: [PATCH v7 0/7] trace2: dump scope when print "interesting" config

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Teng Long <dyroneteng@gmail.com>,
	derrickstolee@github.com, git@jeffhostetler.com,
	git@vger.kernel.org, me@ttaylorr.com,
	tenglong.tl@alibaba-inc.com
Subject: Re: [PATCH v7 0/7] trace2: dump scope when print "interesting" config
Date: Mon, 18 Jul 2022 21:07:30 +0200	[thread overview]
Message-ID: <220718.86edyiw82r.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <xmqqa696fdva.fsf@gitster.g>


On Mon, Jul 18 2022, Junio C Hamano wrote:

> Teng Long <dyroneteng@gmail.com> writes:
>
>> Changes since v6:
>>
>> 1. [1/7] Fixed the mistitled commit mesage.
>>
>>    [PATCH v6 1/7] "clean: fixed issues related to text output format"
>>
>>    to:
>>
>>    [PATCH v7 1/7] "pack-bitmap.c: fix formatting of error messages"
>>
>> 2. [4/7] replace "warning()" to "warning_errno()" and rewrite commit message.
>>
>> 3. [5/7] fix the logic error, move "error_errno()" before close(fd) to
>>    avoid errno lost.
>>
>> 4. [7/7] update Documentation/technical/api-trace2.txt here too.
>>
>> Thanks.
>>
>> Teng Long (7):
>>   pack-bitmap.c: fix formatting of error messages
>>   pack-bitmap.c: mark more strings for translations
>>   pack-bitmap.c: rename "idx_name" to "bitmap_name"
>>   pack-bitmap.c: do not ignore error when opening a bitmap file
>>   pack-bitmap.c: using error() instead of silently returning -1
>>   pack-bitmap.c: continue looping when first MIDX bitmap is found
>>   tr2: dump names if config exist in multiple scopes
>
> This has been "cooking" on the list for quite some time and I found
> that all parts that I had comments on earlier iterations are now in
> good shape.
>
> Fellow reviewers, how does this round look?  The only gripe I have
> is that the last one seems totally disconnected from the rest, but
> That's minor.

Yeah likewise, it even applies directly on master. But I can live with
it :)

One minor nit is that something like this (which needs to be fleshened
up) should be fixed up into 7/7 (and maybe we want to keep the "..."?):

diff --git a/Documentation/technical/api-trace2.txt b/Documentation/technical/api-trace2.txt
index 49bb1ca1924..ce544982a37 100644
--- a/Documentation/technical/api-trace2.txt
+++ b/Documentation/technical/api-trace2.txt
@@ -716,7 +716,7 @@ The "exec_id" field is a command-unique id and is only useful if the
 ------------
 {
 	"event":"def_param",
-	...
+	"scope": ...
 	"param":"core.abbrev",
 	"value":"7"
 }

And that the addition to api-trace2.txt seems to partially be something
that should just link to Documentation/config/trace2.txt, i.e. it's
generally documenting an existing facility.

I think it would be great in any case to have that 7/7 split into what
we do now & docs for that, and then the minor addition of "scope".

The rest all looks good to me, and seems to fully address the feedback
in previous rounds.
