From: Jeff King <peff@peff.net>
Date: Wed, 04 Feb 2009 15:42:27 GMT
Subject: Re: [PATCH 2/2] builtin-remote: make rm operation safer in mirrored repository
Message-ID: <20090204154227.GE6896@sigill.intra.peff.net>
In-Reply-To: <1233683473-87893-2-git-send-email-jaysoffian@gmail.com>

On Tue, Feb 03, 2009 at 12:51:13PM -0500, Jay Soffian wrote:

> So much work for what seemed such a minor change. :) I hope this version is
> well-polished enough.

The more minor the change, the more major the comments. :) Thanks for
sticking with it. This version looks good to me except for one thing:

> +	/* don't delete non-remote branches */
> +	if (prefixcmp(refname, "refs/remotes")) {
> +		if (!prefixcmp(refname, "refs/heads/"))
> +			string_list_append(abbrev_branch(refname),
> +					   branches->skipped);
> +		return 0;
> +	}

Why does this version introduce the "only skip refs/heads/" check?
Shouldn't we also protect other random refs (or if not, shouldn't the
commit message explain why not)?

> +	if (skipped.nr) {
> +		fprintf(stderr, skipped.nr == 1 ?
> +			"Note: A non-remote branch was not removed; "
> +			"to delete it, use:\n" :
> +			"Note: Non-remote branches were not removed; "
> +			"to delete them, use:\n");

Ooh, proper pluralization. Classy.

-Peff