To be consistent with git_mailmap_from_repo, a function name git_mailmap_from_buffer might in fact be favorable. This is only a proposal, the name is fine as is.

So you're simply returning either the original name/email or the mailmap-allocated ones? If so, you should state that lifetime of the out parameters is tied to mailmap and name/email

Can either name or email be NULL?

No, although theoretically that could be handled.

Otherwise an error code?

Otherwise an error code?

You should probably free the blame before freeing the repo

I've seen this several times now, with overly many newlines. We usually don't use that many newlines throughout our codebase. The only thing we try to stick to is a maximum line length of 80 characters

You should verify the return code with cl_git_pass

You shouldn't use cl_check_pass but cl_git_pass

You should probably remove all these hooks just to keep the churn as small as necessary

I didn't notice this was returning void at the first pass. I think you should return an error code here -- while the function cannot fail right now, you never know how you/we'll have to extend that function in the future. So returning error codes right now and having all callers check for failures is the right thing to do.

This can probably be pulled up into the parser interface itself.

I think an interface git_parse_advance_char(&c, ctx, 1); would be nice to have. Advance one character and put the next one into c.

Multi-line comments should start out with an empty /*:

/*
 * Parse a...
 */

if (...) {
    giterr_set(GITERR_MAILMAP, "could not parse real name");
    return -1;
}

You'd have to define GITERR_MAILMAP, but I think it can be helpful to detail what's wrong here

We always ignore these parser errors & just skip the line (hence the potential desire for warnings). Should we still set giterr if we won't produce a visible error code?

I realized that a bit later and thought to have deleted that comment. Existing code is fine as-is.

You can just use strlen(TEST_MAILMAP) here, which is a bit more obvious. The compiler should do it's thing and optimize that out

I don't think this one is valid. At least git-check-mailmap does not mention this syntax

git's documentation doesn't mention this format, but when I was testing mailmap, IIRC this format is supported.

Okay, fair enough

I'd put this one into a separate test. A test should only check one thing

I'd love if you'd validate all four entries.

struct {
    char *real_name;
    char *real_mail;
    char *replace_name;
    char *replace_mail;
} expected[] = {
    { "Foo bar", "foo@bar.com", "foo@baz.com" },
   ...
};

for (i = 0; i < git_mailmap_entry_count(mailmap); i++) {
    entry_byindex(mailmap, i);
   assert_all_things();
}

There's cl_assert_equal_s, for this one and the following tests

I'm a bit unsure about this function. git_mailmap_resolve is in fact nearly the exact same thing, with the only difference being that this one here uses signatures instead of strings. So maybe move this into mailmap.h and call it git_mailmap_resolve_signature?

It's not necessarily parsed in case you've manually added entries or in case you've just allocated it, without parsing from anywhere. So I'd just drop "parsed" here

As this interface is external, we should probably not use a buffer here but a plain const char * pointer with an (optional) size_t len. In case len is 0, we should then fall back to use strlen. Using buffers would only be a nuisance for library users, as we don't expose most of its interface.

This is all duplicated code. Just use git_mailmap_add_entry

We do allocate data, but we detach the buffers so that they don't own it anymore. Slight difference ;)

spec is a bit weird. rev or revision would probably be more obvious

I'd say explicit initialization would be a bit nicer here. All struct members are char pointers, so in the unlikely case we ever rearrange its members, this would silently continue to work. So just zeroing out and then explicitly assigning to the fields would be a bit nicer.

This is unexpected. We just silently abort the test, but in fact we should always exactly know what to expect (and error if g_blame is not set)

Same here

"mailmap_helpers.h" is a bit weird, as there are no helpers in here. It's more like data, so "mailmap_testdata.h" would fit better

I'm sorry for the last-minute review, but I don't love the EEXISTS return here. That really makes it sound like an error case, not a success. Do you think that "replaced an entry" is really such a valuable state to know? If so, what do you think about an out-param that indicates that instead of overloading the return value? Curious what @pks-t thinks.

The reasoning behind this decision was to match the git_vector APIs here. Most consumers of the mailmap API will probably just get it from a git_repository which just ignores errors like this.

I'd probably be fine with just not communicating whether or not an entry was replaced entirely.

Maybe an int force parameter (instead of a int *replaced) ?

I'd probably be fine with just not communicating whether or not an entry was replaced entirely.

I'm +1 on this.

This is not a pattern that we have been using - our APIs either take a NUL-terminated string or a buffer with length. (And if it's the latter, then 0 really means 0.) I think it would be confusing to have an API that differs from this pattern.

I don't have a strong opinion on which way this should go - it seems like we should just take a buf? I'm happy either way but the 0 means we do the strlen doesn't align with the rest of our APIs.

It used to take a buf, and I changed it in an earlier patch. I'm fine with switching back to that if it's what we want to do.

I wonder how much utility there is in making this a public method? It seems like most uses would prefer the git_mailmap_from_buffer. I'd be perfectly happy seeing this be an internal API.

If it does stay public (which is fine, if you have a use for it or think others will) then I do think that the name does not capture exactly what it does (at least when viewed through the lens of the names of our current APIs - add_buffer sounds like it will add a buffer, not add from a buffer).

I would propose git_mailmap_parse? Also, the same notes about the 0 len apply as with git_mailmap_from_buffer.

Sure, I'm fine with making this internal. Not super attached to any of the public APIs other than the from_repo one.