Re: [PATCH 6.1.y 0/1] Backporting patches with git-llm-pick

["Manthey, Norbert" <nmanthey@amazon.de>]

On Tue, 2025-09-02 at 07:48 -0400, Sasha Levin wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On Mon, Sep 01, 2025 at 03:35:58PM +0000, Norbert Manthey wrote:
> > Dear all,
> > 
> > we looked into improving commit backporting workflows. This specific commit in
> > this series caught our attention, as it states that backporting will not be
> > trivial. The commit message has this part:
> > 
> > Backport hint: this patch will have a trivial conflict applying to
> > v6.6.y, and other trivial conflicts applying to stable kernels < v6.6.
> > 
> > We want to automate backporting commits with a similar property. Therefore, we
> > build a tool git-llm-pick that simplifies the backporting commit. After
> > cherry-picking, we try to automatically detect dependency-commits. In case of
> > failure, we then try to use the patch tool. If this process fails, we then take
> > the rejected patch files, and feed their content with other context and a task
> > description to backport to an LLM. The resulting code modification is then
> > applied. In case validation is passed, a commit is created. The commit message
> > is always extended by a description of the adapted code change, and with which
> > technique a commit was applied.
> 
> Very nice!

Thanks.

> 
> I have a similar workflow, but it does a slightly different thing. My main
> issue with having LLM just fix up conflicts and commit them is that it becomes
> really hard to understand what the LLM actually did.
> 
> If you look at the generated commit text in your example message, it states
> "Removed the warning message as per the patch" which is really confusing: does
> this refer to something that the LLM changed? Is this different from what the
> original commit was trying to do? Why is this explicitly called out?

Yes, this part needs an iteration on our side. We need to at least diff the two
versions of the backport, and get the explanation for that difference.

> 
> The way I do it on my end, is that I create a "merge triangle" which contains:
> 
>   - The original commit, as is
>   - The fixup
>   - An explanation to the fixup

Thanks for sharing. We are thinking about extending the tool with another stage
to use agents. While that might work for us, other users might have access to
LLMs directly only. To get feedback early, we started there.

