On Wed, 2011-10-12 at 14:39 -0700, Junio C Hamano wrote:
> Carlos Martín Nieto <cmn@xxxxxxxx> 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:
signature.asc
Description: This is a digitally signed message part