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

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

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?

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.

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.

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.

-Peff