From: Kristoffer Haugsbakk Date: Tue, 17 Mar 2026 22:07:59 GMT Subject: Re: [PATCH 2/2] name-rev: learn --format= Message-ID: In-Reply-To: On Sat, Mar 14, 2026, at 01:22, Junio C Hamano wrote: > kristofferhaugsbakk@fastmail.com writes: > >> diff --git a/Documentation/git-name-rev.adoc b/Documentation/git-name-rev.adoc >> index d4f1c4d5945..8f050cd4763 100644 >> --- a/Documentation/git-name-rev.adoc >> +++ b/Documentation/git-name-rev.adoc >> @@ -9,7 +9,7 @@ git-name-rev - Find symbolic names for given revs >> SYNOPSIS >> -------- >> [verse] >> -'git name-rev' [--tags] [--refs=] >> +'git name-rev' [--tags] [--refs=] [--format=] >> ( --all | --annotate-stdin | ... ) > > We acquired a new option. Do we need a matching change to > the contents of name_rev_usage[] array? I looked at it and it seemed that `[]` were supposed to stay inside that placeholder. But since this is a new mode, maybe: diff --git builtin/name-rev.c builtin/name-rev.c index 6188cf98ce0..13e67a7723c 100644 --- builtin/name-rev.c +++ builtin/name-rev.c @@ -504,6 +504,7 @@ static char const * const name_rev_usage[] = { N_("git name-rev [] ..."), N_("git name-rev [] --all"), N_("git name-rev [] --annotate-stdin"), + N_("git name-rev --format= ..."), NULL }; > >> +--format=:: >> +--no-format:: >> + Format revisions instead of outputting symbolic names. The >> + default is `--no-format`. >> ++ >> +Implies `--name-only`. > > If it is implication, would > > git name-rev --format=reference --no-name-only > > do what is naturally expected? No it wouldn’t. You’re right, it really locks in that option with no escape hatch. But in my next version I have switched to a parse-options callback so that `... --format=... --no-name-only` really does turn off `--name-only`. > >> @@ -462,6 +472,25 @@ static const char *get_rev_name(const struct object *o, struct strbuf *buf) >> if (o->type != OBJ_COMMIT) >> return get_exact_ref_match(o); >> c = (const struct commit *) o; >> + >> + if (format_ctx) { >> + strbuf_reset(buf); >> + >> + if (format_ctx->want.notes) { >> + struct strbuf notebuf = STRBUF_INIT; >> + >> + format_display_notes(&c->object.oid, ¬ebuf, >> + get_log_output_encoding(), >> + format_ctx->ctx.fmt == CMIT_FMT_USERFORMAT); >> + format_ctx->ctx.notes_message = strbuf_detach(¬ebuf, NULL); >> + } >> + >> + pretty_print_commit(&format_ctx->ctx, c, buf); >> + free(format_ctx->ctx.notes_message); > > Is free() the expected thing to do here, or FREE_AND_NULL()? Unlike > callers like log-tree.c:show_log() where a context is prepared, used > once, and then discarded, format_pp is initialized in cmd_name_rev() > once and then repeatedly used by show_name() potentially multiple > times, so there may be a risk of getting confused by this leftover > non-NULL pointer that points at an already free'd piece of memory. > > Or there may not be---I did not check, but you as the author must > have already checked, hence this question. This is supposed to be tested by `--name-rev --format= with a note`; it has a note on the first revision but not the second. Here we never use this pointer again and we get a fresh pointer from the strbuf before freeing again (whether it gets populated with a pointer or not). But it does sound better to just null it. There’s no need to have it laying around. > >> + return buf->buf; >> + } >> + >>[snip] >> @@ -567,6 +599,10 @@ int cmd_name_rev(int argc, >> #endif >> int all = 0, annotate_stdin = 0, allow_undefined = 1, always = 0, peel_tag = 0; >> struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP, STRING_LIST_INIT_NODUP }; >> + const char *format = NULL; >> + struct rev_info format_rev = REV_INFO_INIT; >> + struct pretty_format *format_ctx = NULL; >> + struct pretty_format format_pp = {0}; > > Hmph, would we want to use the full init_revisions() instead of > static REV_INFO_INIT that initialises a lot more members of the > struct properly, most importantly the "repo" member that points at > the repostiory to be used? Here I looked at the doc for `pretty.h:get_commit_format` in order to learn what I needed to set up. Since it doesn’t say much I did the least work that I could get away with when it comes to struct initializing. Since it does seem like a struct for a lot of different situations while this is just a formatting situation. I might have done less research than I ought to. > >> + if (format) { >> + struct pretty_print_context ctx = {0}; >> + struct userformat_want want = {0}; >> + >> + get_commit_format(format, &format_rev); >> + ctx.rev = &format_rev; >> + ctx.fmt = format_rev.commit_format; >> + ctx.abbrev = format_rev.abbrev; >> + ctx.date_mode_explicit = format_rev.date_mode_explicit; >> + ctx.date_mode = format_rev.date_mode; >> + ctx.color = GIT_COLOR_AUTO; >> + format_pp.ctx = ctx; > > Why does this code initialize and assign to a on-stack ctx first and > then assign it to format_pp.ctx, instead of working on format_pp.ctx > directly? You’re right. I’ll just assign directly. > >> + userformat_find_requirements(format, &want); >> + if (want.notes) >> + load_display_notes(NULL); >> + >> + format_pp.want = want; >> + format_ctx = &format_pp; >> + >> + data.name_only = true; >> + } >> +