Re: What's cooking in git.git (Feb 2014, #06; Wed, 19)

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 21, 2014, 18:04 UTC
Message-ID: <xmqqr46w4a24.fsf@gitster.dls.corp.google.com>
In-Reply-To: <FB0B19C8-65BF-49CF-8EE4-5B9D55BBCE7C@quendi.de>

Max Horn <max@quendi.de> writes:

Show 13 quoted lines

>> 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?

Show 15 quoted lines

> -- 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< --