Subject: Re: [PATCH v10 2/3] interpret-trailers: add own-identity option

From: Junio C Hamano <[email-masked]>
Date: Fri, 19 Mar 2021 23:53:09 -0700

ZheNing Hu <[email-masked]> writes:

> In actually ,
>
> @@ -1071,6 +1071,7 @@ static const char *find_author_by_nickname(const
> char *name)
>                 strbuf_release(&buf);
>                 format_commit_message(commit, "%aN <%aE>", &buf, &ctx);
>                 clear_mailmap(&mailmap);
> +               reset_revision_walk();
>                 return strbuf_detach(&buf, NULL);
>         }
>
> then we can reuse this function.
>
> But I think I can give find_author_by_nickname another arg for choice if we want
>  `reset_revision_walk()`.

That is half fixing and half breaking.  It would allow us to call
the helper number of times as long as there is no other revision
traversal is in progress; calling reset_revision_walk() would mean
any and all revision traversal in progress will be broken.  So we
cannot use the helper to tweak each and every commit we encounter
while running "git log", for example.  Imagine adding an option to
"git format-patch" to allow each commit it formats to be tweaked by
adding "--trailers=foo:@ZheNing" and the like.  There is one primary
traversal that is used to list which commit to format in what order,
and every time that primary traversal yields a commit, if we run
find_author_by_nickname(), we end up initiating another traversal,
and then by calling reset, we clear all object flags that are used
for revision traversal, thereby breaking the primary traversal.

The only safe way to introduce a generally usable helper (without
rewriting the revision traversal machinery) is to spawn a subprocess
and do an equivalent of find_author_by_nickname() in it.

A standalone "interpret-trailers" command, as long as it won't do
any other revision traversal, would not have such a problem, and
calling reset every time find_author_by_nickname() is called may be
sufficient.  The only thing I care about is *not* to pretend that
find_author_by_nickname() plus reset() is the generally reusable
helper function and advertise it as such, which will mislead future
developers into misusing the function in a context they shouldn't
(i.e. while they are performing their own revision traversal).

Thanks.