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

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 25, 2021, 18:03 UTC
Message-ID: <YSaF2h67u1WTz1b3@tanuki>
In-Reply-To: <xmqqv93tif9g.fsf@gitster.g>

On Wed, Aug 25, 2021 at 10:51:55AM -0700, Junio C Hamano wrote:

Show 26 quoted lines

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

Show 13 quoted lines

> 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