Re: [PATCH v3 5/6] transport-helper.c::push_refs(): ignore helper-reported status if ref is not to be pushed

Hi,

On Tue, Jan 5, 2010 at 2:32 PM, Jeff King [off-list ref] wrote:

On Thu, Dec 24, 2009 at 03:44:45PM +0800, Tay Ray Chuan wrote:

quoted

-             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