Re: [PATCH v1] alias: support UTF-8 characters via subsection syntax

On Mon, Feb 09, 2026 at 11:01:15PM +0100, Jonatan Holmgren wrote:

Thanks, this mostly looks quite good. I have a few comments below that range from a few small bugs to project-specific gotchas to style/readability suggestions.

Usually patches do not fill out their own release notes, as it would create many conflicts when merging topics in arbitrary order. Instead, the maintainer generally writes up a blurb that goes in the merge commits and is eventually placed in the release notes by script. Suggesting a blurb does save work for the maintainer, but it should go in the cover letter (so for a single patch, after the "---" lines).

There's a bit on this in Documentation/SubmittingPatches, under the section "the-topic-summary".

Yay, I was happy to see nice comprehensive documentation here. And you do not seem to have gotten caught by any of the (many) formatting gotchas.

The use of literal backticks here makes sense, and if you haven't disabled it with a build-time knob, they should be shown in bold. Curiously in my build of the docs this doesn't quite work, and I get "git h" in bold, but "ämta" unbolded. Weird, and presumably a bug in asciidoc (the generated docbook xml shows the same thing, as does html). I get the same with asciidoctor, too. I'm not sure if it is worth working around it (or even how we would do so).

OK, so versus what I showed earlier, we are keeping an is_subsection flag in order to differentiate the cases. And then we use that below...

...to decide whether to be case-insensitive or not. Makes sense.

I was going to suggest some simplifications here: if is_subsection is false, then we know that cmd is a NUL-terminated string, and we could just use strcasecmp() as before. And in fact, with that conditional we do not need the "cmd" aliasing at all. Which also means we don't need a separate flag to distinguish the cases.

You could just write:

  if (cmd) {
	match = (strlen(data->alias) == cmd_len &&
	         !strncmp(data->alias, cmd, cmd_len));
  } else {
	match = !strcasecmp(data->alias, subkey);
  }

But I guess we do use the redirection in the listing path later anyway:

So maybe it is better as-is. I dunno. I have a feeling this would all need even more refactoring even if we ever added any more alias.foo.* config entries. So we can probably just leave it until then for more rearranging.

One thing I did find a little funny is the use of the word "subkey" for one variable. Usually we just call this the "key", but that name is annoyingly already taken by the function parameter (well before your patch). Usually we call that "var" instead. I don't know if it's worth changing or not. It's probably only confusing to me because I've stared at so many other Git config callback functions.

Is there any reason to add a new script here, and not just put these in the existing t0014? If you were bailing from the script when the prereq was not met, we'd need a separate script. But since you are using per-test prereqs, they can just exist alongside the other alias tests.

And also, you can't have two scripts with the same number. ;) Running "make test" should complain about that (I'll assume you ran your tests, but manually as ./t0014).

This is a slight mis-use of set_prereq. You should just need to return success from the lazy-prereq function, and it will be set automatically. So more like:

  test_lazy_prereq UTF8_LOCALE '
	test_have_prereq !MINGW
  '

At which point I wonder if there is much value in having UTF8_LOCALE at all, and not just putting "!MINGW" in each of the tests.

Beyond that, my larger question is: what are we asking of the platform and why does MINGW not work? We do not care about actual utf8 filesystem support, since these are just aliases. As long as the config file can store them and we can pass them on the command-line, that should be enough.

Usually we'd just define these in the tests that use them, which keeps each individual test a bit more self-contained. So more like:

  test_expect_success 'UTF-8 alias with Swedish characters' '
	test_config alias."förgrena".command branch &&
	git förgrena >output &&
	...
  '

But we use them in the listing test, too, so they are each used twice. I dunno. It might make sense for the listing test to just define its own expected set.

So I see we're aliasing "branch" here, which gives us this complicated regex for checking the output. Would something like "!echo ran my alias" make the test easier to understand?

This one needs the UTF8_LOCALE prereq, too (since we'd only have set up those aliases if we had it).

Is doing --get-regexp here all that interesting? It's not testing your new code at all, and would have passed already. I expected us to look at the output of "git help -a" to see that the entries are listed.

And trying that with your patch:

  ./git -c alias.föo.command=branch help -a

shows some possible issues:

  - we show it as "föo.command"

  - the alignment with other aliases is not quite right. Probably it is
    using a byte-count instead of utf8_strwidth()

I was puzzled that we would not parse it as föo correctly, since your patch modified the listing code. It looks like help.c has its own separate parser for this. Yuck. :( I think this also means that "git help föo" will fail.

So probably we want a preparatory patch to teach help.c to rely on list_aliases() and alias_lookup() from alias.[ch].

This probably is covered elsewhere already, but OK. :)

Nice basic test without utf8. Good.

Good thinking.

And these are both very nice to see.

-Peff