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

Hi,

On Wed, Jan 6, 2010 at 8:04 PM, Jeff King [off-list ref] wrote:

On Tue, Jan 05, 2010 at 06:01:13PM +0800, Tay Ray Chuan wrote:

quoted

quoted

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

Well, consider it this way. If it's _not_ REF_STATUS_NONE, then what is
it, and what does it mean to be overwriting it?

Ok, I'll take your suggestion from your previous email and do this:

@@ -429,8 +429,16 @@ static int push_refs(struct transport *transport,
 			continue;
 		}

-		ref->status = status;
-		ref->remote_status = msg;
+		if (ref->status == REF_STATUS_NONE) {
+			ref->status = status;
+			ref->remote_status = msg;
+		} else {
+			/*
+			 * Earlier, the ref was marked not to be pushed, so ignore what
+			 * the remote helper said about the ref.
+			 */
+			continue;
+		}
 	}
 	strbuf_release(&buf);
 	return 0;

Going by this principle (only refs with status of none will be pushed),
I think I should also squash the below into patch 3 (refactor ref status
logic for pushing):

@@ -336,11 +336,10 @@ static int push_refs(struct transport *transport,
 			continue;
 
 		switch (ref->status) {
-		case REF_STATUS_REJECT_NONFASTFORWARD:
-		case REF_STATUS_UPTODATE:
-			continue;
+		case REF_STATUS_NONE:
+			; /* carry on with pushing */
 		default:
-			; /* do nothing */
+			continue;
 		}
 
 		if (force_all)

Maybe I am misunderstanding the problem the patch is addressing, but the
point of these REF_STATUS feels was to act as a small state machine.
Everything starts as NONE, and then:

  - we compare locally against remote refs. We may transition:
      NONE -> UPTODATE
      NONE -> REJECT_NONFASTFORWARD
      NONE -> REJECT_NODELETE

  - we send the push list
      NONE -> EXPECTING_REPORT (if the remote supports individual status)
      NONE -> OK (otherwise)

  - we get back status responses
      EXPECTING_REPORT -> OK
      EXPECTING_REPORT -> REMOTE_REJECT

I haven't looked closely at the new transport helper code, but I would
think it should stick more or less to those transitions. The exception
would be that some transports don't necessarily handle EXPECTING_REPORT
in the same way, and may transition directly from NONE to
OK/REMOTE_REJECT.

minor nit: yes, this may differ from transport-to-transport, but
EXPECTING_REPORT is not used at all in the top-level transport (the
level above the helper).

There's also something I'd like to point out for accuracy: it's that
this sequence of transitions occur at two levels, separately: one at
the top-level transport/transport-helper, and another at the helper.

So, for certain non-ff refs (the type this patch series is looking at),
the sequence of state transitions stops and doesn't continue to step 2
in the top-level transport (sending the push list); but separately, in
the helper, the ref goes through another sequence of state transitions.

What this patch touches is the part in the top-level transport that
syncs the ref status between the helper and the top-level transport: do
we take and present to the user what the helper has done, or not?

Regarding this point, I now think that we should ignore the
helper-reported status only if that status is none, and continue
updating the ref status in the top-level transport if the helper did
push successfully/failed, even if we didn't tell it to push:

@@ -429,7 +429,7 @@ static int push_refs(struct transport *transport,

		ref->status = status;
		ref->remote_status = msg;
-		if (ref->status == REF_STATUS_NONE) {
+		if (ref->status == REF_STATUS_NONE && status == REF_STATUS_NONE) {
			ref->status = status;
			ref->remote_status = msg;
		} else {

So offhand, I would say that your list should also probably include
REJECT_NODELETE. However, I think that status is just for old servers
which didn't support the delete-refs protocol extension. So presumably
that is none of the new helpers, as they all post-date the addition of
that feature by quite a few years.

You're right, AFAIK, for the smart http protocol; I don't think it
supports NODELETE.

quoted

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.

I'd rather not introduce new state. The point of the status flag was to
encapsulate all of that information, and a new state variable just seems
like introducing extra complexity. If we are not in the NONE state, I
don't see why we would tell the helper about a ref at all.

Noted.

-- 
Cheers,
Ray Chuan