Re: [PATCH 1/3] ident: move commit_rewrite_person() to ident.c

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 30, 2022, 23:22 UTC
Message-ID: <xmqqo7y9buu9.fsf@gitster.g>
In-Reply-To: <20220630142444.651948-2-siddharthasthana31@gmail.com>

Siddharth Asthana <siddharthasthana31@gmail.com> writes:

> +/*
> + * Given a commit or tag object buffer, replaces the person's

I do not think you should refer to "or tag" at this point. It is not designed to be used by anything other than a commit, and nobody passes a tag in the original code, or even after this patch is applied.

> + * (author/committer/tagger) name and email with their canonical
> + * name and email using mailmap mechanism. Signals a success with

"using" -> "using the"

> + * 1 and failure with a 0.

I do not think 0 signals a failure. If it makes changes, then the function returns a non-zero value. 0 only indicates "we made no modification to the buffer".

Show 11 quoted lines

> +int commit_rewrite_person(struct strbuf *buf, const char *what, struct string_list *mailmap)
> +{
> +	char *person, *endp;
> +	size_t len, namelen, maillen;
> +	const char *name;
> +	const char *mail;
> +	struct ident_split ident;
> +
> +	person = strstr(buf->buf, what);
> +	if (!person)
> +		return 0;

I do not think it is a good idea to expose this as a public function as is, especially given that what you have to pass in "what" is a bit awkward. The function is designed to find and replace an ident string in the header part, and the way it avoids a random occurence of author Siddharth Asthana <si...@gmail.com> in the text, not nececessarily in the header, is by insisting "author" to appear at the beginning of line by passing "\nauthor " as "what".

Also as you can see, the implementation does not make *any* effort to limit itself to the commit/tag header by locating the blank line that appears after the header part and stopping the search there. That may not be *your* bug, but should be fixed before exposing it as a public function and inflicting its bug to more callers.

Also the interface forces the caller to make multiple calls if it wants to rewrite idents on multiple headers. It shouldn't be the case.

To support the existing caller better, it should be updated to

 * take one or more header names (like "author", "committer"), make
   a single pass in the input buffer to locate these headers and
   replace idents on them;

 * stop at the end of header, ensuring that nothing in the body of
   the commit object is modified.

Alternatively, it may not be a bad idea to simplify the interface so that _all_ headers are subject to be rewritten, without any need to the "what" parameter. If you want to go that route, then you would probably have a loop over buf->buf that iterates one line at a time, stopping at the first empty line that denotes the end of header. For each line, you'd skip to the first SP that is past the header name, see if split_ident_line() thinks the line it got indeed has an ident, and use map_user() to the ident if it found. Do that for each line and you are done when you reached the end of the header.

Once we fix the external interface like so while being a static function inside revision.c and update its two callers (which will become just a single caller), we can move and expose it as a public function.

Thanks.