Kirill Smelkov <kirr@xxxxxxxxxx> writes:
> +static int want_found_object(int exclude, struct packed_git *p)
> +{
> + if (exclude)
> + return 1;
> + if (incremental)
> + return 0;
> +
> + /*
> + * When asked to do --local (do not include an object that appears in a
> + * pack we borrow from elsewhere) or --honor-pack-keep (do not include
> + * an object that appears in a pack marked with .keep), finding a pack
> + * that matches the criteria is sufficient for us to decide to omit it.
> + * However, even if this pack does not satisfy the criteria, we need to
> + * make sure no copy of this object appears in _any_ pack that makes us
> + * to omit the object, so we need to check all the packs.
> + *
> + * We can however first check whether these options can possible matter;
> + * if they do not matter we know we want the object in generated pack.
> + * Otherwise, we signal "-1" at the end to tell the caller that we do
> + * not know either way, and it needs to check more packs.
> + */
> + if (!ignore_packed_keep &&
> + (!local || !have_non_local_packs))
> + return 1;
> +
> + if (local && !p->pack_local)
> + return 0;
> + if (ignore_packed_keep && p->pack_local && p->pack_keep)
> + return 0;
> +
> + /* we don't know yet; keep looking for more packs */
> + return -1;
> +}
Moving this logic out to this helper made the main logic in the
caller easier to grasp.
> @@ -958,15 +993,30 @@ static int want_object_in_pack(const unsigned char *sha1,
> off_t *found_offset)
> {
> struct packed_git *p;
> + int want;
>
> if (!exclude && local && has_loose_object_nonlocal(sha1))
> return 0;
>
> + /*
> + * If we already know the pack object lives in, start checks from that
> + * pack - in the usual case when neither --local was given nor .keep files
> + * are present we will determine the answer right now.
> + */
> + if (*found_pack) {
> + want = want_found_object(exclude, *found_pack);
> + if (want != -1)
> + return want;
> + }
>
> for (p = packed_git; p; p = p->next) {
> + off_t offset;
> +
> + if (p == *found_pack)
> + offset = *found_offset;
> + else
> + offset = find_pack_entry_one(sha1, p);
> +
> if (offset) {
> if (!*found_pack) {
> if (!is_pack_valid(p))
> @@ -974,31 +1024,9 @@ static int want_object_in_pack(const unsigned char *sha1,
> *found_offset = offset;
> *found_pack = p;
> }
> + want = want_found_object(exclude, p);
> + if (want != -1)
> + return want;
> }
> }
As Peff noted in his earlier review, however, MRU code needed to be
grafted in to the caller (an update to the MRU list was done in the
code that was moved to the want_found_object() helper). I think I
did it correctly, which ended up looking like this:
want = want_found_object(exclude, p);
if (!exclude && want > 0)
mru_mark(packed_git_mru, entry);
if (want != -1)
return want;
I somewhat feel that it is ugly that the helper knows about exclude
(i.e. in the original code, we immediately returned 1 without
futzing with the MRU when we find an entry that is to be excluded,
which now is done in the helper), and the caller also knows about
exclude (i.e. the caller knows that the helper may return positive
in two cases, it knows that MRU marking needs to happen only one of
the two cases, and it also knows that "exclude" is what
differentiates between the two cases) at the same time.
But probably the reason why I feel it ugly is only because I knew
how the original looked like. I dunno.