Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths
- From
Teng Long <dyroneteng@gmail.com>
- Date
- Oct 31, 2022, 13:13 UTC
- Message-ID
- <20221031131356.67007-1-dyroneteng@gmail.com>
- In-Reply-To
- <Y1mp23NHB0qzKsPR@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
> --- >8 ---
What does ">8" means?
It's like a Github internal patch between the ">8" and "<8" marks, right?
> ...and as the patch message notes, this is done mostly to prevent > confusion when racily fetching from GitHub, or due to some > misconfiguration.
> And in that instance, I think the patch from Peff is right. If there is > a legitimate bug, we'd see it elsewhere and have sufficiently powerful > tools to investigate it. But the warning is useless and confusing to > users who don't have access to such tools.
Yes, currently there will be only one bitmap to be process and other ones will be skipped, so we should remove it I think because it's no actual meanings for client uses, they do not care about what the server skipped and hope to avoid the disturbing information.
> For the general case of what ships in git.git, I *do* find this warning > ...
"ships in git.git" means as the git developers, right?
> useful. It's helpful when hacking on pack-bitmap.c to know if you've > messed up, and it's useful to see the filename of the affected > bitmap(s).
I agree, let things expose entirely, but it's a trade-off.
Show 7 quoted lines
> You could also imagine adding a configuration knob here to control > whether or not the warning is shown, but I find that to be kind of > gross. > > So I think that the warning is--in general--too useful to consider > removing it entirely, and that at most we should consider just emitting > the basename of the pack, but nothing else.
I think it's no need for a new config knob here, I agree with you on this point.
And keep the pack basename seems like definitly a better solution than the current one and I have a other solution which is to replace "warn()" to a trace2 api, so the developer of git or ships in git.git if I understand it right ;-) could get the warning in trace2 output by:
➜ pack git:(master) GIT_TRACE2_PERF=1 git rev-list --test-bitmap HEAD <<< 21:04:08.702523 common-main.c:49 | d0 | main | version | | | | | 2.38.0 21:04:08.702791 common-main.c:50 | d0 | main | start | | 0.001922 | | | /usr/local/bin/git rev-list --test-bitmap HEAD 21:04:08.703553 git.c:461 | d0 | main | cmd_name | | | | | rev-list (rev-list) 21:04:08.704889 usage.c:86 | d0 | main | error | | | | | ignoring extra bitmap file: '/Users/tenglong.tl/Downloads/trace-test/.git/objects/pack/pack-3cea516b416961285fd8f519e12102b19bcf257e.pack' warning: ignoring extra bitmap file: '/Users/tenglong.tl/Downloads/trace-test/.git/objects/pack/pack-3cea516b416961285fd8f519e12102b19bcf257e.pack' Bitmap v1 test (2 entries loaded) Found bitmap for '2c5959955b5e6167181d08eeb30ee4099b4a4c5b'. 64 bits / ca44d5df checksum 21:04:08.705305 progress.c:268 | d0 | main | region_enter | r0 | 0.004448 | | progress | label:Verifying bitmap entries Verifying bitmap entries: 100% (6/6), done. 21:04:08.705814 progress.c:340 | d0 | main | data | r0 | 0.004955 | 0.000507 | progress | ..total_objects:6 21:04:08.705829 progress.c:346 | d0 | main | region_leave | r0 | 0.004972 | 0.000524 | progress | label:Verifying bitmap entries OK! 21:04:08.705883 git.c:721 | d0 | main | exit | | 0.005026 | | | code:0 21:04:08.705899 trace2/tr2_tgt_perf.c:215 | d0 | main | atexit | | 0.005043 | | | code:0
I don't know which is better and I will go on reading the left replies.
Thanks for the hard working on this patchset, Taylor, really appreciate for that.