On 21.02.2014, at 19:04, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Here is a description I wrote for a tentative commit to queue on
> 'pu' after seeing your response:
>
> transport-helper.c: do not overwrite forced bit
I'd change "forced bit" to "forced_update bit"
>
> It does not necessarily mean the update was *not* forced, when the
> helper did not say "forced update". When the helper does say so, we
> know the update is forced.
>
> A workaround to fix breakage introduced by the previous step,
> proposed by Max Horn.
>
> It troubled me that "it does not necessarily mean" sounded really
> wishy-washy.
But it's correct. But if you want to be more precise, how about this:
If the the transport helper says it was a forced update, then it is
a forced update. But it is possible that an update is forced without
the transport-helper knowing about it, namely because some higher up
code had objections to the update and needed forcing in order to let
it through to the transport helper.
> Isn't it possible for some helpers to _do_ want to
> tell us that it did not have to force after all by _not_ saying
> "forced update" and overwrite ->forced_update with zero?
Yes to the first part, no to the last bit: Yes, a transport helper can (and frequently does) tell us that no force happened -- by not saying "forced update".
But not, it should not get to reset the forced_update bit. Because it can't know what other reasons there might have been for requiring a --force. If you look at the code, right now the only place setting forced_update is set_ref_status_for_push() in remote.c. If it decides to set forced_update, it does so, without giving the transport helper any say in in the matter. So the update will fail. Unless the user forces it. Only then the transport helper gets involved, and indeed, it could happen that the transport helper happily pushes the changes without seeing any reason to signal a "forced update".
But the update still should be marked as "forced", because it *was* forced. The transport helper can't know about that, and consequently, it doesn't make sense to allow it to override the statement of its betters ;-).
> How do we tell helpers that do want to do so apart from other helpers that say
> "forced update" only when they noticed they are indeed forcing?
I am not completely sure I even understand that bit? So far, no remote helper should have ever said "forced update", as they weren't allowed to. Now, we allow transport helpers supporting the "force" feature to signal that, "yes, indeed, I had to force this update". And the only thing we use that for is to report this fact to the user. So, that seems fine and all cases covered. No?
BTW, I guess we could perform an extra test and raise an error if a transport helper dares to request the "forced update" message even though it wasn't told that this update is supposed to be forced, i.e. even though !ref->force && !(flags & TRANSPORT_PUSH_FORCE) holds -- but doing that would mostly be something to help transport helper developers to catch misbehavior in their remote helpers, and could just as well be verified by suitable test cases.
> Perhaps the logic needs to be more like:
>
> if (we know helper will tell us the push did not have to
> force by *not* saying "forced update") {
> (*ref)->forced_update = forced;
> }
>
> i.e. for helpers that do not use the 'forced update' feature, simply
> ignore this "forced" variable altogether, no?
Huh? For helpers not using the "forced updated" feature, the local "forced" variable stays zero, hence "forced_update |= forced" already does nothing.
Max
Attachment:
signature.asc
Description: Message signed with OpenPGP using GPGMail