From: Jeff Hostetler <git@jeffhostetler.com>
Date: Fri, 05 Jan 2018 16:31:08 GMT
Subject: Re: [PATCH v3 2/5] status: add --[no-]ahead-behind to status and commit for V2 format.
Message-ID: <6fe41395-6cb5-cbfe-88cd-5b0b21fad7b8@jeffhostetler.com>
In-Reply-To: <xmqqlghde0uy.fsf@gitster.mtv.corp.google.com>



On 1/4/2018 5:05 PM, Junio C Hamano wrote:
> Jeff Hostetler <git@jeffhostetler.com> writes:
> 
>> +		sti = stat_tracking_info(branch, &nr_ahead, &nr_behind,
>> +					 &base, s->ahead_behind_flags);
>>   		if (base) {
>>   			base = shorten_unambiguous_ref(base, 0);
>>   			fprintf(s->fp, "# branch.upstream %s%c", base, eol);
>>   			free((char *)base);
>>   
>> -			if (ab_info)
>> -				fprintf(s->fp, "# branch.ab +%d -%d%c", nr_ahead, nr_behind, eol);
>> +			if (sti >= 0) {
>> +				if (s->ahead_behind_flags == AHEAD_BEHIND_FULL)
>> +					fprintf(s->fp, "# branch.ab +%d -%d%c",
>> +						nr_ahead, nr_behind, eol);
>> +				else if (s->ahead_behind_flags == AHEAD_BEHIND_QUICK)
>> +					fprintf(s->fp, "# branch.equal %s%c",
>> +						sti ? "neq" : "eq", eol);
> 
> This is related to what I said in the review of [1/5], but this
> arrangement to require the caller to know that _QUICK request
> results in incomplete information smells like a bit of maintenance
> hassle.
> 
> We'd rather allow the caller to tell if it was given incomplete
> information from the values returned by stat_tracking_info()
> function (notice the plural "values" here; what is returned in
> nr_{ahead,behind} also counts).  For example, we can keep the (-1 =>
> "no relation", 0 => "identical", 1 => "different") as return values
> of the function, but have it clear nr_{ahead,behind} when it only
> knows the two are different but not by how many commits.  That way,
> this step can instead do:
> 
> 	ab_info = stat_tracking_info(...);
> 	if (base) {
> 		... do the base thing ...
> 		if (ab_info > 0) {
> 			/* different */
> 			if (nr_ahead || nr_behind)
> 				fprintf(... +%d -%d ... nr_ahead, nr_behind, ...);
> 			else
> 				fprintf(... "neq" ...);
> 		} else if (!ab_info) {
> 			/* same */
> 			fprintf(... +%d -%d ... nr_ahead, nr_behind, ...);
> 		}
> 		...
> 
> That would allow us to later add different kinds of _QUICK that do
> not give full information without having to update this consumer
> with something like
> 
> -	else if (s->ahead_behind_flags == AHEAD_BEHIND_QUICK)
> +	else if (s->ahead_behind_flags == AHEAD_BEHIND_QUICK ||
> +		 s->ahead_behind_flags == AHEAD_BEHIND_QUICK_DIFFERENTLY)
> 
> when it happens.
> 
> Two related tangents.
> 
>   - I do not see why "eq" need to exist at all.  Even in _QUICK mode,
>     when you determine that two are identical, you know your branch
>     is ahead and behind by 0 commit each.
> 
>   - A short-format that is easy to parse by machines does not have to
>     be cryptic to humans.  s/neq/not-equal/ or something may be an
>     improvement.
> 
> Thanks.
> 

Thanks for the review.  Let me update it along the lines suggested here.
It felt odd to define the nr_{ahead,behind} as undefined, but short of
making them negative or something I wasn't sure what was best.

Thanks,
Jeff