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

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

[Dwaipayan Ray]

Hi,
In continuation to our previous discussion about ways to make checkpatch
apply patches into a test brach, and probably run clang-format to generate
a diff, I wrote a small script which does the same.

It can be found here:
https://gist.github.com/raydwaipayan/6b8d10470a163f7ae501c703bc27a83f

The script is still in it's infancy and more configuration options and support
for patch series is pending.

Lukas, it would be great to have your review on this.

Thank you,
Dwaipayan.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Aditya]

On 5/12/20 4:40 pm, Dwaipayan Ray wrote:
> Hi,
> In continuation to our previous discussion about ways to make checkpatch
> apply patches into a test brach, and probably run clang-format to generate
> a diff, I wrote a small script which does the same.
> 
> It can be found here:
> https://gist.github.com/raydwaipayan/6b8d10470a163f7ae501c703bc27a83f
> 
> The script is still in it's infancy and more configuration options and support
> for patch series is pending.
> 
> Lukas, it would be great to have your review on this.
> 
> Thank you,
> Dwaipayan.
> 

Hi Dwaipayan
Nice work!
I was able to run the script, with few changes to the script. I'd add
my suggestions about it:

- The check with: "(! -e $git_clang_format_file)" doesn't work
correctly for me. Although the command runs properly, after I comment
out this if-section. Probably if we just check for a certain version
of git which added "git clang-format" for the first time, in users
system, it might do the trick.

- Also "git branch --show-current" is compatible only with Git 2.22 or
above (For eg, mine is 2.17.1). Maybe replace it with "git rev-parse
--abbrev-ref HEAD"?

Also, I created a patch for testing the script. Here it is:

From 842e18ecc9500fbd2e5d08c4a2b3e7e3c275b6bc Mon Sep 17 00:00:00 2001
From: Aditya Srivastava <yashsri421@gmail.com>
Date: Sat, 5 Dec 2020 19:38:03 +0530
Subject: [PATCH] temp

---
 block/sed-opal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index daafadbb88ca..f7929eb67f37 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -2322,7 +2322,7 @@ static int opal_add_user_to_lr(struct opal_dev *dev,
 	mutex_lock(&dev->dev_lock);
 	setup_opal_dev(dev);
 	ret = execute_steps(dev, steps, ARRAY_SIZE(steps));
-	mutex_unlock(&dev->dev_lock);
+mutex_unlock(&dev->dev_lock);

 	return ret;
 }
-- 
2.17.1


Now this patch gets applied to a new "test-xyz" branch, but it is not
formatted, ie the added line remains un-indented. Is this expected
behaviour from the script?

I have also tried out something similar (using clang-format-diff
though). I'll try to add it in your script and send.

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

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

[Dwaipayan Ray]

Hey Aditya,

> - The check with: "(! -e $git_clang_format_file)" doesn't work
> correctly for me. Although the command runs properly, after I comment
> out this if-section. Probably if we just check for a certain version
> of git which added "git clang-format" for the first time, in users
> system, it might do the trick.
>

True, we need to track that version and terminate the script. Although I
am curious why it didn't work in your case. Is that file non existent on your
system?

> - Also "git branch --show-current" is compatible only with Git 2.22 or
> above (For eg, mine is 2.17.1). Maybe replace it with "git rev-parse
> --abbrev-ref HEAD"?

Yes, that version compatibility might be a problem.
The other option is nice, but again it causes a certain conflict when the
HEAD itself is detached.

In a detached HEAD state, "git rev-parse --abbrev-ref HEAD" just returns
"HEAD".

> diff --git a/block/sed-opal.c b/block/sed-opal.c
> index daafadbb88ca..f7929eb67f37 100644
> --- a/block/sed-opal.c
> +++ b/block/sed-opal.c
> @@ -2322,7 +2322,7 @@ static int opal_add_user_to_lr(struct opal_dev *dev,
>         mutex_lock(&dev->dev_lock);
>         setup_opal_dev(dev);
>         ret = execute_steps(dev, steps, ARRAY_SIZE(steps));
> -       mutex_unlock(&dev->dev_lock);
> +mutex_unlock(&dev->dev_lock);
>
>         return ret;
>  }
> --
> 2.17.1
>
>
> Now this patch gets applied to a new "test-xyz" branch, but it is not
> formatted, ie the added line remains un-indented. Is this expected
> behaviour from the script?
>

Currently it should only output the diff. The --clang-format flag is to be used
though. Maybe the diff could be written into a clang-format-fixes.patch file and
apply it later on.
That way the user has more control over what gets changed.

> I have also tried out something similar (using clang-format-diff
> though). I'll try to add it in your script and send.
>

Yep sure enough. The problem was somehow I didn't have it on
my system. So I couldn't use that. But it should be easy to swap
in git-clang-format for clang-format-diff.

