Re: [PATCH] remote: initialize values that might not be set

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 7, 2021, 23:21 UTC
Message-ID: <xmqqlf7ltg4q.fsf@gitster.g>
In-Reply-To: <nycvar.QRO.7.76.6.2106072355110.55@tvgsbejvaqbjf.bet>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 19 quoted lines

>> diff --git a/remote.c b/remote.c
>> index c3f85c17ca7c..a116392fb057 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -2101,7 +2101,7 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
>>  int format_tracking_info(struct branch *branch, struct strbuf *sb,
>>  			 enum ahead_behind_flags abf)
>>  {
>> -	int ours, theirs, sti;
>> +	int ours = 0, theirs = 0, sti = 0;
>
> While I like this change, I am somewhat confused where the values are used
> for branching. The only time I see them used when `stat_branch_pair()` has
> _not_ initialized `ours` and `theirs` is in those `trace2_data_intmax()`
> calls. Otherwise `sti` is set to -1 and the other users of `ours` and
> `theirs` aren't reached.
>
> If my reading of the code is correct, maybe the commit message could be
> adjusted to talk about tracing instead of branching?

I too wondered why initializing them to 0 is safe (instead of hiding latent bugs). I think that stat_tracking_info() would always return -1 if returns before reaching the point in stat_branch_pair(), but it is not clear how we can futureproof the whole thing.

If these two are initialized to say -1 here, and then we had some sanity check, perhaps like so:

	sti = stat_tracking_info(branch, &ours, &theirs, &full_base, 0, abf);
+	assert(sti < 0 || (0 <= ours && 0 <= theirs));
	if (sti < 0) {
		if (!full_base)
	...

to enforce the invariant we assume (i.e. OK sti means ours and theirs are set), it would allow us to sleep better, perhaps?