Re: [PATCH 4/8] Allow fetch to modify refs

From: Daniel Barkalow <barkalow@iabervon.org>
Date: Sep 4, 2009, 16:29 UTC
Message-ID: <alpine.LNX.2.00.0909041225080.28290@iabervon.org>
In-Reply-To: <alpine.DEB.1.00.0909041243420.4605@intel-tinevez-2-302>

On Fri, 4 Sep 2009, Johannes Schindelin wrote:

Show 10 quoted lines

> Hi,
> 
> On Thu, 3 Sep 2009, Daniel Barkalow wrote:
> 
> > +	/**
> > +	 * Fetch the objects for the given refs. Note that this gets
> > +	 * an array, and should ignore the list structure.
> 
> This is not clear at all.  You should rather say "[...] and should not 
> look at, or set, the 'next' member of the refs".

That is a better wording, yes.

Show 6 quoted lines

> > +	 *
> > +	 * If the transport did not get hashes for refs in
> > +	 * get_refs_list(), it should set the old_sha1 fields in the
> > +	 * provided refs now.
> 
> Not the "new_sha1"?

No, because get_refs_list() sets the old_sha1, and this isn't indicating anything different. The old/new thing is to indicate that the ref is changing value. What's happening here is that the ref isn't changing value but we didn't know what value it always (effectively) had until now.

Show 7 quoted lines

> > +	 **/
> > +	int (*fetch)(struct transport *transport, int refs_nr, struct ref **refs);
> > +
> > [...]
> > +	/** get_refs_list(), fetch(), and push_refs() can keep
> 
> The "/**" wants to have a line to itself.

Good point, thanks.

	-Daniel
*This .sig left intentionally blank*