Re: [PATCH 4/4] fetch: opportunistically update tracking refs

[Jeff King <peff@peff.net>]

On Tue, Aug 06, 2013 at 09:28:28AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > @@ -170,6 +172,20 @@ static struct ref *get_ref_map(struct transport *transport,
> >  			rm->fetch_head_status = FETCH_HEAD_MERGE;
> >  		if (tags == TAGS_SET)
> >  			get_fetch_map(remote_refs, tag_refspec, &tail, 0);
> > +
> > +		/*
> > +		 * For any refs that we happen to be fetching via command-line
> > +		 * arguments, take the opportunity to update their configured
> > +		 * counterparts. However, we do not want to mention these
> > +		 * entries in FETCH_HEAD at all, as they would simply be
> > +		 * duplicates of existing entries.
> > +		 */
> > +		old_tail = tail;
> > +		for (i = 0; i < transport->remote->fetch_refspec_nr; i++)
> > +			get_fetch_map(ref_map, &transport->remote->fetch[i],
> > +				      &tail, 0);
> > +		for (rm = *old_tail; rm; rm = rm->next)
> > +			rm->fetch_head_status = FETCH_HEAD_IGNORE;
> 
> Was there a reason why this change was done by appending new ref at
> the tail of the ref_map list?  I would have expected that a naïve
> and obvious implementation would be to iterate existing refs over
> ref_map to find refs with an empty RHS whose LHS is configured to
> usually store the fetched result to somewhere and to update their
> RHS in place.
> 
> Being curious.

Two reasons:

  1. The implementation you suggest above behaves differently than the
     current code. It does not look for refspecs with an empty RHS. It
     looks for any LHS that matches our configured entries. So if you do
     "git fetch origin master:foobar", we will update both
     "refs/heads/foobar" as well as "refs/remotes/origin/master".

     That means it is purely an opportunistic "hey, during another
     operation we happened to find out something new about
     origin/master, so let's update our tracking branch". Whereas what
     you stated above is more like "we are fetching into FETCH_HEAD, so
     let's also update the tracking branch".

  2. The list of refs after get_ref_map is actually more of an
     instruction/command list for the rest of the code to follow. It
     gets fed to store_updated_ref, but also impacts the status table we
     show. You could implement it such that a single ref entry had
     multiple storage destinations, but that would require changes to
     all of the consumers of the instruction list. Since we already need
     to handle extra refspecs (e.g., you can say "master:foobar
     master:refs/remotes/origin/master" on the command line already), we
     can treat these the same way. We append to the instruction list,
     and the rest of the code treats it as if you specified it on the
     command line.

-Peff