But anyways thanks for testing it out!
A lot more to be done but let's see where it goes :)

Thank you,
Dwaipayan.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Aditya]

On 5/12/20 9:24 pm, Dwaipayan Ray wrote:
> Hey Aditya,
> 
>> - The check with: "(! -e $git_clang_format_file)" doesn't work
>> correctly for me. Although the command runs properly, after I comment
>> out this if-section. Probably if we just check for a certain version
>> of git which added "git clang-format" for the first time, in users
>> system, it might do the trick.
>>
> 
> True, we need to track that version and terminate the script. Although I
> am curious why it didn't work in your case. Is that file non existent on your
> system?
> 
>> - Also "git branch --show-current" is compatible only with Git 2.22 or
>> above (For eg, mine is 2.17.1). Maybe replace it with "git rev-parse
>> --abbrev-ref HEAD"?
> 
> Yes, that version compatibility might be a problem.
> The other option is nice, but again it causes a certain conflict when the
> HEAD itself is detached.
> 
> In a detached HEAD state, "git rev-parse --abbrev-ref HEAD" just returns
> "HEAD".
> 
>> diff --git a/block/sed-opal.c b/block/sed-opal.c
>> index daafadbb88ca..f7929eb67f37 100644
>> --- a/block/sed-opal.c
>> +++ b/block/sed-opal.c
>> @@ -2322,7 +2322,7 @@ static int opal_add_user_to_lr(struct opal_dev *dev,
>>         mutex_lock(&dev->dev_lock);
>>         setup_opal_dev(dev);
>>         ret = execute_steps(dev, steps, ARRAY_SIZE(steps));
>> -       mutex_unlock(&dev->dev_lock);
>> +mutex_unlock(&dev->dev_lock);
>>
>>         return ret;
>>  }
>> --
>> 2.17.1
>>
>>
>> Now this patch gets applied to a new "test-xyz" branch, but it is not
>> formatted, ie the added line remains un-indented. Is this expected
>> behaviour from the script?
>>
> 
> Currently it should only output the diff. The --clang-format flag is to be used
> though. Maybe the diff could be written into a clang-format-fixes.patch file and
> apply it later on.
> That way the user has more control over what gets changed.
> 
Hi Dwaipayan,
I have modified your script with clang-format-diff here:
https://gist.github.com/AdityaSrivast/0fc57fbd49db8c05c7b1154d4289174a

As we wanted, it generates a modified patch which is clang formatted,
with suffix ".clang-format.EXPERIMENTAL". It also modifies the files
with clang-format in test-branch, which was desired with our
"--clang-format" flag to this script.

I think the code probably will need to be refactored once we are sure
that we are on the right track and the script does the desired job.

Lukas, what do you think?

Thanks
Aditya


>> I have also tried out something similar (using clang-format-diff
>> though). I'll try to add it in your script and send.
>>
> 
> Yep sure enough. The problem was somehow I didn't have it on
> my system. So I couldn't use that. But it should be easy to swap
> in git-clang-format for clang-format-diff.
> 
> But anyways thanks for testing it out!
> A lot more to be done but let's see where it goes :)

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

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

[Lukas Bulwahn]

On Sun, Dec 6, 2020 at 11:49 AM Aditya <yashsri421@gmail.com> wrote:
>
> On 5/12/20 9:24 pm, Dwaipayan Ray wrote:
> > Hey Aditya,
> >
> >> - The check with: "(! -e $git_clang_format_file)" doesn't work
> >> correctly for me. Although the command runs properly, after I comment
> >> out this if-section. Probably if we just check for a certain version
> >> of git which added "git clang-format" for the first time, in users
> >> system, it might do the trick.
> >>
> >
> > True, we need to track that version and terminate the script. Although I
> > am curious why it didn't work in your case. Is that file non existent on your
> > system?
> >
> >> - Also "git branch --show-current" is compatible only with Git 2.22 or
> >> above (For eg, mine is 2.17.1). Maybe replace it with "git rev-parse
> >> --abbrev-ref HEAD"?
> >
> > Yes, that version compatibility might be a problem.
> > The other option is nice, but again it causes a certain conflict when the
> > HEAD itself is detached.
> >
> > In a detached HEAD state, "git rev-parse --abbrev-ref HEAD" just returns
> > "HEAD".
> >
> >> diff --git a/block/sed-opal.c b/block/sed-opal.c
> >> index daafadbb88ca..f7929eb67f37 100644
> >> --- a/block/sed-opal.c
> >> +++ b/block/sed-opal.c
> >> @@ -2322,7 +2322,7 @@ static int opal_add_user_to_lr(struct opal_dev *dev,
> >>         mutex_lock(&dev->dev_lock);
> >>         setup_opal_dev(dev);
> >>         ret = execute_steps(dev, steps, ARRAY_SIZE(steps));
> >> -       mutex_unlock(&dev->dev_lock);
> >> +mutex_unlock(&dev->dev_lock);
> >>
> >>         return ret;
> >>  }
> >> --
> >> 2.17.1
> >>
> >>
> >> Now this patch gets applied to a new "test-xyz" branch, but it is not
> >> formatted, ie the added line remains un-indented. Is this expected
> >> behaviour from the script?
> >>
> >
> > Currently it should only output the diff. The --clang-format flag is to be used
> > though. Maybe the diff could be written into a clang-format-fixes.patch file and
> > apply it later on.
> > That way the user has more control over what gets changed.
> >
> Hi Dwaipayan,
> I have modified your script with clang-format-diff here:
> https://gist.github.com/AdityaSrivast/0fc57fbd49db8c05c7b1154d4289174a
>
> As we wanted, it generates a modified patch which is clang formatted,
> with suffix ".clang-format.EXPERIMENTAL". It also modifies the files
> with clang-format in test-branch, which was desired with our
> "--clang-format" flag to this script.
>
> I think the code probably will need to be refactored once we are sure
> that we are on the right track and the script does the desired job.
>
> Lukas, what do you think?
>

