Re: [PATCH] fetch: skip formatting updated refs with `--quiet`

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2395 bytes --]

On Wed, Aug 25, 2021 at 10:51:55AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> 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;

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]