On 2014-02-21 10.55, Max Horn wrote: > > On 20.02.2014, at 20:22, Junio C Hamano <gitster@xxxxxxxxx> wrote: > >> Max Horn <max@xxxxxxxxx> writes: >> >>> On 19.02.2014, at 22:41, Junio C Hamano <gitster@xxxxxxxxx> wrote: >>> >>>> * fc/transport-helper-fixes (2013-12-09) 6 commits >>>> - remote-bzr: support the new 'force' option >>>> - test-hg.sh: tests are now expected to pass >>>> - transport-helper: check for 'forced update' message >>>> - transport-helper: add 'force' to 'export' helpers >>>> - transport-helper: don't update refs in dry-run >>>> - transport-helper: mismerge fix >>>> >>>> Reported to break t5541, and has been stalled for a while without >>>> fixes. >>> ... >>> Since I somewhat care about transport-helpers, I had a closer look >>> at this failure. >> >> 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! > > Indeed, these tests include a test with a force push, and trigger the code path added in that commit. The remaining problem then is that the new code should be changed to only tell git when a remote-helper signals a forced update; but it should not incorrectly reset the forced_update flag to 0 if git already considers the update to be forced. > > 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. > > -- 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< -- > > Ack from my side: There are 2 fields: ref->force and ref->forced_update. forced_update is set in set_ref_status_for_push() in remote.c: if (!force_ref_update) ref->status = reject_reason; else if (reject_reason) ref->forced_update = 1; } And transport-helper.c sets it to 0 even if it had been 1, which is wrong. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html