Re: [PATCH v7 0/7] trace2: dump scope when print "interesting" config
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jul 18, 2022, 19:07 UTC
- Message-ID
- <220718.86edyiw82r.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <xmqqa696fdva.fsf@gitster.g>
On Mon, Jul 18 2022, Junio C Hamano wrote:
Show 37 quoted lines
> 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.