From: "Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail•com>
To: "Junio C Hamano" <gitster@pobox•com>
Cc: git@vger•kernel.org, "Kristoffer Haugsbakk" <code@khaugsbakk•name>
Subject: Re: [PATCH 2/2] name-rev: learn --format=<pretty>
Date: Tue, 17 Mar 2026 23:07:59 +0100 [thread overview]
Message-ID: <d3813f1d-174b-4d1b-b0c5-c6a8db260f6c@app.fastmail.com> (raw)
In-Reply-To: <xmqq8qbvz2dm.fsf@gitster.g>
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=<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
};
>
>> +--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`.
>
>> @@ -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=<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.
>
>> + 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;
>> + }
>> +
next prev parent reply other threads:[~2026-03-17 22:08 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-03-13 16:03 [PATCH 0/2] name-rev: learn --format=<pretty> kristofferhaugsbakk
2026-03-13 16:03 ` [PATCH 1/2] name-rev: wrap both blocks in braces kristofferhaugsbakk
2026-03-14 0:22 ` Junio C Hamano
2026-03-17 22:10 ` Kristoffer Haugsbakk
2026-03-13 16:03 ` [PATCH 2/2] name-rev: learn --format=<pretty> kristofferhaugsbakk
2026-03-14 0:22 ` Junio C Hamano
2026-03-17 22:07 ` Kristoffer Haugsbakk [this message]
2026-03-18 15:36 ` Kristoffer Haugsbakk
2026-03-20 13:09 ` [PATCH v2 0/2] " kristofferhaugsbakk
2026-03-20 13:09 ` [PATCH v2 1/2] name-rev: wrap both blocks in braces kristofferhaugsbakk
2026-03-20 13:09 ` [PATCH v2 2/2] name-rev: learn --format=<pretty> kristofferhaugsbakk
2026-03-20 15:25 ` D. Ben Knoble
2026-03-23 17:34 ` Kristoffer Haugsbakk
2026-04-28 22:25 ` [PATCH v3 0/5] format-rev: introduce builtin for on-demand pretty formatting kristofferhaugsbakk
2026-04-28 22:25 ` [PATCH v3 1/5] name-rev: wrap both blocks in braces kristofferhaugsbakk
2026-04-28 22:25 ` [PATCH v3 2/5] name-rev: run clang-format before factoring code kristofferhaugsbakk
2026-04-28 22:25 ` [PATCH v3 3/5] name-rev: factor code for sharing with a new command kristofferhaugsbakk
2026-04-30 13:54 ` Phillip Wood
2026-05-01 17:24 ` kristofferhaugsbakk
2026-05-02 10:00 ` Phillip Wood
2026-05-05 19:21 ` Kristoffer Haugsbakk
2026-04-28 22:25 ` [PATCH v3 4/5] name-rev: make dedicated --annotate-stdin --name-only test kristofferhaugsbakk
2026-04-28 22:25 ` [PATCH v3 5/5] format-rev: introduce builtin for on-demand pretty formatting kristofferhaugsbakk
2026-04-29 13:41 ` Kristoffer Haugsbakk
2026-04-30 6:23 ` Kristoffer Haugsbakk
2026-04-30 9:21 ` Kristoffer Haugsbakk
2026-05-01 10:16 ` Phillip Wood
2026-05-01 18:27 ` kristofferhaugsbakk
2026-05-02 10:00 ` Phillip Wood
2026-05-05 19:27 ` Kristoffer Haugsbakk
2026-05-03 19:19 ` Junio C Hamano
2026-05-07 19:34 ` [PATCH v4 0/5] " kristofferhaugsbakk
2026-05-07 19:34 ` [PATCH v4 1/5] name-rev: wrap both blocks in braces kristofferhaugsbakk
2026-05-07 19:34 ` [PATCH v4 2/5] name-rev: run clang-format before factoring code kristofferhaugsbakk
2026-05-07 19:34 ` [PATCH v4 3/5] name-rev: factor code for sharing with a new command kristofferhaugsbakk
2026-05-07 19:34 ` [PATCH v4 4/5] name-rev: make dedicated --annotate-stdin --name-only test kristofferhaugsbakk
2026-05-07 19:34 ` [PATCH v4 5/5] format-rev: introduce builtin for on-demand pretty formatting kristofferhaugsbakk
2026-05-08 13:25 ` Kristoffer Haugsbakk
2026-05-11 13:25 ` Kristoffer Haugsbakk
2026-05-11 15:45 ` [PATCH v5 0/5] " kristofferhaugsbakk
2026-05-11 15:45 ` [PATCH v5 1/5] name-rev: wrap both blocks in braces kristofferhaugsbakk
2026-05-11 15:45 ` [PATCH v5 2/5] name-rev: run clang-format before factoring code kristofferhaugsbakk
2026-05-11 15:45 ` [PATCH v5 3/5] name-rev: factor code for sharing with a new command kristofferhaugsbakk
2026-05-11 15:45 ` [PATCH v5 4/5] name-rev: make dedicated --annotate-stdin --name-only test kristofferhaugsbakk
2026-05-11 15:45 ` [PATCH v5 5/5] format-rev: introduce builtin for on-demand pretty formatting kristofferhaugsbakk
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d3813f1d-174b-4d1b-b0c5-c6a8db260f6c@app.fastmail.com \
--to=kristofferhaugsbakk@fastmail$(echo .)com \
--cc=code@khaugsbakk$(echo .)name \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox