Re: [PATCH 3/4] fetch: honor the user-provided refspecs when pruning refs
- From
Carlos Martín Nieto <cmn@elego.de>
- Date
- Oct 12, 2011, 23:18 UTC
- Message-ID
- <1318461519.4484.26.camel@centaur.lab.cmartin.tk>
- In-Reply-To
- <7vsjmx7uur.fsf@alter.siamese.dyndns.org>
On Wed, 2011-10-12 at 14:39 -0700, Junio C Hamano wrote:
Show 22 quoted lines
> 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.
Show 54 quoted lines
>
> > @@ -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.
Show 5 quoted lines
> > 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