Re: [PATCH 2/2] name-rev: learn --format=<pretty>

From: Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com>
Date: Mar 17, 2026, 22:07 UTC
Message-ID: <d3813f1d-174b-4d1b-b0c5-c6a8db260f6c@app.fastmail.com>
In-Reply-To: <xmqq8qbvz2dm.fsf@gitster.g>

On Sat, Mar 14, 2026, at 01:22, Junio C Hamano wrote:

Show 16 quoted lines

> 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=<pattern>]
>> +'git name-rev' [--tags] [--refs=<pattern>] [--format=<pretty>]
>>  	       ( --all | --annotate-stdin | <commit-ish>... )
>
> 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 `[<options>]` 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 [<options>] <commit>..."),
            N_("git name-rev [<options>] --all"),
            N_("git name-rev [<options>] --annotate-stdin"),
    +       N_("git name-rev --format=<pretty> ..."),
            NULL
     };

Show 13 quoted lines

>
>> +--format=<pretty>::
>> +--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`.

Show 30 quoted lines

>
>> @@ -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, &notebuf,
>> +					     get_log_output_encoding(),
>> +					     format_ctx->ctx.fmt == CMIT_FMT_USERFORMAT);
>> +			format_ctx->ctx.notes_message = strbuf_detach(&notebuf, 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=<pretty> 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.

Show 18 quoted lines

>
>> +		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.

Show 17 quoted lines

>
>> +	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.

Show 11 quoted lines

>
>> +		userformat_find_requirements(format, &want);
>> +		if (want.notes)
>> +			load_display_notes(NULL);
>> +
>> +		format_pp.want = want;
>> +		format_ctx = &format_pp;
>> +
>> +		data.name_only = true;
>> +	}
>> +