Re: [PATCH 2/2] builtin-remote: make rm operation safer in mirrored repository
- From
Jeff King <peff@peff.net>
- Date
- Feb 4, 2009, 15:42 UTC
- 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:
Show 7 quoted lines
> + /* 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)?
Show 6 quoted lines
> + 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