Re: [PATCH v4 3/3] clean up interface for refs_warn_dangling_symrefs

From: Jeff King <peff@peff.net>
Date: Jul 8, 2025, 01:35 UTC
Message-ID: <20250708013534.GA549007@coredump.intra.peff.net>
In-Reply-To: <20250702011214.2835529-5-phil.hord@gmail.com>

On Tue, Jul 01, 2025 at 06:12:15PM -0700, Phil Hord wrote:

Show 10 quoted lines

> The refs_warn_dangling_symrefs interface is a bit fragile as it passes
> in printf-formatting strings with expectations about the number of
> arguments. This patch series made it worse by adding a 2nd positional
> argument. But there are only two call sites, and they both use almost
> identical display options.
> 
> Make this safer by moving the format strings into the function that uses
> them to make it easier to see when the arguments don't match. Pass a
> prefix string and a dry_run flag so the decision logic can be handled
> where needed.

Thanks, I think the result is nicer. I have two comments, but I don't think either will merit a re-roll.

Show 17 quoted lines

> @@ -1384,9 +1384,6 @@ static int prune_refs(struct display_state *display_state,
>  	struct ref *ref, *stale_refs = get_stale_heads(rs, ref_map);
>  	struct strbuf err = STRBUF_INIT;
>  	struct string_list refnames = STRING_LIST_INIT_NODUP;
> -	const char *dangling_msg = dry_run
> -		? _("   %s will become dangling after %s is deleted")
> -		: _("   %s has become dangling after %s was deleted");
>  
>  	for (ref = stale_refs; ref; ref = ref->next)
>  		string_list_append(&refnames, ref->name);
> @@ -1417,7 +1414,7 @@ static int prune_refs(struct display_state *display_state,
>  		}
>  		string_list_sort(&refnames);
>  		refs_warn_dangling_symrefs(get_main_ref_store(the_repository),
> -					   stderr, dangling_msg, &refnames);
> +					   stderr, "   ", dry_run, &refnames);
>  	}

I had imagined passing in an "int indent", and not an arbitrary string. But passing in the string is actually more flexible (it really could be any prefix, not just an indentation). I think calling it "prefix" in the actual function might be the more usual term here, but it's probably just bike-shedding.

Show 6 quoted lines

> -	fprintf(d->fp, d->msg_fmt, refname, resolves_to);
> -	fputc('\n', d->fp);
> +	msg = d->dry_run
> +		? _("%s%s will become dangling after %s is deleted\n")
> +		: _("%s%s has become dangling after %s was deleted\n");
> +	fprintf(d->fp, msg, d->indent, refname, resolves_to);

Translators might find the extra "%s" at the beginning confusing without context. I think you can do something like:

  /* TRANSLATORS: The first %s is whitespace indentation. */

or similar. But maybe it would be more obvious as:

  fputs(d->indent, d->fp);
  fprintf(d->fp, msg, refname, resolves_to);
  fputc('\n', d->fp);

? I dunno. Maybe putting it all together gives translators more options (e.g., in a RTL language). I don't know much about translation.

Likewise on including the newline in the translated string. I think we usually don't, just because we're mostly passing in strings for error(), etc. But I don't know how much it matters.

-Peff