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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 20, 2021, 06:53 UTC
Message-ID: <xmqqzgyyjoyy.fsf@gitster.g>
In-Reply-To: <CAOLTT8T54RHPm8P=mJ3vOKZPEwpJQgnbeU3VqczWbPSk36wZcA@mail.gmail.com>

ZheNing Hu <adlternative@gmail.com> writes:

Show 15 quoted lines

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