On 23/12/20 6:46 pm, Aditya wrote:

On 22/12/20 4:23 pm, Lukas Bulwahn wrote:

quoted

On Tue, Dec 22, 2020 at 11:34 AM Dwaipayan Ray [off-list ref] wrote:

quoted

On Tue, Dec 22, 2020 at 3:55 PM Aditya [off-list ref] wrote:

quoted

On 22/12/20 1:17 am, Dwaipayan Ray wrote:

quoted

quoted

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.

quoted

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

quoted

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?

Good discussion. Consider that the first users will fight with the installation.
So, the tool should carefully check if the environment is set up
before trying to do any steps; otherwise, it might be in some strange
intermediate stage and it requires a lot of scripting to roll back
operations etc.

Also, developers do read README; so, any hints there are always helpful.

quoted

quoted

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.

Well, the developer that has a patch series would want to have the
stylistic patches to just fold into their original patch order.

So, you have two reasonable alternatives:

1. You simply create some diff commits on top of the patch series,
i.e., one diff commit for each patch in the series. That allows the
developer to review the format change for each patch and then, the
developer can simply squash the stylistic cleanup on the appropriate
patch in the series.
2. You simply "automate" that workflow in option 1, where you generate
new patches with the format patch applied, just as Aditya suggested.

For now, as we are still experimenting, I am in favor of 1; simply
because it is easier to review for now, when somebody applies it for
testing. Option 2 is probably good once it works reliably and we have
trust in the formatting setup.

Hi Lukas

I and Dwaipayan have been working on clang-format script. We have
refined it to a great extent. Now it is not dependent on version
specific clang-format-diff as well, as was the case earlier.
I have also tested it with patch series, and it works fine.

All the formatted changes(with patch commits) are made to the test
branch, while the original branch is left untouched.
We have also managed to handle command errors and break the script
execution at relevant places.

The file and the steps to be followed can be found here:
https://github.com/raydwaipayan/apply-patch/tree/dev

I have also added test patches for easier testing.
You can follow these steps for quicker testing of script:
https://github.com/raydwaipayan/apply-patch/tree/dev#steps-to-test-the-script

The readme has also been updated correspondingly.

Please test it and let us know your views on it :)

I think one of the limitations currently is that
clang-format-fixes.diff gets overwritten and just tells the diff wrt
last patch.
I am working on it. Probably, I'll be able to fix this by tomorrow.

Thanks
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees