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

From: Carlos Martín Nieto <cmn@elego.de>
Date: Oct 7, 2011, 16:37 UTC
Message-ID: <1318005433.4579.5.camel@centaur.lab.cmartin.tk>
In-Reply-To: <20111007162625.GB4399@sigill.intra.peff.net>

On Fri, 2011-10-07 at 12:26 -0400, Jeff King wrote:

Show 13 quoted lines

> On Thu, Oct 06, 2011 at 11:21:46PM +0200, Carlos Martín Nieto wrote:
> 
> > If the user gave us refspecs on the command line, we should use those
> > when deciding whether to prune a ref instead of relying on the
> > refspecs in the config.
> > 
> > Previously, running
> > 
> >     git fetch --prune origin refs/heads/master:refs/remotes/origin/master
> > 
> > would delete every other tag under the origin namespace because we
> 
> I assume you mean s/tag/branch/ in the last line?

Yeah, maybe ref would be better?

Show 9 quoted lines

> 
> > ---
> >  builtin/fetch.c  |    6 ++--
> >  builtin/remote.c |    2 +-
> >  remote.c         |   78 +++++++++++++++++++++++++++++++++++++++++++++++++-----
> >  remote.h         |    3 +-
> >  4 files changed, 77 insertions(+), 12 deletions(-)
> 
> Tests?

Good point.

Show 33 quoted lines

> 
> >  static int get_stale_heads_cb(const char *refname,
> >  	const unsigned char *sha1, int flags, void *cb_data)
> >  {
> >  	struct stale_heads_info *info = cb_data;
> >  	struct refspec refspec;
> > +	int ret;
> >  	memset(&refspec, 0, sizeof(refspec));
> >  	refspec.dst = (char *)refname;
> > -	if (!remote_find_tracking(info->remote, &refspec)) {
> > -		if (!((flags & REF_ISSYMREF) ||
> > -		    string_list_has_string(info->ref_names, refspec.src))) {
> > -			struct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);
> > -			hashcpy(ref->new_sha1, sha1);
> > -		}
> > +
> > +	/*
> > +	 * If the user speicified refspecs on the command line, we
> > +	 * should only use those to check. Otherwise, look in the
> > +	 * remote's configuration for the branch.
> > +	 */
> > +	if (info->ref_count)
> > +		ret = find_in_refs(info->refs, info->ref_count, &refspec);
> > +	else
> > +		ret = remote_find_tracking(info->remote, &refspec);
> 
> Minor typo in the comment. But more importantly, this feels like a very
> low-level place to be thinking about things like what the user gave us
> on the command line.
> 
> Shouldn't get_stale_heads not get a remote at all, and just get a set of
> refspecs? Those should be the minimal information it needs to get its
> answer, right?

OK, so take a step back and figure out what we want the rules to be before we call get_stale_heads. It does sound like a more elegant approach. I was trying to disrupt the callers as little as possible, but then again, there's only two. Will change.

cmn