On 21.02.2014, at 19:04, Junio C Hamano <gits...@pobox.com> 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 

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.


Attachment: signature.asc
Description: Message signed with OpenPGP using GPGMail

Reply via email to