On 1/4/2018 5:05 PM, Junio C Hamano wrote:
Jeff Hostetler <g...@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

Reply via email to