git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Patrick Steinhardt <ps@pks.im>
Cc: git@vger.kernel.org, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH] fetch: skip formatting updated refs with `--quiet`
Date: Wed, 25 Aug 2021 11:37:38 -0700	[thread overview]
Message-ID: <xmqqo89lid59.fsf@gitster.g> (raw)
In-Reply-To: <YSaF2h67u1WTz1b3@tanuki> (Patrick Steinhardt's message of "Wed, 25 Aug 2021 20:03:06 +0200")

Patrick Steinhardt <ps@pks.im> 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.

  reply	other threads:[~2021-08-25 18:37 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-25 15:45 [PATCH] fetch: skip formatting updated refs with `--quiet` Patrick Steinhardt
2021-08-25 16:57 ` Ævar Arnfjörð Bjarmason
2021-08-25 17:51 ` Junio C Hamano
2021-08-25 18:03   ` Patrick Steinhardt
2021-08-25 18:37     ` Junio C Hamano [this message]
2021-08-30 10:54 ` [PATCH v2] " Patrick Steinhardt
2021-08-30 17:17   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqo89lid59.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=ps@pks.im \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).