Patrick Steinhardt [off-list ref] writes:
When fetching, Git will by default print a list of all updated refs in a
nicely formatted table. In order to come up with this table, Git needs
to iterate refs twice: first to determine the maximum column width, and
a second time to actually format these changed refs.
While this table will not be printed in case the user passes `--quiet`,
we still go out of our way and do all these steps. In fact, we even do
more work compared to not passing `--quiet`: without the flag, we will
skip all references in the column width computation which have not been
updated, but if it is set we will now compute widths for all refs.
Interesting. This line
/* uptodate lines are only shown on high verbosity level */
if (!verbosity && oideq(&ref->peer_ref->old_oid, &ref->old_oid))
return;
at the beginning of the adjust_refcol_width() function indeed does
not skip if verbosity is negative, so the comment is wrong---it is
not only computed on high verbosity level. Why doesn't this patch
include a change like this then?
if (verbosity <= 0 || oideq(&ref->peer_ref->old_oid, &ref->old_oid))
return;
Another thing I notice is this part from store_updated_refs():
if (note.len) {
if (verbosity >= 0 && !shown_url) {
fprintf(stderr, _("From %.*s\n"),
url_len, url);
shown_url = 1;
}
if (verbosity >= 0)
fprintf(stderr, " %s\n", note.buf);
}
We no longer need to check for verbosity, right?
Other than these two, I like the approach a lot.
On Wed, Aug 25, 2021 at 10:51:55AM -0700, Junio C Hamano wrote:
Patrick Steinhardt [off-list ref] writes:
quoted
When fetching, Git will by default print a list of all updated refs in a
nicely formatted table. In order to come up with this table, Git needs
to iterate refs twice: first to determine the maximum column width, and
a second time to actually format these changed refs.
While this table will not be printed in case the user passes `--quiet`,
we still go out of our way and do all these steps. In fact, we even do
more work compared to not passing `--quiet`: without the flag, we will
skip all references in the column width computation which have not been
updated, but if it is set we will now compute widths for all refs.
Interesting. This line
/* uptodate lines are only shown on high verbosity level */
if (!verbosity && oideq(&ref->peer_ref->old_oid, &ref->old_oid))
return;
at the beginning of the adjust_refcol_width() function indeed does
not skip if verbosity is negative, so the comment is wrong---it is
not only computed on high verbosity level. Why doesn't this patch
include a change like this then?
if (verbosity <= 0 || oideq(&ref->peer_ref->old_oid, &ref->old_oid))
return;
This was indeed my first iteration. But if we just fix the condition
like you do here, then we still iterate over all refs even though we
know that we're not going to do anything with them. So my version just
skips over the iteration completely.
Another thing I notice is this part from store_updated_refs():
if (note.len) {
if (verbosity >= 0 && !shown_url) {
fprintf(stderr, _("From %.*s\n"),
url_len, url);
shown_url = 1;
}
if (verbosity >= 0)
fprintf(stderr, " %s\n", note.buf);
}
We no longer need to check for verbosity, right?
Right. It would be less obvious though that we indeed never print to the
buffer if `verbosity < 0`, which is why I bailed on that refactoring.
I wonder whether we can just refactor this such that the buffer is
caller-provided. If `--quiet`, the caller just passes a `NULL` pointer
so it's explicit that it cannot be written to. This would also address
Ævar's feedback about the "tricky action at a distance", which is valid
criticism in my eyes.
Patrick