Aditya, Dwaipayan, can you set up a git repository that you use
together to work on this script?

And provide a short description of how to use the script with an
example in that repository?

Then, I am happy to try it out... and provide you more feedback.

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

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

[Dwaipayan Ray]

>
> Aditya, Dwaipayan, can you set up a git repository that you use
> together to work on this script?
>
> And provide a short description of how to use the script with an
> example in that repository?
>
> Then, I am happy to try it out... and provide you more feedback.
>
> Lukas

Hi Lukas,
Sure enough, if Aditya doesn't mind, I will put up a repo for the same.

Thank you,
Dwaipayan.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Lukas Bulwahn]

On Fri, Dec 11, 2020 at 7:24 AM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote:
>
> >
> > Aditya, Dwaipayan, can you set up a git repository that you use
> > together to work on this script?
> >
> > And provide a short description of how to use the script with an
> > example in that repository?
> >
> > Then, I am happy to try it out... and provide you more feedback.
> >
> > Lukas
>
> Hi Lukas,
> Sure enough, if Aditya doesn't mind, I will put up a repo for the same.
>

Dwaipayan, just set up a repository with your latest state that you
are currently happy with, then Aditya can fork that and put changes on
top as he might see fit and open a pull request, so you integrate them
back.

> Thank you,
> Dwaipayan.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Aditya]

On 11/12/20 11:57 am, Lukas Bulwahn wrote:
> On Fri, Dec 11, 2020 at 7:24 AM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote:
>>
>>>
>>> Aditya, Dwaipayan, can you set up a git repository that you use
>>> together to work on this script?
>>>
>>> And provide a short description of how to use the script with an
>>> example in that repository?
>>>
>>> Then, I am happy to try it out... and provide you more feedback.
>>>
>>> Lukas
>>
>> Hi Lukas,
>> Sure enough, if Aditya doesn't mind, I will put up a repo for the same.
>>
> 
> Dwaipayan, just set up a repository with your latest state that you
> are currently happy with, then Aditya can fork that and put changes on
> top as he might see fit and open a pull request, so you integrate them
> back.
> 

Hi
I have added my changes in the repo and also provided steps to run the
script in readme.

Repo Link: https://github.com/raydwaipayan/apply-patch

You can use this patch for testing. It applies on next-20201204.

From c631c1b9c4c7e3cdbf0e41b21b8fa1d34ba20aef Mon Sep 17 00:00:00 2001
From: Aditya Srivastava <yashsri421@gmail.com>
Date: Fri, 11 Dec 2020 14:19:52 +0530
Subject: [PATCH] patch for testing

This patch has been created for testing clang-format on c-file.
---
 block/bfq-cgroup.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
index b791e2041e49..8409ee064204 100644
--- a/block/bfq-cgroup.c
+++ b/block/bfq-cgroup.c
@@ -184,7 +184,8 @@ void bfqg_stats_set_start_empty_time(struct
bfq_group *bfqg)
 		return;

 	stats->start_empty_time = ktime_get_ns();
-	bfqg_stats_mark_empty(stats);
+
+bfqg_stats_mark_empty(stats);
 }

 void bfqg_stats_update_idle_time(struct bfq_group *bfqg)
-- 
2.17.1


Thanks
Aditya

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

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

[Lukas Bulwahn]

