From: Toon Claes Date: Mon, 16 Mar 2026 19:12:37 GMT Subject: Re: [PATCH v4 1/2] sequencer: extract revert message formatting into shared function Message-ID: <87pl53si56.fsf@iotcl.com> In-Reply-To: Junio C Hamano writes: > Siddharth Asthana writes: > >> The logic for formatting revert commit messages (handling "Revert" and >> "Reapply" cases, appending "This reverts commit .", and handling >> merge-parent references) currently lives inline in do_pick_commit(). >> The upcoming replay --revert mode needs to reuse this logic. >> >> Extract all of this into a new sequencer_format_revert_message() >> function. The function takes a repository, the subject line, commit, >> parent, a use_commit_reference flag, and the output strbuf. It handles >> both regular reverts ("Revert """) and revert-of-revert cases >> ("Reapply """), and uses refer_to_commit() internally to >> format the commit reference. >> >> Update refer_to_commit() to take a struct repository parameter instead >> of relying on the_repository, and a bool instead of reading from >> replay_opts directly. This makes it usable from the new shared function >> without pulling in sequencer-specific state. I wouldn't mind if you put removing the use of `the_repository` in a separate commit. >> >> Signed-off-by: Siddharth Asthana >> --- >> sequencer.c | 78 +++++++++++++++++++++++++++++++---------------------- >> sequencer.h | 14 ++++++++++ >> 2 files changed, 60 insertions(+), 32 deletions(-) > > Relative to the previous round, sequencer_format_revert_message() > that does a bit more than sequencer_format_revert_header() we had > makes the existing code easier to follow, even though the total > codeflow amounts to the same thing. A new caller that will use the > function now has to do less. > > Also, even though this is an internal implementation detail, > changing the list of parameters refer_to_commit() takes makes it > easier to understand which part of the replay_opts structure is used > (i.e., we only care about "do we use the commit reference, or not?" > bit, and we have no interest in any other members of the struct). Patrick suggested[1] to use flags, but it makes sense to keep it simple for now. [1]: https://lore.kernel.org/git/aTZ5RrjnwJ2ZnT7A@pks.im/ > >> +/* >> + * Formats a complete revert commit message following standard Git conventions. >> + * Handles regular reverts ("Revert \"\""), revert of revert cases >> + * ("Reapply \"\""), and the --reference style. Appends "This reverts >> + * commit ." using either the abbreviated or full commit reference >> + * depending on use_commit_reference. Also handles merge-parent references. >> + */ I think you're trying to put too much in the comments here. I would suggest to be a bit more concise. Maybe something like: /* * Format a revert commit message with appropriate "Revert" or "Reapply" * prefix and "This reverts commit ." body. When use_commit_reference * is set, is an abbreviated hash with subject and date. */ >> +void sequencer_format_revert_message(struct repository *r, >> + const char *subject, >> + const struct commit *commit, >> + const struct commit *parent, >> + bool use_commit_reference, >> + struct strbuf *message); >> + >> #endif /* SEQUENCER_H */ > > OK. > -- Cheers, Toon