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.
