Re: [PATCH v2 1/2] fetch-prune: optimize dangling-ref reporting
- From
Jeff King <peff@peff.net>
- Date
- Jun 24, 2025, 10:49 UTC
- Message-ID
- <20250624104904.GE636332@coredump.intra.peff.net>
- In-Reply-To
- <20250623234327.335490-2-phil.hord@gmail.com>
On Mon, Jun 23, 2025 at 04:43:26PM -0700, Phil Hord wrote:
Show 14 quoted lines
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 40a0e8d24434..65d606c6de08 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1383,9 +1383,13 @@ static int prune_refs(struct display_state *display_state,
> int result = 0;
> 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)")
> - : _(" (%s has become dangling)");
> + ? _(" %s will become dangling after %s is deleted")
> + : _(" %s has become dangling after %s was deleted");This approach seems reasonable. It is a little ugly that refs_warn_dangling_symrefs() takes a printf-formatted string that must contain the correct number of "%s" fields (and that we get no compiler warnings if we get it wrong).
But that is not really new in your series. Given that there are two callers and they use (almost) the same string, I wonder if we could refactor the interface. We'd need to pass in the indentation level, and the dry-run flag.
I guess alternatively, we could have a function which passes back a strvec or similar of danglers, but then both call sites would have more printing boilerplate. I dunno. Maybe we should just avert our eyes and live with it. ;)
Show 14 quoted lines
> diff --git a/refs.c b/refs.c
> index dce5c49ca2ba..e2075a98c844 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -461,7 +461,9 @@ static int warn_if_dangling_symref(const char *refname, const char *referent UNU
> return 0;
> }
>
> - fprintf(d->fp, d->msg_fmt, refname);
> + skip_prefix(refname, "refs/remotes/", &refname);
> + skip_prefix(resolves_to, "refs/remotes/", &resolves_to);
> + fprintf(d->fp, d->msg_fmt, refname, resolves_to);
> fputc('\n', d->fp);
> return 0;This prefix handling feels kind of ad-hoc. Should we use something like refs_shorten_unambiguous_ref() to follow the usual rules?
This is also shortening the symref name, which didn't happen before. Arguably that should happen in a separate patch, but I can live with it either way.
-Peff