Re: [RFC PATCH 1/2] add a --delete option to git push

From: Sverre Rabbelier <srabbelier@gmail.com>
Date: Aug 14, 2009, 06:24 UTC
Message-ID: <fabb9a1e0908132324td6869aydc752f67b95546f1@mail.gmail.com>
In-Reply-To: <20090814052153.GA2881@coredump.intra.peff.net>

Heya,

On Thu, Aug 13, 2009 at 22:21, Jeff King<peff@peff.net> wrote:

> On Thu, Aug 13, 2009 at 10:05:48PM -0700, Sverre Rabbelier wrote:
> From reading your patch, it looks like it just touches the command-line.
> I think that's the right thing to do, but I think it makes sense to
> think half a second to make sure.

Indeed, the reason I sent out this RFC was to gather more opinions :).

> And with the way you have it, "git push --delete" will silently ignore
> the --delete and push configured refspecs. Probably it should say
> "--delete is useless without refspecs on the command line".

This makes sense, I will fix that.

Show 11 quoted lines

>>     Currently `git push --delete master:master` results in a somewhat
>>     cryptic error message. It seems unlikely however, that those new
>>     to git would use the 'old:new' notation, so I haven't bothered
>>     guarding against it and settled for documenting it.
>
> It seems like it would be simple enough to just check whether the
> refspec contains a colon; if so, silently leave it alone. That could
> also protect configured refspecs, as mentioned above, but I wouldn't
> rule out somebody have a single-name refspec in their config (in fact, I
> think "remote.$X.push = HEAD" is reasonable -- should that delete the
> HEAD on "git push --delete"?).

I don't think we should touch any configured refspecs, think about how often one would use that vs. the inconvenience of doing so unintentionally.

Show 8 quoted lines

>> +--delete::
>> +    Delete the specified refs. Prefixes all refs with ':' to tell the
>> +    push machinery to delete the specified ref. As such, the refs
>> +    that are to be deleted should not contain a ':' specifier.
>> +
>
> This impacts _all_ refspecs. Remember that we can have multiple refspecs
> on the command-line. So I can "move" a ref remotely with:

Correct, hence the plural 'refs'.

Show 5 quoted lines

>  git push :old-name old-name:new-name
>
> but I can't do:
>
>  git push --delete old-name old-name:new-name

I don't think that's the use case for this option, it is mostly for new users who do not know about the colon notation; now you do raise a valid point that we might want to add a 'git push --rename old new' at some point, but I think that's beyond the scope of this patch.

> So maybe it would make more sense for it to be "--delete <ref>" and
> impact only a single ref. The simple case of "git push --delete foo"
> would remain unchanged.

I thought about that, but I decided that it was both intuitive and convenient to be able to delete multiple refs this way.

> The counter-argument is that "--delete" does not necessarily need to be
> as powerful as the ":ref" syntax, but I don't see the downside in making
> it so.

I do, it's easy to make mistakes when it's more powerful, and I think less intuitive. I think we want this to be as intuitive as possible.

I'm not very opinionated over any of this, if you have strong feelings yourself please let me know and I'll change the patch.

-- 
Cheers,

Sverre Rabbelier