Re: [PATCH 3/4] fetch: honor the user-provided refspecs when pruning refs

["Carlos Martín Nieto" <cmn@elego.de>]

[-- Attachment #1: Type: text/plain, Size: 4652 bytes --]

On Wed, 2011-10-12 at 14:39 -0700, Junio C Hamano wrote:
> Carlos Martín Nieto <cmn@elego.de> writes:
> 
> > -static int prune_refs(struct transport *transport, struct ref *ref_map)
> > +static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map)
> >  {
> >  	int result = 0;
> > -	struct ref *ref, *stale_refs = get_stale_heads(transport->remote, ref_map);
> > +	struct ref *ref, *stale_refs = get_stale_heads(ref_map, refs, ref_count);
> 
> So in short, get_state_heads() used to take a ref_map and a remote. The
> ref_map is what we actually observed from the remote after talking
> ls-remote with it. It tried to see if any existing ref in our refspace may
> have come from that remote by inspecting the fetch refspec associated with
> that remote (and the ones that does not exist anymore are queued in the
> stale ref list).
> 
> Now get_state_heads() takes a ref_map and <refs, ref_count> (you made the
> patch unnecessarily harder to read by swapping the order of parameters).
> The latter "pair" roughly corresponds to what the "remote" parameter used
> to mean, but instead of using the refspec associated with that remote, we
> would use the refspec used for this particular fetch to determine which
> refs we have are stale.

Right. The only reason that the remote was passed was in order to use
its refspec. The order reversal wasn't on purpose, I'll change that.

> 
> > @@ -699,8 +699,12 @@ static int do_fetch(struct transport *transport,
> >  		free_refs(ref_map);
> >  		return 1;
> >  	}
> > -	if (prune)
> > -		prune_refs(transport, ref_map);
> > +	if (prune) {
> > +		if (ref_count)
> > +			prune_refs(refs, ref_count, ref_map);
> > +		else
> > +			prune_refs(transport->remote->fetch, transport->remote->fetch_refspec_nr, ref_map);
> > +	}
> 
> And this is consistent to my two paragraph commentary above.
> 
> > diff --git a/builtin/remote.c b/builtin/remote.c
> > index f2a9c26..79d898b 100644
> > --- a/builtin/remote.c
> > +++ b/builtin/remote.c
> > @@ -349,7 +349,8 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat
> >  		else
> >  			string_list_append(&states->tracked, abbrev_branch(ref->name));
> >  	}
> > -	stale_refs = get_stale_heads(states->remote, fetch_map);
> > +	stale_refs = get_stale_heads(fetch_map, states->remote->fetch,
> > +				     states->remote->fetch_refspec_nr);
> 
> So is this.
> 
> > diff --git a/remote.c b/remote.c
> > index b8ecfa5..13c9153 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -1681,36 +1681,84 @@ struct ref *guess_remote_head(const struct ref *head,
> >  }
> >  
> >  struct stale_heads_info {
> > -	struct remote *remote;
> >  	struct string_list *ref_names;
> >  	struct ref **stale_refs_tail;
> > +	struct refspec *refs;
> > +	int ref_count;
> >  };
> >  
> > +/* Returns 0 on success, -1 if it couldn't find a match in the refspecs. */
> > +static int find_in_refs(struct refspec *refs, int ref_count, struct refspec *query)
> > +{
> > +	int i;
> > +	struct refspec *refspec;
> 
> This function replaces the role remote_find_tracking() used to play in the
> old code and the difference in the behaviour (except the obvious lack of
> "find_src/find_dst") feels gratuitous.

remote_find_tracking wants a remote, and that's what we don't have
anymore. The main reason was that it does "too much". The previous
versions had the callback doing more by itself, so I overlooked the
possibilities of remote_find_tracking when rewriting it. Looking at the
code again, it does look like what we want.

> 
> The original code in remote_find_tracking() uses "->pattern" to see if a
> pattern match is necessary, but this scans the refspec for an asterisk,
> assuring a breakage when the refspec language is updated to understand
> other glob magic in the future. Why isn't refspec->pattern used here?

Trees, forest etc. I noticed that a bit late. I have a patch on top of
this one that does use ->pattern, which I was going to ask you to squash
in, but it's moot now, as I need to rewrite the patch anyway.

> 
> Can't these two functions share more logic?  It appears to me that by
> enhancing the logic here a little bit, it may be possible to implement
> remote_find_tracking() ed in terms of this function as a helper.

Yes, remote_find_tracking should use a version of this function; or
probably better, its loop should become the next version of
find_in_refs, so remote_find_tracking is just a wrapper for when we want
to use the remote's fetch refspec.


I'll resend the series with these changes.

   cmn


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]