Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths

> I mean that later in the process, if we need to find an object we may
> open the .idx file to look for it. So by opening them all up front, we
> _might_ just be doing work that would get done later.
>
> But it's not guaranteed. Imagine you have 10,000 small packs, and one
> big bitmapped pack. If you can serve the request from just the big pack,
> then you'd never need to open those other .idx files at all. However,
> the current code will open them anyway.
>
> I care less about mmap space, and more that it's work (syscalls, and
> examining the contents of the idx) to open each one. It's probably not
> even measurable unless you have a ton of packs, though.
>
> > > So it may not be worth worrying about. It does seem like it would be
> > > easy to reorder open_pack_bitmap_1() to look for a bitmap file first and
> > > only open the idx if it finds something.
> >
> > I think it may be worthy if we have lots of packs and the bitmap is refer to
> > an older one, but I didn't make the test. At least, the scenario is common, I
> > agree with that, so maybe we could shuffle the sort order in "open_pack_bitmap()".
>
> I don't mean the order in which we look at packs. I mean the order of
> operations in open_pack_bitmap_1(), something like:

> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 440407f1be..1df2f6c8b6 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -411,9 +411,6 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
>  	struct stat st;
>  	char *bitmap_name;
>
> -	if (open_pack_index(packfile))
> -		return -1;
> -
>  	bitmap_name = pack_bitmap_filename(packfile);
>  	fd = git_open(bitmap_name);
>
> @@ -438,6 +435,10 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
>  		return -1;
>  	}
>
> +	/* now we know we have a plausible bitmap; make sure the idx is OK, too */
> +	if (open_pack_index(packfile))
> +		return -1;
> +
>  	if (!is_pack_valid(packfile)) {
>  		close(fd);
>  		return -1;
>
> But we can further observe that the first thing is_pack_valid() will do
> is open the idx file. :) So we can really just drop this line entirely,
> I'd think.

> BTW, another oddity I noticed in this function. We check:
>
>    if (bitmap_git->pack || bitmap_git->midx) {
> 	   /* ignore extra bitmap file; we can only handle one */
> 	   ...
>    }
>
> but it's impossible for bitmap_git->midx to be set here. If we opened
> the midx bitmap, we'll skip calling open_pack_bitmap() entirely.