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

[ebiederm@xmission.com (Eric W. Biederman)]

Junio C Hamano <junkio@cox.net> writes:

> ebiederm@xmission.com (Eric W. Biederman) writes:
>
>> diff --git a/fetch-pack.c b/fetch-pack.c
>> index a3bcad0..c767d84 100644
>> --- a/fetch-pack.c
>> +++ b/fetch-pack.c
>> @@ -260,6 +260,27 @@ static void mark_recent_complete_commits
>>  	}
>>  }
>>  
>> +static struct ref **get_sha1_heads(struct ref **refs, int nr_heads, char
> **head)
>> +{
>> +	int i;
>> +	for (i  = 0; i < nr_heads; i++) {
>> +		struct ref *ref;
>> +		unsigned char sha1[20];
>> +		char *s = head[i];
>> +		int len = strlen(s);
>> +
>> +		if (len != 40 || get_sha1_hex(s, sha1))
>> +			continue;
>
> 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.

Yes.

>> +
>> +		ref = xcalloc(1, sizeof(*ref) + len + 1);
>> +		memcpy(ref->old_sha1, sha1, 20);
>> +		memcpy(ref->name, s, len + 1);
>> +		*refs = ref;
>> +		refs = &ref->next;
>> +	}
>> +	return refs;
>> +}
>> +
>
> 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.

Ack. That does look like a bug.  I knew there as something
fishy about that code.  But it worked for my basic testing so I didn't
worry about it.

> 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)) {
> 		...

Actually because we want the filter to resolve sha1s by
default in terms of what was passed on the command line.  I'm pretty
certain that should be:

	tail = get_sha1_heads(&ref, nr_match, match);
	tail = get_remote_heads(fd[0], tail, 0, NULL, 0);
        ...


>> @@ -311,6 +332,8 @@ static int everything_local(struct ref *
>>  	if (cutoff)
>>  		mark_recent_complete_commits(cutoff);
>>  
>> +	filter_refs(refs, nr_match, match);
>> +
>
> I am not sure about this change.

Agreed.  It was a hold over from an earlier way of injecting
the sha1 into the logic.  

As for what happens I think I need to audit everything that
takes a ref from fetch_pack.  To make certain I have not
messed up the logic.

> 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.

It feels like it.

> 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.

Sounds sane.  We also introduce a new possibility of having a
ref that is complete but not remote.

> 	/*
> 	 * 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).

Sounds sane.
So that gives me:
	unsigned int force : 1;
	unsigned int injected : 1;

Which aligns them to an int boundary but since we are followed
immediately by a pointer should result in no additional storage being
consumed.

>> @@ -373,6 +394,7 @@ static int fetch_pack(int fd[2], int nr_
>>  		packet_flush(fd[1]);
>>  		die("no matching remote head");
>>  	}
>> +	get_sha1_heads(&ref, nr_match, match);
>
> I talked about this one already...
>
>> diff --git a/git-parse-remote.sh b/git-parse-remote.sh
>> index 187f088..2372df8 100755
>> --- a/git-parse-remote.sh
>> +++ b/git-parse-remote.sh
>> @@ -105,6 +105,7 @@ canon_refs_list_for_fetch () {
>>  		'') remote=HEAD ;;
>>  		refs/heads/* | refs/tags/* | refs/remotes/*) ;;
>>  		heads/* | tags/* | remotes/* ) remote="refs/$remote" ;;
>> +
> [0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F])
> ;;
>
> Yuck.  Don't we have $_x40 somewhere?

I couldn't find one in shell.  

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

I'm not certain why we always add make $remote="refs/heads/$remote" by
default in that switch statement. git-fetch-pack at least doesn't need
it.

If that is true of the other consumers we could easily make the test:
[0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F]*) ;;
Or even simply make the default case *) ;;

But for the moment I will stick to the long form because it is
obviously correct.

Eric