Hi Phillip, ---- On Thu, 08 May 2025 22:17:17 +0800 Phillip Wood <phillip.wood123@xxxxxxxxx> wrote --- > Hi Li > > On 06/05/2025 13:58, Li Chen wrote: > > From: Li Chen <chenl311@xxxxxxxxxxxxxxx> > > > > Implement a new `--trailer <text>` option for `git rebase` > > (support merge backend only now), which appends arbitrary > > trailer lines to each rebased commit message. Reject early > > if used with the apply backend (git am) since it lacks > > message‑filter/trailer hook. Automatically set REBASE_FORCE when > > any trailer is supplied. > > I think this is a reasonable idea but unfortunately I think the trailer > API needs improving so that the implementation > > (a) Checks the trailers given on the command-line before the user edits > the todo list. That way we reject invalid trailers and exit before the > user has spent any effort editing the todo list. > > (b) Does not fork another process to add the trailers. Without this the > performance is going to suffer. Hopefully it wont be too difficult to > modify the existing code to take a struct strbuf and a list of trailers > to append to it. > > (c) Only adds the trailers on the commandline. I'm a bit confused by the > various trailer config options - the man page reads to me like "git > interpret-trailers" can add missing trailers that are configured but not > passed on the commandline. > > The changes to the trailer api should be made in one or more preparatory > commits before adding support for --trailer to "git rebase" Thanks a lot for the detailed feedback and for outlining the gaps in the current trailer API – it really helps me see the next steps more clearly. I have one questions: Who should take the API work? I’m very happy to roll up my sleeves and prototype the improvements you described (reject invalid trailers up‑front, add an in‑process API that works on a struct strbuf, and ensure only the CLI‑supplied trailers are added). That would give me a chance to contribute a bit more deeply to Git. Of course, if you already have something in mind and would rather drive that part yourself, I’m equally happy to wait and re‑base my series on top of it. Please let me know which you prefer. > I've left some comments on the changes to builtin/rebase.c and the > tests, I've skipped the changes to sequencer.c for now as they'll have > to be updated to avoid forking "git interpret-trailers" Thanks for all your great reviews! I'll address all your reviews in next version.
