Re: [PATCH 6/8] fetch: deduplicate logic to print remote URL

Patrick Steinhardt [off-list ref] writes:

When fetching from a remote, we not only print the actual references
that have changed, but will also print the URL from which we have
fetched them to standard output. The logic to handle this is duplicated
across two different callsites with some non-trivial logic to compute
the anonymized URL. Furthermore, we're using global state to track
whether we have already shown the URL to the user or not.

If we are certain that store_updated_refs() is called only once for
the entire process, then storing the preprocessed url in the display
state and passing it around does sound like a good optimization and
clean-up.  What do we do when fetching from multiple remotes?

+	display->url_len = strlen(display->url);
+	for (i = display->url_len - 1; display->url[i] == '/' && 0 <= i; i--)
+		;
+	display->url_len = i + 1;

This loop is inherited from the original, but we may want to use
strrchr() or rindex() as a post clean-up after this series settles.

On Wed, Mar 15, 2023 at 04:02:18PM -0700, Junio C Hamano wrote:

Patrick Steinhardt [off-list ref] writes:

quoted

When fetching from a remote, we not only print the actual references
that have changed, but will also print the URL from which we have
fetched them to standard output. The logic to handle this is duplicated
across two different callsites with some non-trivial logic to compute
the anonymized URL. Furthermore, we're using global state to track
whether we have already shown the URL to the user or not.

If we are certain that store_updated_refs() is called only once for
the entire process, then storing the preprocessed url in the display
state and passing it around does sound like a good optimization and
clean-up.  What do we do when fetching from multiple remotes?

We execute separate git-fetch(1) processes when fetching from multiple
remotes or when fetching submodules, so we should be fine here.

quoted

+	display->url_len = strlen(display->url);
+	for (i = display->url_len - 1; display->url[i] == '/' && 0 <= i; i--)
+		;
+	display->url_len = i + 1;

This loop is inherited from the original, but we may want to use
strrchr() or rindex() as a post clean-up after this series settles.

Yeah, that'd make sense.

Patrick