On 1/8/2021 1:17 PM, Taylor Blau wrote:
> -int find_revindex_position(struct packed_git *p, off_t ofs)
> +int offset_to_pack_pos(struct packed_git *p, off_t ofs, uint32_t *pos)
> {
> int lo = 0;
> - int hi = p->num_objects + 1;
> - const struct revindex_entry *revindex = p->revindex;
> + int hi;
> + const struct revindex_entry *revindex;
> +
> + if (load_pack_revindex(p) < 0)
> + return -1;
> +
> + hi = p->num_objects + 1;
> + revindex = p->revindex;
>
> do {
> const unsigned mi = lo + (hi - lo) / 2;
> if (revindex[mi].offset == ofs) {
> - return mi;
> + *pos = mi;
> + return 0;
> } else if (ofs < revindex[mi].offset)
> hi = mi;
> else
> @@ -189,20 +196,6 @@ int find_revindex_position(struct packed_git *p, off_t ofs)
> return -1;
> }
>
> -int offset_to_pack_pos(struct packed_git *p, off_t ofs, uint32_t *pos)
> -{
> - int ret;
> -
> - if (load_pack_revindex(p) < 0)
> - return -1;
> -
> - ret = find_revindex_position(p, ofs);
> - if (ret < 0)
> - return -1;
> - *pos = ret;
> - return 0;
> -}
> -
Not that this is new to the current patch, but this patch made me
wonder if we should initialize *pos = -1 in the case of a failure
to find the position? A correct caller should not use the value
if they are checking for the fail-to-find case properly. But, I
could see someone making a mistake and having trouble diagnosing
the problem because their position variable was initialized to
zero or a previous successful case.
Thanks,
-Stolee
