| Message ID | 2acaa3f097e0ab08a63ae1719454f5e11bb15a44.1669032426.git.dyroneteng@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pack-bitmap.c: avoid exposing absolute paths | expand |
On Mon, Nov 21, 2022 at 08:16:15PM +0800, Teng Long wrote: > From: Jeff King <peff@peff.net> > > We retained pack bitmaps as a quick recovery mechanism while > test-deploying midx bitmaps. This is an internal mechanism, and we > want to expose this rule externally through trace2 so that end users, > repo-maintainers, and debuggers know what is happening in the process. Re-reading this outside the context of the earlier thread, I think "this rule" is a little vague. Maybe: When we find a midx bitmap, we do not bother checking for pack bitmaps, since we can use only one. But since we will warn of unused bitmaps via trace2, let's continue looking for pack bitmaps when tracing is enabled. > @@ -559,11 +557,20 @@ static int open_midx_bitmap(struct repository *r, > static int open_bitmap(struct repository *r, > struct bitmap_index *bitmap_git) > { > + int found = 0; > + > assert(!bitmap_git->map); > > - if (!open_midx_bitmap(r, bitmap_git)) > - return 0; > - return open_pack_bitmap(r, bitmap_git); > + found = !open_midx_bitmap(r, bitmap_git); I think we can drop the initialization of "found = 0"; that value is unused, since we'll always assign to it (I think my initial attempt had setting it to 1 inside the conditional). It's not hurting anything functionally, but it makes the code slightly more confusing to read. -Peff
Jeff King <peff@peff.net> writes: > ... > It's not hurting anything functionally, but it makes the code slightly > more confusing to read. Thanks for all these good suggestions. As you pointed out, the first two seems to be identical to what Taylor queued in 'next' already (so they do not have to be re-sent but it is not a huge deal if they did---it would be a huge deal if they were rewritten, though), and both of the two follow-on patches make sense with suggested (minor) updates. I'll expect a reroll before queuing these two and mark the topic in 'next' to be waiting for them. Thanks, both.
Junio C Hamano <gitster@pobox.com> write: > Thanks for all these good suggestions. > > As you pointed out, the first two seems to be identical to what > Taylor queued in 'next' already (so they do not have to be re-sent > but it is not a huge deal if they did---it would be a huge deal if > they were rewritten, though), and both of the two follow-on patches > make sense with suggested (minor) updates. I'll expect a reroll > before queuing these two and mark the topic in 'next' to be waiting > for them. Sorry, Not online for a few days last week. Will send a reroll today. Thanks.
Jeff King <peff@peff.net> writes: > > From: Jeff King <peff@peff.net> > > > > We retained pack bitmaps as a quick recovery mechanism while > > test-deploying midx bitmaps. This is an internal mechanism, and we > > want to expose this rule externally through trace2 so that end users, > > repo-maintainers, and debuggers know what is happening in the process. > > Re-reading this outside the context of the earlier thread, I think "this > rule" is a little vague. Maybe: > > When we find a midx bitmap, we do not bother checking for pack > bitmaps, since we can use only one. But since we will warn of unused > bitmaps via trace2, let's continue looking for pack bitmaps when > tracing is enabled. Thanks. That's more clear and will be applies on next patch. > > @@ -559,11 +557,20 @@ static int open_midx_bitmap(struct repository *r, > > static int open_bitmap(struct repository *r, > > struct bitmap_index *bitmap_git) > > { > > + int found = 0; > > + > > assert(!bitmap_git->map); > > > > - if (!open_midx_bitmap(r, bitmap_git)) > > - return 0; > > - return open_pack_bitmap(r, bitmap_git); > > + found = !open_midx_bitmap(r, bitmap_git); > > I think we can drop the initialization of "found = 0"; that value is > unused, since we'll always assign to it (I think my initial attempt had > setting it to 1 inside the conditional). > > It's not hurting anything functionally, but it makes the code slightly > more confusing to read. I agree it's not hurting here, it's OK for me to make the improvement here. But I have a question, do we prefer to omit the initialization in such scenarios if we make sure it will initialized correctly？ Thanks.
On Mon, Nov 28, 2022 at 08:37:40PM +0800, Teng Long wrote: > > > @@ -559,11 +557,20 @@ static int open_midx_bitmap(struct repository *r, > > > static int open_bitmap(struct repository *r, > > > struct bitmap_index *bitmap_git) > > > { > > > + int found = 0; > > > + > > > assert(!bitmap_git->map); > > > > > > - if (!open_midx_bitmap(r, bitmap_git)) > > > - return 0; > > > - return open_pack_bitmap(r, bitmap_git); > > > + found = !open_midx_bitmap(r, bitmap_git); > > > > I think we can drop the initialization of "found = 0"; that value is > > unused, since we'll always assign to it (I think my initial attempt had > > setting it to 1 inside the conditional). > > > > It's not hurting anything functionally, but it makes the code slightly > > more confusing to read. > > I agree it's not hurting here, it's OK for me to make the improvement > here. But I have a question, do we prefer to omit the initialization > in such scenarios if we make sure it will initialized correctly？ I don't know that we have a particular rule here, but I would say that yes, if you know the initialization isn't used, then skip it. That communicates the expectation to anybody reading the code. And if you're wrong, the compiler will generally notice. -Peff
Jeff King <peff@peff.net> writes: > > I agree it's not hurting here, it's OK for me to make the improvement > > here. But I have a question, do we prefer to omit the initialization > > in such scenarios if we make sure it will initialized correctly？ > > I don't know that we have a particular rule here, but I would say that > yes, if you know the initialization isn't used, then skip it. That > communicates the expectation to anybody reading the code. And if you're > wrong, the compiler will generally notice. Thanks for the explanation!
diff --git a/pack-bitmap.c b/pack-bitmap.c index 3b6c2f804a..44a80ed8f2 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -524,8 +524,6 @@ static int open_pack_bitmap(struct repository *r, struct packed_git *p; int ret = -1; - assert(!bitmap_git->map); - for (p = get_all_packs(r); p; p = p->next) { if (open_pack_bitmap_1(bitmap_git, p) == 0) { ret = 0; @@ -559,11 +557,20 @@ static int open_midx_bitmap(struct repository *r, static int open_bitmap(struct repository *r, struct bitmap_index *bitmap_git) { + int found = 0; + assert(!bitmap_git->map); - if (!open_midx_bitmap(r, bitmap_git)) - return 0; - return open_pack_bitmap(r, bitmap_git); + found = !open_midx_bitmap(r, bitmap_git); + + /* + * these will all be skipped if we opened a midx bitmap; but run it + * anyway if tracing is enabled to report the duplicates + */ + if (!found || trace2_is_enabled()) + found |= !open_pack_bitmap(r, bitmap_git); + + return found ? 0 : -1; } struct bitmap_index *prepare_bitmap_git(struct repository *r)