Re: Tag peeling peculiarities

On 03/14/2013 02:40 PM, Jeff King wrote:

My patch series is nearly done. I will need another day or two to review and make it submission-ready, but I wanted to give you an idea of what I'm up to and I could also use your feedback on some points.

The (preliminary) patch series is available on GitHub:

    https://github.com/mhagger/git/commits/pack-refs-unification

Highlights:

* Move pack-refs code into refs.c

* Implement fully-peeled packed-refs files, as discussed upthread: peel
  references outside of refs/tags, and keep rigorous track of
  which references have REF_KNOWS_PEELED.

* Change how references are iterated over internally (without changing
  the exported API):

  * Provide direct access to the ref_entries

  * Don't set current_ref if not iterating over all refs

* Change pack-refs to use the peeled information from ref_entries if it
  is available, rather than looking up the references again.

* repack_without_ref() writes peeled refs to the packed-refs file.
  Use the old values if available; otherwise look them up.  (The old
  version of repack_without_refs() wrote a packed-refs file without
  any peeled values even if they were present in the old packed-refs
  file.)  Remove the deleted reference from the in-RAM packed ref cache
  in-place instead of discarding the whole cache.

* Add some tests and lots of comments.

Here are my questions for you:

1. There are multiple functions for peeling refs:
   peel_ref() (and peel_object(), which was extracted from it);
   peel_entry() (derived from pack-refs.c:pack_one_ref()).  It would be
   nice to combine these into the One True Function.  But given the
   problem that you mentioned above (which is rooted in parts of the
   code that I don't know) I don't know what that version should do.

2. repack_without_ref() now writes peeled refs, peeling them if
   necessary.  It does this *without* referring to the loose refs
   to avoid having to load all of the loose references, which can be
   time-consuming.  But this means that it might try to lookup SHA1s
   that are not the current value of the corresponding reference any
   more, and might even have been garbage collected.  Is the code that
   I use to do this, in peel_entry(), safe to call for nonexistent
   SHA1s (I would like it to return 0 if the SHA1 is invalid)?:

	o = parse_object(entry->u.value.sha1);
	if (o->type == OBJ_TAG) {
		o = deref_tag(o, entry->name, 0);
		if (o) {
			hashcpy(entry->u.value.peeled, o->sha1);
			entry->flag |= REF_KNOWS_PEELED;
			return 1;
		}
	}
	return 0;

   I've tried to test this experimentally and it seems to work, but I
   would like to have some theoretical support :-)

3. This same change to repack_with_ref() means that it could become
   significantly slower to delete a packed ref if the packed-ref file
   is not fully-peeled.  On the plus side, once this is done once, the
   packed-refs files will be rewritten in fully-peeled form.  But if
   two versions of Git are being used in a repository, this cost could
   be incurred repeatedly.  Does anybody object?

4. Should I change the locking policy as discussed in this thread?:

       http://article.gmane.org/gmane.comp.version-control.git/212131

Michael