Re: [PATCH v3 5/6] transport-helper.c::push_refs(): ignore helper-reported status if ref is not to be pushed
- From
Tay Ray Chuan <rctay89@gmail.com>
- Date
- Jan 5, 2010, 10:01 UTC
- Message-ID
- <20100105180113.6e0572dc.rctay89@gmail.com>
- In-Reply-To
- <20100105063253.GA19368@coredump.intra.peff.net>
Hi,
On Tue, Jan 5, 2010 at 2:32 PM, Jeff King <peff@peff.net> wrote:
Show 27 quoted lines
> On Thu, Dec 24, 2009 at 03:44:45PM +0800, Tay Ray Chuan wrote:
>
>> - ref->status = status;
>> - ref->remote_status = msg;
>> + switch (ref->status) {
>> + case REF_STATUS_REJECT_NONFASTFORWARD:
>> + case REF_STATUS_UPTODATE:
>> + /*
>> + * Earlier, the ref was marked not to be pushed, so ignore what
>> + * the remote helper said about the ref.
>> + */
>> + continue;
>> + default:
>> + ref->status = status;
>> + ref->remote_status = msg;
>> + }
>
> It seems like this should be checking for REF_STATUS_NONE explicitly
> instead of trying to enumerate the reasons we might not have tried to
> push. Shouldn't helpers _only_ be pushing REF_STATUS_NONE refs?
>
> I think right now the two cases are equivalent, since non-ff and
> uptodate are the only two states set before the helper is invoked. But
> we have discussed in the past (and I still have a patch floating around
> for) a REF_STATUS_REWIND which would treat strict rewinds differently
> (silently ignoring them instead of making an error). Explicitly checking
> REF_STATUS_NONE future-proofs against new states being added.I'm not really sure if this is true (ie. that if status is not non-ff or uptodate, then it is REF_STATUS_NONE), but we could step around this by introducing a property, say, ref->should_push, that is set to 1, after all the vetting has been carried out and just before we talk to the server.
That way, we just check that property, without having to know the ref statuses that would mark a ref not-for-pushing. Sounds future-proof (in your sense) to me.
-- Cheers, Ray Chuan