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

[ZheNing Hu <adlternative@gmail.com>]

Junio C Hamano <gitster@pobox.com> 于2021年3月20日周六 下午2:53写道：
>
> ZheNing Hu <adlternative@gmail.com> 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.
>

I understand what you meaning. I may not be very thorough in considering
 the potential problems. This is as dangerous as calling `list_del` in the
 middle of `list_for_each`.

> 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.
>

If I didn't notice what you mentioned later, I might think about it
for a long time.
Execute a child process specifically to execute `find_author_by_nickname()`,
which feels a bit wasteful.

> 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.

After listening to your previous explanation, I agree with this view.
`interpret-trailers` alone as a process, as long as it does not traverse
the revision externally, It is safe for us to call `reset_revision_walk()`
after `find_author_by_nickname()` inside interpret-trailers.

Thank you for your thoughts and answers.