> 
> So using this commit as an example, it would generate:
> 
> $ git log --oneline --graph -5
> *   5d59fa7c4b981 (HEAD) Explanation
> > \
> > * 46beb491ffff2 fs/notify: fix merge conflict in fdinfo.c
> > * b5031eb894d34 fs: relax assertions on failure to encode file handles
> > /
> * f89b6e15694c1 (tag: v6.1.149, stable/linux-6.1.y) Linux 6.1.149
> * 46643021596a6 alloc_fdtable(): change calling conventions.
> 
> Which gives me "atomic" git units to work with, but still allowing for easy review.
> 
> The original patch will be committed with the merge conflict:
> 
> diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c
> index 55081ae3a6ec0..22adbf533855b 100644
> --- a/fs/notify/fdinfo.c
> +++ b/fs/notify/fdinfo.c
> @@ -50,11 +50,15 @@ static void show_mark_fhandle(struct seq_file *m, struct inode *inode)
>          f.handle.handle_bytes = sizeof(f.pad);
>          size = f.handle.handle_bytes >> 2;
> 
> +<<<<<<< HEAD
>          ret = exportfs_encode_inode_fh(inode, (struct fid *)f.handle.f_handle, &size, NULL);
>          if ((ret == FILEID_INVALID) || (ret < 0)) {
>                  WARN_ONCE(1, "Can't encode file handler for inotify: %d\n", ret);
> +=======
> +       ret = exportfs_encode_fid(inode, (struct fid *)f->f_handle, &size);
> +       if ((ret == FILEID_INVALID) || (ret < 0))
> +>>>>>>> 974e3fe0ac61d (fs: relax assertions on failure to encode file handles)
>                  return;
> -       }
> 
>          f.handle.handle_type = ret;
>          f.handle.handle_bytes = size * sizeof(u32);
> 
> And then the following commit will address that:
> 
> diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c
> index 22adbf533855b..dd5bc6ffae858 100644
> --- a/fs/notify/fdinfo.c
> +++ b/fs/notify/fdinfo.c
> @@ -50,14 +50,8 @@ static void show_mark_fhandle(struct seq_file *m, struct inode *inode)
>          f.handle.handle_bytes = sizeof(f.pad);
>          size = f.handle.handle_bytes >> 2;
> 
> -<<<<<<< HEAD
>          ret = exportfs_encode_inode_fh(inode, (struct fid *)f.handle.f_handle, &size, NULL);
> -       if ((ret == FILEID_INVALID) || (ret < 0)) {
> -               WARN_ONCE(1, "Can't encode file handler for inotify: %d\n", ret);
> -=======
> -       ret = exportfs_encode_fid(inode, (struct fid *)f->f_handle, &size);
>          if ((ret == FILEID_INVALID) || (ret < 0))
> ->>>>>>> 974e3fe0ac61d (fs: relax assertions on failure to encode file handles)
>                  return;
> 
>          f.handle.handle_type = ret;
> 
> In this way, it's easy for me to see what exactly the LLM changed, making for
> way easier review of "fuzzy" backports.
> 
> And finally, the merge commit will contain a longer LLM generated explanation
> that I can also validate. This way, I validate two things: the actual code
> change, and the LLM's reasoning. If both are sane, then it helps be build trust
> in the backport. Here's the explanation for this commit:
> 
> commit 5d59fa7c4b981ec18ac730b4d8629ec33619de6f (HEAD)
> Merge: f89b6e15694c1 46beb491ffff2
> Author: Sasha Levin <sashal@kernel.org>
> Date:   Tue Sep 2 07:28:10 2025 -0400
> 
>      Explanation
> 
>      The Problem
> 
>      The HEAD commit (b5031eb894d3) contained an improperly resolved merge conflict
>      in fs/notify/fdinfo.c.
> 
>      The conflict markers showed:
> 
>      - HEAD version: Used exportfs_encode_inode_fh() with a WARN_ONCE() assertion
>      - Incoming change: Incorrectly used exportfs_encode_fid() (non-existent
>        function) with wrong syntax
> 
>      The Resolution
> 
>      I fixed it by:
> 
>      1. Keeping the correct function: exportfs_encode_inode_fh() (not the
>      non-existent exportfs_encode_fid())
>      2. Removing the WARN_ONCE assertion: As intended by the original commit to
>      "relax assertions"
>      3. Preserving correct syntax: Using f.handle.f_handle (not f->f_handle)
> 
>      Why This is Correct
> 
>      The original commit's purpose was to relax assertions when file handle encoding
>      fails, because these assertions were incorrect and could trigger in valid
>      scenarios.
> 
>      The fix:
> 
>      - Removes the problematic WARN_ONCE() that could trigger incorrectly
>      - Keeps the proper API function that actually exists in the kernel
>      - Maintains clean error handling without warnings for legitimate failures
> 
>      The code now silently returns on encoding failures instead of triggering
>      warnings, which aligns with the commit's stated goal of relaxing assertions for
>      file handle encoding failures
> 
> At this point in my workflow, I can just squash the fix into the original
> commit and decorate the new resulting commit as needed for -stable.
> 
> > We made the tool available on github: https://github.com/awslabs/Git_llm_pick/
> > We currently use LLMs via the AWS Bedrock service, and default to the Nova Pro
> > model. The patch in this series uses the vanilla version of the tool's current
> > output when running git-llm-pick in its virtual test environment:
> 
> One note about the tool: in my experience, unless the tool can also act as an
> agent and investigate the relevant git repo (and attempt builds and run tests)
> on it's own, the results used to be very lackluster.

I agree in general. On the other hand, we want to keep the amount of work done by
the LLM or agent small. For now, we only submit a bit of context and the commit
messages. The validation is executed by the application independently of the
agent. There is no feedback loop yet, or similar -- that could all be done in the
agent-stage. We have a few more filters and limits to only process commits that
are likely to be finished successfully by an LLM.

Best,
Norbert

> 
> Without agentic access to more context/tooling, the LLM was guessing way too
> much :)
> 
> --
> Thanks,
> Sasha




Amazon Web Services Development Center Germany GmbH
Tamara-Danz-Str. 13
10243 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B
Sitz: Berlin
Ust-ID: DE 365 538 597