Re: [PATCH 2/2] builtin-remote: make rm operation safer in mirrored repository

From: Jay Soffian <jaysoffian@gmail.com>
Date: Feb 4, 2009, 15:56 UTC
Message-ID: <76718490902040756m1f5c6f37o45865c51ad1a2e6d@mail.gmail.com>
In-Reply-To: <20090204154227.GE6896@sigill.intra.peff.net>

On Wed, Feb 4, 2009 at 10:42 AM, Jeff King <peff@peff.net> wrote:

Show 11 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)?

Note that we do protect refs, but we only emit messages about those refs which are obviously branches. Frankly, I wasn't sure what other kinds of refs there might be, so wasn't sure what an appropriate message is for anything other than those under refs/heads.

In particular though, I noticed that w/o this check, I was emitting an incorrect message about anything under refs/tags. I thought about saying "and you can clean up these ignored tags like so", but that is likely to emit a huge number of messages, so I thought it best just to silently ignore non-remote non-branch refs. Perhaps I should better explain that in a code comment.

Alternately, it could do something like:

Note: A non-remote branch was not removed; to delete it use:
   git branch -d ...

Note: Tags were not removed, to delete them use:
   git tag -d ...

Note: Some refs were ignored:
   refs/whoknows/whatthisis
   refs/whoknows/whatthiscouldbe

But that's getting a little insane me thinks.

j.