[Linux-kernel-mentees] POC script to apply patches into a test-branch and run clang-format

Dwaipayan Ray dwaipayanray1 at gmail.com
Tue Dec 22 10:34:14 UTC 2020


On Tue, Dec 22, 2020 at 3:55 PM Aditya <yashsri421 at gmail.com> wrote:
>
> On 22/12/20 1:17 am, Dwaipayan Ray wrote:
> >> Okay, I downloaded and tried it:
> >>
> >> perl ./scripts/applypatch.pl --clang-format test.patch
> >> --branch=test-clang-format-bfq-cgroup
> >>
> >> but it failed:
> >>
> >> git checkout test-clang-format-bfq-cgroup
> >> Already on 'test-clang-format-bfq-cgroup'
> >> git am $patchtest.patch
> >> fatal: could not open '.patch' for reading: No such file or directory
> >> Clang formatted patchfile generated at:
> >> test.patch.EXPERIMENTAL-clang_format-fixes
> >> git checkout test-clang-format-bfq-cgroup
> >> Already on 'test-clang-format-bfq-cgroup'
> >>
> >> How about setting up a minimal travis.ci check that runs this command
> >> on a small git repository with one file and one patch on top for
> >> testing purposes?
> >>
> >> The script does not have error handling yet, so the next steps are
> >> just executed blindly even if the one before failed and is really
> >> required for the next one to do the right thing.
> >>
> >> I generally like the script, let us work a bit on tuning it for proper
> >> error handling, add some basic tests and then discuss it with Miguel.
> >>
> >> Lukas
> >>
> >
> > Hello Lukas,
> > Sorry for being late on this update. I have my college exams going on
> > right now, so that caused a bit of delay.
> >
> > Nevertheless I have applied some changes along with some error handling fixes.
> > The script is available here:
> > https://github.com/raydwaipayan/apply-patch
> >
> > The script has the following options now:
> > usage: $P [options] patchfile(s)
> > Options:
> > --branch=<branch-name> => test branch name
> > --format => Use clang format
> > --clang-format-diff=<s> => clang-format-diff path
> > --help => show this help information
> > --root=<root_dir> => root dir location
> >
> > By default script searches for "clang-format-diff.py" in path. If it goes
> > by any other name that should be supplied using
> > --clang-format-diff="FULL_PATH_OF_EXECUTABLE"
> >
> > I ran the script like this:
> > $ ./scripts/applypatch.pl
> > 0001-scripts-mod-cleanup-a-few-checkpatch.pl-warnings.patch --format
> >
> > Expected output:
> > git checkout -b test-master
> > Switched to a new branch 'test-master'
> > git am 0001-scripts-mod-cleanup-a-few-checkpatch.pl-warnings.patch
> > Patch applied successfully:
> > 0001-scripts-mod-cleanup-a-few-checkpatch.pl-warnings.patch
> > Running clang-format
> > Diff written to clang-format-fixes.diff
> > git checkout master
> > Switched to branch 'master
> >
> > Lukas, could you check if this script works on your system as expected?
> >
> > Also Aditya, I think the latest clang-format-diff goes by the name
> > 'clang-format-diff-8' on debian based systems. While in my arch based one
> > it goes by 'clang-format-diff.py'. Can you verify whether the script works
> > directly or is the executable path flag needed?
> >
>
> For debian based system, the user will have to add
> --clang-format-diff="/usr/bin/clang-format-diff-8".
>
I think that is currently an extra step for the user. Maybe we can
target most used flavours and fill the binary location automatically.

> A few more improvements can be made. For eg., the script should exit
> if the patch isn't applying. Currently, it continues a step further
> and stops after giving error: "Error: This script requires
> clang-format-diff to be installed.".
> Also, this puts the git status in mid of git am, which needs to be reset.
>
Yeah maybe instead of die we can just return by saying "clang-format won't
apply".

> Maybe we can improve the error message by displaying them about
> "--clang-format-diff" option and giving install options in the error
> message. This should make the script easier to be used.
>

In that case we may have to give instructions for all the available
distributions. Maybe we can point them to the LLVM page?

> One more improvement I guess we can make with the .diff file, by
> renaming .diff after patchfile name. This way it won't be overwritten
> each time and also be in consistency with checkpatch.pl --fix.
>

About this I was thinking we should avoid creating individual diffs.
Since in the future the script may apply a patch series too, we can
just generate a single diff and name accordingly. That way in a single
step the committer can apply all fixes.

> I'll try to make these improvements and send a PR or a patch to you,
> where we can discuss any changes.
>
Thanks Aditya, please do.

Regards,
Dwaipayan.


More information about the Linux-kernel-mentees mailing list