Patrick Steinhardt <ps@xxxxxx> writes:
> When displaying reference updates, we try to print the references in a
> neat table. As the table's width is determined its contents we thus need
> to precalculate the overall width before we can start printing updated
> references.
>
> The calculation is driven by `display_state_init()`, which invokes
> `refcol_width()` for every reference that is to be printed. This split
> is somewhat confusing. For one, we filter references that shall be
> attributed to the overall width in both places. And second, we
> needlessly recalculate the maximum line length based on the terminal
> columns and display format for every reference.
>
> Refactor the code so that the complete width calculations are neatly
> contained in `refcol_width()`.
Through no fault of yours, I have to admit that I found this refactor
quite hard to read. I ended up redoing the refactor and ended up with a
result very similar to yours. That was probably overkill since we have
pretty extensive tests in this area, but I'm quite happy with the change
since it's far more readable.
> - /*
> - * rough estimation to see if the output line is too long and
> - * should not be counted (we can't do precise calculation
> - * anyway because we don't know if the error explanation part
> - * will be printed in update_local_ref)
> - */
> - if (compact_format) {
> - llen = 0;
> + max = term_columns();
> + if (compact_format)
> max = max * 2 / 3;
> - }
> - len = 21 /* flag and summary */ + rlen + 4 /* -> */ + llen;
> - if (len >= max)
> - return 0;
The one thing that changed for the better (vs keeping the lines in the
same order as before) is that this comment that used to be anchored on
"if (compact_format) {"...
> + /*
> + * rough estimation to see if the output line is too long and
> + * should not be counted (we can't do precise calculation
> + * anyway because we don't know if the error explanation part
> + * will be printed in update_local_ref)
> + */
> + len = 21 /* flag and summary */ + rlen + 4 /* -> */ + llen;
> + if (len >= max)
> + continue;
is now more accurately anchored to the check that throws away the line.
Looks good.
