Re: [PATCH 1/2] pack-objects: Teach --use-bitmap-index codepath to respect --local, --honor-pack-keep and --incremental

On Mon, Aug 08, 2016 at 03:37:35PM +0300, Kirill Smelkov wrote:

Hmm. So this is basically sticking the found-pack at the front of the loop.

We either need to look at zero packs here (we already know where the object is, and we don't need to bother with --local or .keep lookups), or we need to look at all of them (to check for local/keep).

I guess you structured it this way to try to reuse the "can we break out early" logic from the middle of the loop. So we go through the loop one time, and then break out. And then this:

is meant to make that one-time through the loop cheaper. So I don't think it's wrong, but it's very confusing to me.

Would it be simpler to stick that logic in a function like:

  static int want_found_object(int exclude, struct packed_git *pack)
  {
	if (exclude)
		return 1;
	if (incremental)
		return 0;

	/* if we can break early, then do so */
	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;

	/* indeterminate; keep looking for more packs */
	return -1;
  }

  static int want_object_in_pack(...)
  {
	...
	if (!exclude && local && has_loose_object_nonlocal(sha1))
		return 0;

	if (*found_pack) {
		int ret = want_found_object(exclude, *found_pack);
		if (ret != -1)
			return ret;
	}

	for (p = packed_git; p; p = p->next) {
		off_t offset;

		if (p == *found_pack)
			offset = *found_offset;
		else
			offset = find_pack_entry(sha1, p);
		if (offset) {
			... fill in *found_pack ...
			int ret = want_found_object(exclude, p);
			if (ret != -1)
				return ret;
		}
	}
	return 1;
  }

That's a little more verbose, but IMHO the flow is a lot easier to follow (especially as the later re-rolls of that series actually muck with the loop order more, but with this approach there's no conflict).

This part looks correct and easy to understand.

Our usual method for getting a sha1 is "git rev-parse". I don't think there's anything wrong with your method, but it might be better to stick to the canonical one (I had to actually look up "show-ref -s").

Style: we don't put a space between ">" and the filename.

I'm not sure why we need two objects in the fake alt_objects repository. Shouldn't one be enough to do the test?

I think this will actually skip the writing of the loose object, because it's already available in the alternate object store. You probably want to do this before adding it there.

No need for "\" after a "|"; the shell knows it has to keep looking.

I'd have expected you to use the non-stdout version here. Is this meant to be independent of your other patch (I think that's OK).

It's cheaper to use "git show-index <1.pack", and the output is saner, too.

"echo -e" isn't portable. You can use "printf", or two echos like:

  {
    echo one &&
    echo two
  } >file

(though I'm still not sure what we gain by checking both).

grep -f isn't portable. However, I think:

  echo $objsha1 >expect &&
  git show-index <1.pack | cut -d' ' -f2 >actual
  test_cmp expect actual

would work (if you do stick with two entries, you might need to sort your "expect").

I think similar comments apply to the other tests. I would have expected "respects --local (non-local pack)" to come next (i.e., to keep all of the --local tests together). But you seem to interleave them with --honor-pack-keep.

-Peff