Patrick Steinhardt <ps@xxxxxx> writes:
>> 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.
>> 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.