On Fri, Dec 11, 2020 at 10:16 AM Aditya <yashsri421@gmail.com> wrote:
>
> On 11/12/20 11:57 am, Lukas Bulwahn wrote:
> > On Fri, Dec 11, 2020 at 7:24 AM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote:
> >>
> >>>
> >>> Aditya, Dwaipayan, can you set up a git repository that you use
> >>> together to work on this script?
> >>>
> >>> And provide a short description of how to use the script with an
> >>> example in that repository?
> >>>
> >>> Then, I am happy to try it out... and provide you more feedback.
> >>>
> >>> Lukas
> >>
> >> Hi Lukas,
> >> Sure enough, if Aditya doesn't mind, I will put up a repo for the same.
> >>
> >
> > Dwaipayan, just set up a repository with your latest state that you
> > are currently happy with, then Aditya can fork that and put changes on
> > top as he might see fit and open a pull request, so you integrate them
> > back.
> >
>
> Hi
> I have added my changes in the repo and also provided steps to run the
> script in readme.
>
> Repo Link: https://github.com/raydwaipayan/apply-patch
>
> You can use this patch for testing. It applies on next-20201204.
>

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

> From c631c1b9c4c7e3cdbf0e41b21b8fa1d34ba20aef Mon Sep 17 00:00:00 2001
> From: Aditya Srivastava <yashsri421@gmail.com>
> Date: Fri, 11 Dec 2020 14:19:52 +0530
> Subject: [PATCH] patch for testing
>
> This patch has been created for testing clang-format on c-file.
> ---
>  block/bfq-cgroup.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
> index b791e2041e49..8409ee064204 100644
> --- a/block/bfq-cgroup.c
> +++ b/block/bfq-cgroup.c
> @@ -184,7 +184,8 @@ void bfqg_stats_set_start_empty_time(struct
> bfq_group *bfqg)
>                 return;
>
>         stats->start_empty_time = ktime_get_ns();
> -       bfqg_stats_mark_empty(stats);
> +
> +bfqg_stats_mark_empty(stats);
>  }
>
>  void bfqg_stats_update_idle_time(struct bfq_group *bfqg)
> --
> 2.17.1
>
>
> Thanks
> Aditya
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Dwaipayan Ray]

> 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?

Thank you,
Dwaipayan.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Aditya]

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

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.

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.

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.

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

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

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

[Dwaipayan Ray]

On Tue, Dec 22, 2020 at 3:55 PM Aditya <yashsri421@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.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Lukas Bulwahn]

On Tue, Dec 22, 2020 at 11:34 AM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote:
>
> On Tue, Dec 22, 2020 at 3:55 PM Aditya <yashsri421@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?
>

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.

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

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

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

[Aditya]

On 22/12/20 4:23 pm, Lukas Bulwahn wrote:
> On Tue, Dec 22, 2020 at 11:34 AM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote:
>>
>> On Tue, Dec 22, 2020 at 3:55 PM Aditya <yashsri421@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?
>>
> 
> 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.
> 
>>> 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 Dwaipayan,
I have made a PR to the repository.
These are the changes that I have made:

- check if the patch applies before applying it. If so, don't apply
the patch.
- remove the user's need to enter file path for clang-format-diff
- fix python version dependency with respect to clang-format version.
Detect required python version on the basis of clang-version.
- Do not run clang-format if patch application fails
- Update Readme accordingly.

A big improvement we have made is that we no longer require
clang-format-8 specifically. It can be any version and we'll invoke it
accordingly with corresponding python version dependency.

In addition, I feel we have overcome the flavor dependency for
clang-format-diff. This is based on my assumption that the file will
either be present as "clang-format-diff.py" or "clang-format-diff".

I haven't been able to test this though for different flavors, but I
believe this will be true as I haven't seen "clang-format-diff" being
used in any other form apart from "clang-format-diff" or
"clang-format-diff.py" in any doc online.

What do you think?

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

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

[Aditya]

On 23/12/20 6:46 pm, Aditya wrote:
> On 22/12/20 4:23 pm, Lukas Bulwahn wrote:
>> On Tue, Dec 22, 2020 at 11:34 AM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote:
>>>
>>> On Tue, Dec 22, 2020 at 3:55 PM Aditya <yashsri421@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?
>>>
>>
>> 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.
>>
>>>> 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

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

[Aditya]

On 6/2/21 1:26 am, Aditya wrote:
> On 23/12/20 6:46 pm, Aditya wrote:
>> On 22/12/20 4:23 pm, Lukas Bulwahn wrote:
>>> On Tue, Dec 22, 2020 at 11:34 AM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote:
>>>>
>>>> On Tue, Dec 22, 2020 at 3:55 PM Aditya <yashsri421@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?
>>>>
>>>
>>> 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.
>>>
>>>>> 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.

Hi Lukas
I have made necessary changes for this feature as well.
Now, for all the patches in patch series, a diff file is getting
created, and it will be stored alongside the patchfile, as
"patchfile.clang-format-fixes.diff"

This is in compliance with your suggestion here:

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

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