On Mon, Jul 18 2022, Junio C Hamano wrote:
> Teng Long <dyroneteng@xxxxxxxxx> 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.
