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

Junio C Hamano [off-list ref] 于2021年3月20日周六 下午2:53写道：

ZheNing Hu [off-list ref] writes:

quoted

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.