Re: [PATCH v14 13/27] bisect--helper: `bisect_start` shell function partially in C

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 25, 2016, 19:43 UTC
Message-ID: <xmqq8tvkmy7b.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqd1kwn035.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 17 quoted lines

>> +	for (i = 0; i < revs.nr; i++) {
>> +		if (bad_seen)
>> +			string_list_append(&states, terms->term_good.buf);
>> +		else {
>> +			bad_seen = 1;
>> +			string_list_append(&states, terms->term_bad.buf);
>> +		}
>> +	}
>
> This is certainly different from the original.  We used to do one
> "bisect_write" per element in revs in the loop; we no longer do that
> and instead collect them in states list.  Each element in these two
> lists, i.e. revs.item[i] and states.item[i], corresponds to each
> other.
>
> That explains why the variable is renamed to states.  We haven't
> seen enough to say if this behaviour change is a good idea or not.

Ahh, I misread the original. It accumulates the writes to be executed in $eval and does so at the end. So there is no change in behaviour.

So please ignore that point in the previous message. That leaves only the following points:

 * Perhaps 'retval' with 'goto finish' pattern?

 * ref_exists()?  Perhaps use skip_prefix(head, "refs/heads/, &branch)?

 * if (clean-state) { return -1 }?

 * Is comment on trap relevant here?

Sorry, and thanks.