Re: [PATCH v3 5/5] bitmap: add trace2 outputs during open "bitmap" file

From: Teng Long <dyroneteng@gmail.com>
To: me@ttaylorr.com
Cc: avarab@gmail.com, derrickstolee@github.com, dyroneteng@gmail.com,
	git@vger.kernel.org, gitster@pobox.com,
	tenglong.tl@alibaba-inc.com
Subject: Re: [PATCH v3 5/5] bitmap: add trace2 outputs during open "bitmap" file
Date: Tue, 21 Jun 2022 14:58:53 +0800	[thread overview]
Message-ID: <20220621065853.126718-1-dyroneteng@gmail.com> (raw)
In-Reply-To: <Yqfm+rsP5DWyj95L@nand.local>

On Mon, 13 Jun 2022 21:40:10 -0400, Taylor Blau wrote:

> I'm nitpicking, but I usually say "single-pack bitmap" or "multi-pack
> (MIDX) bitmap" when distinguishing between the two.

Ha, it's OK, I'm hungry. Your description is a better way I think.

Will optimize the relevant places.

> Odd spacing, and there should be a single space character separating "$"
> from "git" (like "$ git").

Yeah, will Fix.

> While I'm thinking about it: is the error message here up-to-date with
> the changes made by the previous patch?

It's still the same after the remake and test (actually it's just a case
not all the cases have the same error message. now by the previous path
if "core.multipackindex=false" and single-pack doesn't exist, the error
message will look like this).

> This could probably be trimmed down for brevity, but I don't feel
> strongly about it. If you wanted to make your commit message a tad
> shorter, perhaps something like:
> 
>   When a user sees output like this, it's unclear which kind(s) of
>   .bitmap exist, and which were read. For example, it's possible a MIDX
>   bitmap exists, but was not read (e.g., because
>   core.multiPackIndex=false), among many other scenarios.
> 
> would suffice.

I think yours is better, I will use it instead the listed way.

> s/TRACE2/trace2

Will fix.

> s/NORMAL/pack

Will fix as s/NORMAL/single-pack.

> s/$GIT/$ GIT

Will fix.

> The output here is quite wide, and I wonder if this whole section could
> be shortened. For example, scenario 2 is arguably more interesting than
> scenario 1 (I think readers would reasonably infer what happens in
> scenario 1 by reading what happens in scenario 2).
> ...
> And scenario 2 could be cleaned up by just showing a few of the columns
> from the trace2 output. Perhaps along the lines of:
> ...
> Reading the below scenarios, I think just showing this example is more
> than sufficient for illustrating your point.

That makes sense. I'll keep two scenarios and omit some of the output to
keep it shorter in next patch.

> I wonder, why don't we show these errors to the user? Should we
> introduce a "ret" variable and set it to (for e.g.)
> 
>     ret = error(_("failed to load bitmap header"));
> 
> or something?

Just this "load header error", I think it's unnecessary to show more
errors to uses because in "load_bitmap_header()", the function already
does, but in my patch, it's unnecessary to add "trace2_data_string()"
too, so here I will remove it (directly "goto cleanup" maybe) in
next patch.

> I'm not sure we want to dump the whole repository configuration into
> trace2 output. Is there a more convenient place to log any important
> value(s) _after_ they have been read?

I think maybe I can add a argument like "int trace" in next patch, so
the caller can decide whether to print to trace or not.

Thanks for your help, Taylor.