Re: [PATCH v3 2/5] status: add --[no-]ahead-behind to status and commit for V2 format.

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 4, 2018, 22:05 UTC
Message-ID: <xmqqlghde0uy.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20180103214733.797-3-git@jeffhostetler.com>

Jeff Hostetler <git@jeffhostetler.com> writes:

Show 16 quoted lines

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