Max Horn [off-list ref] writes:
quoted
Thanks. Let's keep it a bit longer and see how your new
investigation (and possibly help from others) develops to a
solution.
So I had a closer look, and I now believe to now understand what
the right fix is. Simply dropping transport-helper: check for
'forced update' message would be wrong, because it would cause the
contrib/remote-helpers test cases to fail -- when I tested last
night, I forgot that I had to run those separately. Silly me!
...
In other words, the following patch should be the correct
solution, as far as I can tell. I verified that it fixes t5541 and
causes no regressions in the contrib/remote-helpers test suite.
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
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. 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? 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?
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?
quoted hunk
-- 8< --
diff --git a/transport-helper.c b/transport-helper.c
index fa7c608..86e1679 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -734,7 +734,7 @@ static int push_update_ref_status(struct strbuf *buf,
}
(*ref)->status = status;
- (*ref)->forced_update = forced;
+ (*ref)->forced_update |= forced;
(*ref)->remote_status = msg;
return !(status == REF_STATUS_OK);
}
-- 8< --
On 21.02.2014, at 19:04, Junio C Hamano [off-list ref] 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