[PATCH] tools/mvtest: ensure testcase is executable (755)

[PATCH] tools/mvtest: ensure testcase is executable (755)

[Shiyang Ruan]

Some test cases lack executable permission ('x'). Before running each
test case, `./check` checks and grants them 'x' permission. However,
this always leads to a dirty git repo. And the absence of 'x' permission
in test cases is often overlooked during reviews.

Since maintainers use mvtest to assign new case, add this change for
convenience of maintainers.

Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
---
 tools/mvtest | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/mvtest b/tools/mvtest
index 99b154142..e839f0256 100755
--- a/tools/mvtest
+++ b/tools/mvtest
@@ -28,6 +28,8 @@ append() {
 test "${src}" != "${dest}" || die "Test \"${src}\" is the same as dest."
 test -e "tests/${src}" || die "Test \"${src}\" does not exist."
 test ! -e "tests/${dest}" || die "Test \"${src}\" already exists."
+# make sure testcase is executable
+test `stat -c '%a' tests/${src}` == 755 || chmod 755 "tests/${src}"
 
 sid="$(basename "${src}")"
 did="$(basename "${dest}")"
-- 
2.42.0

Re: [PATCH] tools/mvtest: ensure testcase is executable (755)

[Zorro Lang]

On Wed, Sep 06, 2023 at 03:13:49PM +0800, Shiyang Ruan wrote:
> Some test cases lack executable permission ('x'). Before running each
> test case, `./check` checks and grants them 'x' permission. However,
> this always leads to a dirty git repo. And the absence of 'x' permission
> in test cases is often overlooked during reviews.
> 
> Since maintainers use mvtest to assign new case, add this change for
> convenience of maintainers.
> 
> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
> ---
>  tools/mvtest | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/mvtest b/tools/mvtest
> index 99b154142..e839f0256 100755
> --- a/tools/mvtest
> +++ b/tools/mvtest
> @@ -28,6 +28,8 @@ append() {
>  test "${src}" != "${dest}" || die "Test \"${src}\" is the same as dest."
>  test -e "tests/${src}" || die "Test \"${src}\" does not exist."
>  test ! -e "tests/${dest}" || die "Test \"${src}\" already exists."
> +# make sure testcase is executable
> +test `stat -c '%a' tests/${src}` == 755 || chmod 755 "tests/${src}"

I think we can check and set the file permission on the target file after the
move operation done, not the source file.

And we recommend using $() to replace ``, if both of them work.

Thanks,
Zorro

>  
>  sid="$(basename "${src}")"
>  did="$(basename "${dest}")"
> -- 
> 2.42.0
> 

[PATCH v2] tools/mvtests: ensure testcase is executable (755)

[Shiyang Ruan]

Some test cases lack executable permission ('x'). Before running each
test case, `./check` checks and grants them 'x' permission. However,
this always leads to a dirty git repo. And the absence of 'x' permission
in test cases is often overlooked during reviews.

Since maintainers use mvtest to assign new case, add this change for
convenience of maintainers.

Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
---
 tools/mvtest | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/mvtest b/tools/mvtest
index 99b154142..1ad775a3e 100755
--- a/tools/mvtest
+++ b/tools/mvtest
@@ -34,6 +34,8 @@ did="$(basename "${dest}")"
 
 git mv "tests/${src}" "tests/${dest}"
 git mv "tests/${src}.out" "tests/${dest}.out"
+# make sure testcase is executable
+test $(stat -c '%a' tests/${dest}) == 755 || chmod 755 "tests/${dest}"
 sed -e "s/^# FS[[:space:]]*QA.*Test.*[0-9]\+$/# FS QA Test No. ${did}/g" -i "tests/${dest}"
 sed -e "s/^QA output created by ${sid}$/QA output created by ${did}/g" -i "tests/${dest}.out"
 sed -e "s/test-${sid}/test-${did}/g" -i "tests/${dest}.out"
-- 
2.42.0

Re: [PATCH v2] tools/mvtests: ensure testcase is executable (755)

[Zorro Lang]

On Thu, Sep 07, 2023 at 07:35:01PM +0800, Shiyang Ruan wrote:
> Some test cases lack executable permission ('x'). Before running each
> test case, `./check` checks and grants them 'x' permission. However,
> this always leads to a dirty git repo. And the absence of 'x' permission
> in test cases is often overlooked during reviews.
> 
> Since maintainers use mvtest to assign new case, add this change for
> convenience of maintainers.
> 
> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
> ---
>  tools/mvtest | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/mvtest b/tools/mvtest
> index 99b154142..1ad775a3e 100755
> --- a/tools/mvtest
> +++ b/tools/mvtest
> @@ -34,6 +34,8 @@ did="$(basename "${dest}")"
>  
>  git mv "tests/${src}" "tests/${dest}"
>  git mv "tests/${src}.out" "tests/${dest}.out"
> +# make sure testcase is executable
> +test $(stat -c '%a' tests/${dest}) == 755 || chmod 755 "tests/${dest}"

I think `chmod 755 "tests/${dest}"` is enough at here. With this change:
Reviewed-by: Zorro Lang <zlang@redhat.com>

>  sed -e "s/^# FS[[:space:]]*QA.*Test.*[0-9]\+$/# FS QA Test No. ${did}/g" -i "tests/${dest}"
>  sed -e "s/^QA output created by ${sid}$/QA output created by ${did}/g" -i "tests/${dest}.out"
>  sed -e "s/test-${sid}/test-${did}/g" -i "tests/${dest}.out"
> -- 
> 2.42.0
> 

Re: [PATCH] tools/mvtest: ensure testcase is executable (755)

[Zorro Lang]

On Wed, Sep 06, 2023 at 03:13:49PM +0800, Shiyang Ruan wrote:
> Some test cases lack executable permission ('x'). Before running each
> test case, `./check` checks and grants them 'x' permission. However,
> this always leads to a dirty git repo. And the absence of 'x' permission
> in test cases is often overlooked during reviews.
> 
> Since maintainers use mvtest to assign new case, add this change for
> convenience of maintainers.
> 
> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
> ---
>  tools/mvtest | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/mvtest b/tools/mvtest
> index 99b154142..e839f0256 100755
> --- a/tools/mvtest
> +++ b/tools/mvtest
> @@ -28,6 +28,8 @@ append() {
>  test "${src}" != "${dest}" || die "Test \"${src}\" is the same as dest."
>  test -e "tests/${src}" || die "Test \"${src}\" does not exist."
>  test ! -e "tests/${dest}" || die "Test \"${src}\" already exists."
> +# make sure testcase is executable
> +test `stat -c '%a' tests/${src}` == 755 || chmod 755 "tests/${src}"

(Weird, I thought I've replied this patch, but I can't find my reply anywhere.)

I said we can check and set the permission on the ${dest} file, after the
move operation is done. Due to we actually want to get a 755 ${dest} file
by running the mvtest.

Thanks,
Zorro

>  
>  sid="$(basename "${src}")"
>  did="$(basename "${dest}")"
> -- 
> 2.42.0
> 

Re: [PATCH] tools/mvtest: ensure testcase is executable (755)

[Shiyang Ruan]

在 2023/9/7 21:54, Zorro Lang 写道:
> On Wed, Sep 06, 2023 at 03:13:49PM +0800, Shiyang Ruan wrote:
>> Some test cases lack executable permission ('x'). Before running each
>> test case, `./check` checks and grants them 'x' permission. However,
>> this always leads to a dirty git repo. And the absence of 'x' permission
>> in test cases is often overlooked during reviews.
>>
>> Since maintainers use mvtest to assign new case, add this change for
>> convenience of maintainers.
>>
>> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
>> ---
>>   tools/mvtest | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/tools/mvtest b/tools/mvtest
>> index 99b154142..e839f0256 100755
>> --- a/tools/mvtest
>> +++ b/tools/mvtest
>> @@ -28,6 +28,8 @@ append() {
>>   test "${src}" != "${dest}" || die "Test \"${src}\" is the same as dest."
>>   test -e "tests/${src}" || die "Test \"${src}\" does not exist."
>>   test ! -e "tests/${dest}" || die "Test \"${src}\" already exists."
>> +# make sure testcase is executable
>> +test `stat -c '%a' tests/${src}` == 755 || chmod 755 "tests/${src}"
> 
> (Weird, I thought I've replied this patch, but I can't find my reply anywhere.)
> 
> I said we can check and set the permission on the ${dest} file, after the
> move operation is done. Due to we actually want to get a 755 ${dest} file
> by running the mvtest.

It seems that maillist server was down at that time.  But I received 
your reply:

 > I think we can check and set the file permission on the target file
 > after the move operation done, not the source file.
 >
 > And we recommend using $() to replace ``, if both of them work.

Then I have sent v2[1] which is using $() instead of `` and doing the 
check-then-set tests/${dest} after `git mv`, as you suggested.

[1] 
https://lore.kernel.org/fstests/20230907113501.4119112-1-ruansy.fnst@fujitsu.com/


--
Thanks,
Ruan.

> 
> Thanks,
> Zorro
> 
>>   
>>   sid="$(basename "${src}")"
>>   did="$(basename "${dest}")"
>> -- 
>> 2.42.0
>>
> 

Re: [PATCH] tools/mvtest: ensure testcase is executable (755)

[Zorro Lang]

On Fri, Sep 08, 2023 at 10:13:01AM +0800, Shiyang Ruan wrote:
> 
> 
> 在 2023/9/7 21:54, Zorro Lang 写道:
> > On Wed, Sep 06, 2023 at 03:13:49PM +0800, Shiyang Ruan wrote:
> > > Some test cases lack executable permission ('x'). Before running each
> > > test case, `./check` checks and grants them 'x' permission. However,
> > > this always leads to a dirty git repo. And the absence of 'x' permission
> > > in test cases is often overlooked during reviews.
> > > 
> > > Since maintainers use mvtest to assign new case, add this change for
> > > convenience of maintainers.
> > > 
> > > Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
> > > ---
> > >   tools/mvtest | 2 ++
> > >   1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/tools/mvtest b/tools/mvtest
> > > index 99b154142..e839f0256 100755
> > > --- a/tools/mvtest
> > > +++ b/tools/mvtest
> > > @@ -28,6 +28,8 @@ append() {
> > >   test "${src}" != "${dest}" || die "Test \"${src}\" is the same as dest."
> > >   test -e "tests/${src}" || die "Test \"${src}\" does not exist."
> > >   test ! -e "tests/${dest}" || die "Test \"${src}\" already exists."
> > > +# make sure testcase is executable
> > > +test `stat -c '%a' tests/${src}` == 755 || chmod 755 "tests/${src}"
> > 
> > (Weird, I thought I've replied this patch, but I can't find my reply anywhere.)
> > 
> > I said we can check and set the permission on the ${dest} file, after the
> > move operation is done. Due to we actually want to get a 755 ${dest} file
> > by running the mvtest.
> 
> It seems that maillist server was down at that time.  But I received your
> reply:

Recently I missed some emails/patches, I don't know if it was a problem of
fstests@ or redhat mail service. If I didn't reply a patch for long time,
please feel free to ping me or the ping fstests list.

Thanks,
Zorro

> 
> > I think we can check and set the file permission on the target file
> > after the move operation done, not the source file.
> >
> > And we recommend using $() to replace ``, if both of them work.
> 
> Then I have sent v2[1] which is using $() instead of `` and doing the
> check-then-set tests/${dest} after `git mv`, as you suggested.
> 
> [1] https://lore.kernel.org/fstests/20230907113501.4119112-1-ruansy.fnst@fujitsu.com/
> 
> 
> --
> Thanks,
> Ruan.
> 
> > 
> > Thanks,
> > Zorro
> > 
> > >   sid="$(basename "${src}")"
> > >   did="$(basename "${dest}")"
> > > -- 
> > > 2.42.0
> > > 
> > 
> 

[PATCH v3] tools/mvtest: ensure testcase is executable (755)

[Shiyang Ruan]

Some test cases lack executable permission ('x'). Before running each
test case, `./check` checks and grants them 'x' permission. However,
this always leads to a dirty git repo. And the absence of 'x' permission
in test cases is often overlooked during reviews.

Since maintainers use mvtest to assign new case, add this change for
convenience of maintainers.

Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
Reviewed-by: Zorro Lang <zlang@redhat.com>
---
 tools/mvtest | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/mvtest b/tools/mvtest
index 99b154142..bd4006eb2 100755
--- a/tools/mvtest
+++ b/tools/mvtest
@@ -34,6 +34,8 @@ did="$(basename "${dest}")"
 
 git mv "tests/${src}" "tests/${dest}"
 git mv "tests/${src}.out" "tests/${dest}.out"
+# make sure testcase is executable
+chmod 755 "tests/${dest}"
 sed -e "s/^# FS[[:space:]]*QA.*Test.*[0-9]\+$/# FS QA Test No. ${did}/g" -i "tests/${dest}"
 sed -e "s/^QA output created by ${sid}$/QA output created by ${did}/g" -i "tests/${dest}.out"
 sed -e "s/test-${sid}/test-${did}/g" -i "tests/${dest}.out"
-- 
2.42.0

Re: [PATCH v3] tools/mvtest: ensure testcase is executable (755)

[Darrick J. Wong]

On Fri, Sep 08, 2023 at 01:43:35PM +0800, Shiyang Ruan wrote:
> Some test cases lack executable permission ('x'). Before running each
> test case, `./check` checks and grants them 'x' permission. However,
> this always leads to a dirty git repo. And the absence of 'x' permission
> in test cases is often overlooked during reviews.
> 
> Since maintainers use mvtest to assign new case, add this change for
> convenience of maintainers.
> 
> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
> Reviewed-by: Zorro Lang <zlang@redhat.com>
> ---
>  tools/mvtest | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/mvtest b/tools/mvtest
> index 99b154142..bd4006eb2 100755
> --- a/tools/mvtest
> +++ b/tools/mvtest
> @@ -34,6 +34,8 @@ did="$(basename "${dest}")"
>  
>  git mv "tests/${src}" "tests/${dest}"
>  git mv "tests/${src}.out" "tests/${dest}.out"
> +# make sure testcase is executable
> +chmod 755 "tests/${dest}"

No objections to forcing 0755, though "a+x" here would have gotten the
job done as well.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>  sed -e "s/^# FS[[:space:]]*QA.*Test.*[0-9]\+$/# FS QA Test No. ${did}/g" -i "tests/${dest}"
>  sed -e "s/^QA output created by ${sid}$/QA output created by ${did}/g" -i "tests/${dest}.out"
>  sed -e "s/test-${sid}/test-${did}/g" -i "tests/${dest}.out"
> -- 
> 2.42.0
>