Re: [RFC][PATCH] Allow transfer of any valid sha1

ebiederm@xmission.com (Eric W. Biederman) writes:

So the new convention is fetch-pack can take ref name (as before), or a bare 40-byte hexadecimal. I think sane people would not use ambiguous refname that says "deadbeef" five times, and even if the do so they could disambiguate by explicitly saying "refs/heads/" followed by "deadbeef" five times, so it should be OK.

This function takes the pointer to a location that holds a pointer to a "struct ref" -- it is the location to store the newly allocated ref structure, i.e. the next pointer of the last element in the list. When it returns, the location pointed at by the pointer given to you points at the first element you allocated, and it returns the next pointer of the last element allocated by it. That is the same calling convention as connect.c::get_remote_heads(). So when calling this function to append to a list you already have, you would give the next pointer to the last element of the existing list. But you do not seem to do that.

I think the body of fetch_pack() should become something like:

	struct ref *ref, **tail;

        tail = get_remote_heads(fd[0], &ref, 0, NULL, 0);
	if (server_supports("multi_ack")) {
		...
	}
	tail = get_sha1_heads(tail, nr_match, match);
	if (everything_local(&ref, nr_match, match)) {
		...

I am not sure about this change.

In the original code we do not let get_remote_heads() to filter the refs but call filter_refs() after the "mark all complete remote refs as common" step for a reason. Even though we may not be fetching from some remote refs, we would want to take advantage of the knowledge of what objects they have so that we can mark as many objects as common as possible in the early stage. I suspect this change defeats that optimization.

So instead I would teach "mark all complete remote refs" loop that not everything in refs list is a valid remote ref, and skip what get_sha1_heads() injected, because these arbitrary ones we got from the command line are not something we know exist on the remote side. Maybe something like this.

	/*
	 * Mark all complete remote refs as common refs.
	 * Don't mark them common yet; the server has to be told so first.
	 */
	for (ref = *refs; ref; ref = ref->next) {
		struct object *o;
                if (ref is SHA1 from the command line)
                	continue;
		o = deref_tag(lookup_object(ref->old_sha1), NULL, 0);
		if (!o || o->type != commit_type || !(o->flags & COMPLETE))
			continue;
		...

To implement "ref is SHA1 from the command line", I would add another 1-bit field to "struct ref" and mark the new ones you create in get_sha1_heads() as such (existing "force" field could also become an 1-bit field -- we do not neeed a char).

I talked about this one already...

Yuck.  Don't we have $_x40 somewhere?

We never use uppercase so at least we could save 24 columns from here ;-).