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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 25, 2021, 18:37 UTC
Message-ID: <xmqqo89lid59.fsf@gitster.g>
In-Reply-To: <YSaF2h67u1WTz1b3@tanuki>

Patrick Steinhardt <ps@pks.im> writes:

Show 18 quoted lines

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

I didn't ask "why isn't this patch the above change and nothing else?"

With or without the "don't bother counting columns under --quiet" fix, the condition used in the above if statement is wrong. With the fix, only because the function is never called with negative verbosity, "if (!verbosity)" means the same thing as "only shown on high verbosity level", but the reader must have followed the flow of logic to realize it. If you fix the condition to exclude all non-verbose cases (including --quiet), the readers do not have to do so to realize that the code is doing what the comment claims that it is doing.

Show 16 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 actually think that the resulting code will still be obvious, even with the (apparently unrelated) URL display, and actually even be better, if we drop the condition on verbosity from this code.

The code that led to this part would have stuffed bytes in the note strbuf only when we wanted to show something based on the verbosity setting, and we show what is in note.len only when we have something to say (the URL thing is to give the header for the message). Decision to give what contents to show (or not to show at all) is made elsewhere---it happens to involve verbosity and perhaps in the future it may use some other input, but this part of the code does not have to know.

Thanks.