Siddharth Asthana <siddharthasthana31@xxxxxxxxx> 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".
> +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...@xxxxxxxxx> 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.
