patchDo not send "want" lines for complete objects

> +		if (has_sha1_file(remote)
> +				&& parse_object(remote)->flags & COMPLETE) {
> +			if (rev_command_len + 44 < sizeof(rev_command)) {
> +				snprintf(rev_command + rev_command_len, 44,
> +					" ^%s^", sha1_to_hex(remote));
> +				rev_command_len += 43;
> +			}

> Let's draw a single strand of pearl case to illustrate.  You
> have a commit chain A->B->C->D, and the other end says she has
> C.  At this point, telling her that you have C is enough, and by
> not telling her about A and B, you would save her from depreting
> MAX_HAVE slots.  Of course, if the other end has D as another
> head, then the above logic would give "^D^" to rev-list as well,
> telling it not to tell her about C, but that is what we want --
> because she already knows you have C too when you tell her that
> you have D.  I think I like this optimization.

> Junio C Hamano <junkio@cox.net> writes:
> 
> > Let's draw a single strand of pearl case to illustrate.  You
> > have a commit chain A->B->C->D, and the other end says she has
> > C.  At this point, telling her that you have C is enough, and by
> > not telling her about A and B, you would save her from depreting
> > MAX_HAVE slots.  Of course, if the other end has D as another
> > head, then the above logic would give "^D^" to rev-list as well,
> > telling it not to tell her about C, but that is what we want --
> > because she already knows you have C too when you tell her that
> > you have D.  I think I like this optimization.
> 
> This was subtly wrong.  ^D^ would barf if D is a tag that points
> at a non commit (refs/tags/v2.6.11-tree).  Also it would do a
> suboptimal thing for a merge commit, since it will not cull the
> second and later parents.

> +		    ((commit = (struct commit *) deref_tag(o)) != NULL) &&
> +		    (commit->object.type = commit_type)) {
> +			struct commit_list *p = commit->parents;
> +			while (p && rev_command_len + 44 < sizeof(rev_command)) {
>  				snprintf(rev_command + rev_command_len, 44,
> -					" ^%s^", sha1_to_hex(remote));
> +					 " ^%s",
> +					 sha1_to_hex(p->item->object.sha1));
>  				rev_command_len += 43;
> +				p = p->next;
>  			}

> -
> -			continue;
> +			if (!p)
> +				continue;
>  		}
>  
>  		packet_write(fd[1], "want %s\n", sha1_to_hex(remote));
> 

> Hi,
>
>> > +		    ((o = parse_object(remote)) != NULL) &&
>> > +		    (o->flags & COMPLETE) &&
>
> I just realized that parse_object() always reads the file, then does a 
> lookup (which makes the above code work), and then parses the file. It 
> always does all of these steps, even if the object was already parsed. Any 
> reason for this?

> Hi,
> 
> > > +		    ((o = parse_object(remote)) != NULL) &&
> > > +		    (o->flags & COMPLETE) &&
> 
> I just realized that parse_object() always reads the file, then does a 
> lookup (which makes the above code work), and then parses the file. It 
> always does all of these steps, even if the object was already parsed. Any 
> reason for this?

> On Thu, 20 Oct 2005, Johannes Schindelin wrote:
> 
> > Hi,
> > 
> > > > +		    ((o = parse_object(remote)) != NULL) &&
> > > > +		    (o->flags & COMPLETE) &&
> > 
> > I just realized that parse_object() always reads the file, then does a 
> > lookup (which makes the above code work), and then parses the file. It 
> > always does all of these steps, even if the object was already parsed. Any 
> > reason for this?
> 
> I'm lazy and haven't sent in a patch to clean that up. There's no reason 
> it couldn't check whether the value it gets is already parsed.

> ... I did not use it originally, because lines 
> 24-25 of commit.h say
> 
> 	/** Internal only **/
> 	struct object *lookup_object(const unsigned char *sha1);
> 
> Is this obsolete?

> Hi,
> 
> On Thu, 20 Oct 2005, Daniel Barkalow wrote:
> 
> > On Thu, 20 Oct 2005, Johannes Schindelin wrote:
> > 
> > > Hi,
> > > 
> > > > > +		    ((o = parse_object(remote)) != NULL) &&
> > > > > +		    (o->flags & COMPLETE) &&
> > > 
> > > I just realized that parse_object() always reads the file, then does a 
> > > lookup (which makes the above code work), and then parses the file. It 
> > > always does all of these steps, even if the object was already parsed. Any 
> > > reason for this?
> > 
> > I'm lazy and haven't sent in a patch to clean that up. There's no reason 
> > it couldn't check whether the value it gets is already parsed.
> 
> Actually, you don't have to... Junio already replaced parse_object() by 
> lookup_object() in this case.

> I did not use it originally, because lines 24-25 of commit.h say
> 
> 	/** Internal only **/
> 	struct object *lookup_object(const unsigned char *sha1);
> 
> Is this obsolete?

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Also, git-fetch-pack does not have to ask for descendants of remote refs 
> > which are complete (for now, git-rev-list is told to ignore only the first 
> > parent).
> 
> I should interpret what you said with s/descendant/ancestor/
> applied, I think.  I would like to make sure I understand what
> you are doing here.

> > +		if (has_sha1_file(remote)
> > +				&& parse_object(remote)->flags & COMPLETE) {
> > +			if (rev_command_len + 44 < sizeof(rev_command)) {
> > +				snprintf(rev_command + rev_command_len, 44,
> > +					" ^%s^", sha1_to_hex(remote));
> > +				rev_command_len += 43;
> > +			}
> 
> This rev-list command is to generate the list of "have", and we
> learned that the other side says she has remote -- we choose not
> to tell her that we have ancestors of it, but we do tell her
> about the remote head itself.
> 
> Let's draw a single strand of pearl case to illustrate.  You
> have a commit chain A->B->C->D, and the other end says she has
> C.  At this point, telling her that you have C is enough, and by
> not telling her about A and B, you would save her from depreting
> MAX_HAVE slots.  Of course, if the other end has D as another
> head, then the above logic would give "^D^" to rev-list as well,
> telling it not to tell her about C, but that is what we want --
> because she already knows you have C too when you tell her that
> you have D.  I think I like this optimization.
> 
> One thing that might help, when we are telling the other end
> about what we have, is an output ordering option to get-rev-list
> that shows not in chronological order, but in the order of
> distance from the tip.  That may give the other end a better
> chance to find the latest (in commit order) common commit in
> each branch without running out is MAX_HAS buffer.