[dpdk-dev] [PATCH] devtools: check commit log fixes syntax

[dpdk-dev] [PATCH] devtools: check commit log fixes syntax

[Ferruh Yigit]

Fixes line commit id length defined as 12 in fixline alias:
fixline = log -1 --abbrev=12 --format='Fixes: %h (\"%s\")%nCc: %ae'

Check if the Fixes line commit id length matches the defined value.

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
Cc: Qi Zhang <qi.z.zhang@intel.com>
---
 devtools/check-git-log.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/devtools/check-git-log.sh b/devtools/check-git-log.sh
index d39064f9d..f4d6c1fba 100755
--- a/devtools/check-git-log.sh
+++ b/devtools/check-git-log.sh
@@ -177,6 +177,11 @@ bad=$(for fixtag in $fixtags ; do
 done | sed 's,^,\t,')
 [ -z "$bad" ] || printf "Wrong 'Fixes' reference:\n$bad\n"
 
+bad=$(for fixtag in $fixtags ; do
+	echo $fixtag | awk '{print $2}' | awk 'length != 12 {print}'
+done)
+[ -z "$bad" ] || printf "Wrong 'Fixes' syntax:\n$bad\n"
+
 # check Cc: stable@dpdk.org for fixes
 bad=$(for fix in $stablefixes ; do
 	git log --format='%b' -1 $fix | grep -qi '^Cc: *stable@dpdk.org' ||
-- 
2.17.2

Re: [dpdk-dev] [PATCH] devtools: check commit log fixes syntax

[David Marchand]

On Tue, Jan 29, 2019 at 4:31 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> Fixes line commit id length defined as 12 in fixline alias:
> fixline = log -1 --abbrev=12 --format='Fixes: %h (\"%s\")%nCc: %ae'
>
> Check if the Fixes line commit id length matches the defined value.
>

Can't git decide to report a longer string in case of collisions of
abbreviated id ?

Tried this for 2 characters, and git forcefully reported 5 chars:
$ git log -1 --abbrev=2 origin/master --format='Fixes: %h (\"%s\")'
Fixes: a2f9c (\"version: 19.02-rc4\")

I did not find any collisions with 12 characters abbreviated commitid, but
I am not sure enforcing the check on exactly 12 characters is a good idea
in the long run.


> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> Cc: Qi Zhang <qi.z.zhang@intel.com>
> ---
>  devtools/check-git-log.sh | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/devtools/check-git-log.sh b/devtools/check-git-log.sh
> index d39064f9d..f4d6c1fba 100755
> --- a/devtools/check-git-log.sh
> +++ b/devtools/check-git-log.sh
> @@ -177,6 +177,11 @@ bad=$(for fixtag in $fixtags ; do
>  done | sed 's,^,\t,')
>  [ -z "$bad" ] || printf "Wrong 'Fixes' reference:\n$bad\n"
>
> +bad=$(for fixtag in $fixtags ; do
>
+       echo $fixtag | awk '{print $2}' | awk 'length != 12 {print}'
>
+done)
>

Not an awk expert (this could be done in pure shell, but this is a
different story :-p), but I would see something like:

for fixtag in $fixtags; do
  echo $fixtag | awk 'length($2) < 12 { print $2 }';
done


-- 
David Marchand

Re: [dpdk-dev] [PATCH] devtools: check commit log fixes syntax

[Ferruh Yigit]

On 1/29/2019 5:34 PM, David Marchand wrote:
> On Tue, Jan 29, 2019 at 4:31 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
>> Fixes line commit id length defined as 12 in fixline alias:
>> fixline = log -1 --abbrev=12 --format='Fixes: %h (\"%s\")%nCc: %ae'
>>
>> Check if the Fixes line commit id length matches the defined value.
>>
> 
> Can't git decide to report a longer string in case of collisions of
> abbreviated id ?
> 
> Tried this for 2 characters, and git forcefully reported 5 chars:
> $ git log -1 --abbrev=2 origin/master --format='Fixes: %h (\"%s\")'
> Fixes: a2f9c (\"version: 19.02-rc4\")
> 
> I did not find any collisions with 12 characters abbreviated commitid, but
> I am not sure enforcing the check on exactly 12 characters is a good idea
> in the long run.

Yes git can report a longer string in case of collisions, but I don't expect to
have one with 12 characters.

This is mainly for some cases either people use full 40 chars or small ones.

Indeed in background I am (and most probably Thomas too) fixing them while
merging, I thought it is better idea to integrate that into script so that
developers can be aware of the syntax issue and fix it before sending.

> 
> 
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> ---
>> Cc: Qi Zhang <qi.z.zhang@intel.com>
>> ---
>>  devtools/check-git-log.sh | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/devtools/check-git-log.sh b/devtools/check-git-log.sh
>> index d39064f9d..f4d6c1fba 100755
>> --- a/devtools/check-git-log.sh
>> +++ b/devtools/check-git-log.sh
>> @@ -177,6 +177,11 @@ bad=$(for fixtag in $fixtags ; do
>>  done | sed 's,^,\t,')
>>  [ -z "$bad" ] || printf "Wrong 'Fixes' reference:\n$bad\n"
>>
>> +bad=$(for fixtag in $fixtags ; do
>>
> +       echo $fixtag | awk '{print $2}' | awk 'length != 12 {print}'
>>
> +done)
>>
> 
> Not an awk expert (this could be done in pure shell, but this is a
> different story :-p), but I would see something like:
> 
> for fixtag in $fixtags; do
>   echo $fixtag | awk 'length($2) < 12 { print $2 }';
> done

Yes, looks better, I will update the script.

And no specific preference on shell or awk implementation, there is no
performance concern in this script and awk already used by it, I am good as long
as it is functional.

Re: [dpdk-dev] [PATCH] devtools: check commit log fixes syntax

[Thomas Monjalon]

29/01/2019 16:30, Ferruh Yigit:
> Fixes line commit id length defined as 12 in fixline alias:
> fixline = log -1 --abbrev=12 --format='Fixes: %h (\"%s\")%nCc: %ae'
> 
> Check if the Fixes line commit id length matches the defined value.

This check was missing on purpose, in order to not be too strict.
I think it's OK if the length of the SHA1 is not always the same.

> --- a/devtools/check-git-log.sh
> +++ b/devtools/check-git-log.sh
> @@ -177,6 +177,11 @@ bad=$(for fixtag in $fixtags ; do
>  done | sed 's,^,\t,')
>  [ -z "$bad" ] || printf "Wrong 'Fixes' reference:\n$bad\n"
>  
> +bad=$(for fixtag in $fixtags ; do
> +	echo $fixtag | awk '{print $2}' | awk 'length != 12 {print}'
> +done)
> +[ -z "$bad" ] || printf "Wrong 'Fixes' syntax:\n$bad\n"
> +
>  # check Cc: stable@dpdk.org for fixes
>  bad=$(for fix in $stablefixes ; do
>  	git log --format='%b' -1 $fix | grep -qi '^Cc: *stable@dpdk.org' ||

If you really want to be that strict, it can be done simpler:

 bad=$(for fixtag in $fixtags ; do
        hash=$(echo "$fixtag" | sed 's,^Fixes: \([0-9a-f]*\).*,\1,')
        if git branch --contains $hash 2>&- | grep -q '^\*' ; then
-               good="Fixes: $hash "$(git log --format='("%s")' -1 $hash 2>&-)
+               good=$(git log --abbrev=12 --format='Fixes: %h ("%s")' -1 $hash 2>&-)
        else
                good="reference not in current branch"
        fi

Re: [dpdk-dev] [PATCH] devtools: check commit log fixes syntax

[David Marchand]

On Tue, Jan 29, 2019 at 7:07 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 1/29/2019 5:34 PM, David Marchand wrote:
> > On Tue, Jan 29, 2019 at 4:31 PM Ferruh Yigit <ferruh.yigit@intel.com>
> wrote:
> >
> >> Fixes line commit id length defined as 12 in fixline alias:
> >> fixline = log -1 --abbrev=12 --format='Fixes: %h (\"%s\")%nCc: %ae'
> >>
> >> Check if the Fixes line commit id length matches the defined value.
> >>
> >
> > Can't git decide to report a longer string in case of collisions of
> > abbreviated id ?
> >
> > Tried this for 2 characters, and git forcefully reported 5 chars:
> > $ git log -1 --abbrev=2 origin/master --format='Fixes: %h (\"%s\")'
> > Fixes: a2f9c (\"version: 19.02-rc4\")
> >
> > I did not find any collisions with 12 characters abbreviated commitid,
> but
> > I am not sure enforcing the check on exactly 12 characters is a good idea
> > in the long run.
>
> Yes git can report a longer string in case of collisions, but I don't
> expect to
> have one with 12 characters.
>
> This is mainly for some cases either people use full 40 chars or small
> ones.
>
> Indeed in background I am (and most probably Thomas too) fixing them while
> merging, I thought it is better idea to integrate that into script so that
> developers can be aware of the syntax issue and fix it before sending.
>

I can understand you want to avoid such edits yes, and so this patch.

However, I think we can do one more thing.
The contributing guide does indicate you are supposed to run both
checkpatches.sh and check-git-log.sh.
I am pretty sure I missed this second step in the past..

How about calling check-git-log.sh from checkpatches.sh ?
check-git-log.sh does not support patch files as input, so it would need
support for it.



> >
> >
> >> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >> ---
> >> Cc: Qi Zhang <qi.z.zhang@intel.com>
> >> ---
> >>  devtools/check-git-log.sh | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/devtools/check-git-log.sh b/devtools/check-git-log.sh
> >> index d39064f9d..f4d6c1fba 100755
> >> --- a/devtools/check-git-log.sh
> >> +++ b/devtools/check-git-log.sh
> >> @@ -177,6 +177,11 @@ bad=$(for fixtag in $fixtags ; do
> >>  done | sed 's,^,\t,')
> >>  [ -z "$bad" ] || printf "Wrong 'Fixes' reference:\n$bad\n"
> >>
> >> +bad=$(for fixtag in $fixtags ; do
> >>
> > +       echo $fixtag | awk '{print $2}' | awk 'length != 12 {print}'
> >>
> > +done)
> >>
> >
> > Not an awk expert (this could be done in pure shell, but this is a
> > different story :-p), but I would see something like:
> >
> > for fixtag in $fixtags; do
> >   echo $fixtag | awk 'length($2) < 12 { print $2 }';
> > done
>
> Yes, looks better, I will update the script.
>
> And no specific preference on shell or awk implementation, there is no
> performance concern in this script and awk already used by it, I am good
> as long
> as it is functional.
>

Well, I've seen Thomas reply, looks even better ;-).


-- 
David Marchand

Re: [dpdk-dev] [PATCH] devtools: check commit log fixes syntax

[Ferruh Yigit]

On 1/29/2019 8:41 PM, Thomas Monjalon wrote:
> 29/01/2019 16:30, Ferruh Yigit:
>> Fixes line commit id length defined as 12 in fixline alias:
>> fixline = log -1 --abbrev=12 --format='Fixes: %h (\"%s\")%nCc: %ae'
>>
>> Check if the Fixes line commit id length matches the defined value.
> 
> This check was missing on purpose, in order to not be too strict.
> I think it's OK if the length of the SHA1 is not always the same.

That is OK, if we don't want to be strict on this, I will update patch as rejected.

> 
>> --- a/devtools/check-git-log.sh
>> +++ b/devtools/check-git-log.sh
>> @@ -177,6 +177,11 @@ bad=$(for fixtag in $fixtags ; do
>>  done | sed 's,^,\t,')
>>  [ -z "$bad" ] || printf "Wrong 'Fixes' reference:\n$bad\n"
>>  
>> +bad=$(for fixtag in $fixtags ; do
>> +	echo $fixtag | awk '{print $2}' | awk 'length != 12 {print}'
>> +done)
>> +[ -z "$bad" ] || printf "Wrong 'Fixes' syntax:\n$bad\n"
>> +
>>  # check Cc: stable@dpdk.org for fixes
>>  bad=$(for fix in $stablefixes ; do
>>  	git log --format='%b' -1 $fix | grep -qi '^Cc: *stable@dpdk.org' ||
> 
> If you really want to be that strict, it can be done simpler:
> 
>  bad=$(for fixtag in $fixtags ; do
>         hash=$(echo "$fixtag" | sed 's,^Fixes: \([0-9a-f]*\).*,\1,')
>         if git branch --contains $hash 2>&- | grep -q '^\*' ; then
> -               good="Fixes: $hash "$(git log --format='("%s")' -1 $hash 2>&-)
> +               good=$(git log --abbrev=12 --format='Fixes: %h ("%s")' -1 $hash 2>&-)
>         else
>                 good="reference not in current branch"
>         fi
> 
> 
> 

Re: [dpdk-dev] [PATCH] devtools: check commit log fixes syntax

[Ferruh Yigit]

On 1/30/2019 9:58 AM, David Marchand wrote:
> On Tue, Jan 29, 2019 at 7:07 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
>> On 1/29/2019 5:34 PM, David Marchand wrote:
>>> On Tue, Jan 29, 2019 at 4:31 PM Ferruh Yigit <ferruh.yigit@intel.com>
>> wrote:
>>>
>>>> Fixes line commit id length defined as 12 in fixline alias:
>>>> fixline = log -1 --abbrev=12 --format='Fixes: %h (\"%s\")%nCc: %ae'
>>>>
>>>> Check if the Fixes line commit id length matches the defined value.
>>>>
>>>
>>> Can't git decide to report a longer string in case of collisions of
>>> abbreviated id ?
>>>
>>> Tried this for 2 characters, and git forcefully reported 5 chars:
>>> $ git log -1 --abbrev=2 origin/master --format='Fixes: %h (\"%s\")'
>>> Fixes: a2f9c (\"version: 19.02-rc4\")
>>>
>>> I did not find any collisions with 12 characters abbreviated commitid,
>> but
>>> I am not sure enforcing the check on exactly 12 characters is a good idea
>>> in the long run.
>>
>> Yes git can report a longer string in case of collisions, but I don't
>> expect to
>> have one with 12 characters.
>>
>> This is mainly for some cases either people use full 40 chars or small
>> ones.
>>
>> Indeed in background I am (and most probably Thomas too) fixing them while
>> merging, I thought it is better idea to integrate that into script so that
>> developers can be aware of the syntax issue and fix it before sending.
>>
> 
> I can understand you want to avoid such edits yes, and so this patch.
> 
> However, I think we can do one more thing.
> The contributing guide does indicate you are supposed to run both
> checkpatches.sh and check-git-log.sh.
> I am pretty sure I missed this second step in the past..
> 
> How about calling check-git-log.sh from checkpatches.sh ?
> check-git-log.sh does not support patch files as input, so it would need
> support for it.

That sounds good idea to have single script to run.

> 
> 
> 
>>>
>>>
>>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>> ---
>>>> Cc: Qi Zhang <qi.z.zhang@intel.com>
>>>> ---
>>>>  devtools/check-git-log.sh | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/devtools/check-git-log.sh b/devtools/check-git-log.sh
>>>> index d39064f9d..f4d6c1fba 100755
>>>> --- a/devtools/check-git-log.sh
>>>> +++ b/devtools/check-git-log.sh
>>>> @@ -177,6 +177,11 @@ bad=$(for fixtag in $fixtags ; do
>>>>  done | sed 's,^,\t,')
>>>>  [ -z "$bad" ] || printf "Wrong 'Fixes' reference:\n$bad\n"
>>>>
>>>> +bad=$(for fixtag in $fixtags ; do
>>>>
>>> +       echo $fixtag | awk '{print $2}' | awk 'length != 12 {print}'
>>>>
>>> +done)
>>>>
>>>
>>> Not an awk expert (this could be done in pure shell, but this is a
>>> different story :-p), but I would see something like:
>>>
>>> for fixtag in $fixtags; do
>>>   echo $fixtag | awk 'length($2) < 12 { print $2 }';
>>> done
>>
>> Yes, looks better, I will update the script.
>>
>> And no specific preference on shell or awk implementation, there is no
>> performance concern in this script and awk already used by it, I am good
>> as long
>> as it is functional.
>>
> 
> Well, I've seen Thomas reply, looks even better ;-).
> 
> 

Re: [dpdk-dev] [PATCH] devtools: check commit log fixes syntax

[Bruce Richardson]

On Wed, Jan 30, 2019 at 11:17:00AM +0000, Ferruh Yigit wrote:
> On 1/30/2019 9:58 AM, David Marchand wrote:
> > On Tue, Jan 29, 2019 at 7:07 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> > 
> >> On 1/29/2019 5:34 PM, David Marchand wrote:
> >>> On Tue, Jan 29, 2019 at 4:31 PM Ferruh Yigit <ferruh.yigit@intel.com>
> >> wrote:
> >>>
> >>>> Fixes line commit id length defined as 12 in fixline alias:
> >>>> fixline = log -1 --abbrev=12 --format='Fixes: %h (\"%s\")%nCc: %ae'
> >>>>
> >>>> Check if the Fixes line commit id length matches the defined value.
> >>>>
> >>>
> >>> Can't git decide to report a longer string in case of collisions of
> >>> abbreviated id ?
> >>>
> >>> Tried this for 2 characters, and git forcefully reported 5 chars:
> >>> $ git log -1 --abbrev=2 origin/master --format='Fixes: %h (\"%s\")'
> >>> Fixes: a2f9c (\"version: 19.02-rc4\")
> >>>
> >>> I did not find any collisions with 12 characters abbreviated commitid,
> >> but
> >>> I am not sure enforcing the check on exactly 12 characters is a good idea
> >>> in the long run.
> >>
> >> Yes git can report a longer string in case of collisions, but I don't
> >> expect to
> >> have one with 12 characters.
> >>
> >> This is mainly for some cases either people use full 40 chars or small
> >> ones.
> >>
> >> Indeed in background I am (and most probably Thomas too) fixing them while
> >> merging, I thought it is better idea to integrate that into script so that
> >> developers can be aware of the syntax issue and fix it before sending.
> >>
> > 
> > I can understand you want to avoid such edits yes, and so this patch.
> > 
> > However, I think we can do one more thing.
> > The contributing guide does indicate you are supposed to run both
> > checkpatches.sh and check-git-log.sh.
> > I am pretty sure I missed this second step in the past..
> > 
> > How about calling check-git-log.sh from checkpatches.sh ?
> > check-git-log.sh does not support patch files as input, so it would need
> > support for it.
> 
> That sounds good idea to have single script to run.
> 
+1 to this

Re: [dpdk-dev] [PATCH] devtools: check commit log fixes syntax

[Thomas Monjalon]

30/01/2019 12:17, Ferruh Yigit:
> On 1/30/2019 9:58 AM, David Marchand wrote:
> > The contributing guide does indicate you are supposed to run both
> > checkpatches.sh and check-git-log.sh.
> > I am pretty sure I missed this second step in the past..
> > 
> > How about calling check-git-log.sh from checkpatches.sh ?
> > check-git-log.sh does not support patch files as input, so it would need
> > support for it.
> 
> That sounds good idea to have single script to run.

It will never be only one script to run.
There are more scripts in devtools and I plan to add more.
Some checks can be done only once for a group of commits,
and will be really too slow if run for each patch when merging a branch.

About check-git-log, some tests cannot be possible outside of a git tree.
Do you want to run it when checkpatches is run on git tree?
Wouldn't it be confusing?

Re: [dpdk-dev] [PATCH] devtools: check commit log fixes syntax

[Bruce Richardson]

On Wed, Jan 30, 2019 at 11:15:44AM +0000, Ferruh Yigit wrote:
> On 1/29/2019 8:41 PM, Thomas Monjalon wrote:
> > 29/01/2019 16:30, Ferruh Yigit:
> >> Fixes line commit id length defined as 12 in fixline alias:
> >> fixline = log -1 --abbrev=12 --format='Fixes: %h (\"%s\")%nCc: %ae'
> >>
> >> Check if the Fixes line commit id length matches the defined value.
> > 
> > This check was missing on purpose, in order to not be too strict.
> > I think it's OK if the length of the SHA1 is not always the same.
> 
> That is OK, if we don't want to be strict on this, I will update patch as rejected.
> 
I think having this check is still good. It's not enforcing the rule, just
warning when violated. In 99% of cases this warning should be fixed IMHO,
especially if you guys are fixing these manually anyway.

/Bruce

Re: [dpdk-dev] [PATCH] devtools: check commit log fixes syntax

[Thomas Monjalon]

30/01/2019 12:29, Bruce Richardson:
> On Wed, Jan 30, 2019 at 11:15:44AM +0000, Ferruh Yigit wrote:
> > On 1/29/2019 8:41 PM, Thomas Monjalon wrote:
> > > 29/01/2019 16:30, Ferruh Yigit:
> > >> Fixes line commit id length defined as 12 in fixline alias:
> > >> fixline = log -1 --abbrev=12 --format='Fixes: %h (\"%s\")%nCc: %ae'
> > >>
> > >> Check if the Fixes line commit id length matches the defined value.
> > > 
> > > This check was missing on purpose, in order to not be too strict.
> > > I think it's OK if the length of the SHA1 is not always the same.
> > 
> > That is OK, if we don't want to be strict on this, I will update patch as rejected.
> > 
> I think having this check is still good. It's not enforcing the rule, just
> warning when violated. In 99% of cases this warning should be fixed IMHO,
> especially if you guys are fixing these manually anyway.

I don't fix it manually. I think SHA1 length has no importance.
Why do you think it should be fixed?

Re: [dpdk-dev] [PATCH] devtools: check commit log fixes syntax

[Bruce Richardson]

On Wed, Jan 30, 2019 at 12:27:58PM +0100, Thomas Monjalon wrote:
> 30/01/2019 12:17, Ferruh Yigit:
> > On 1/30/2019 9:58 AM, David Marchand wrote:
> > > The contributing guide does indicate you are supposed to run both
> > > checkpatches.sh and check-git-log.sh.
> > > I am pretty sure I missed this second step in the past..
> > > 
> > > How about calling check-git-log.sh from checkpatches.sh ?
> > > check-git-log.sh does not support patch files as input, so it would need
> > > support for it.
> > 
> > That sounds good idea to have single script to run.
> 
> It will never be only one script to run.
> There are more scripts in devtools and I plan to add more.
> Some checks can be done only once for a group of commits,
> and will be really too slow if run for each patch when merging a branch.
> 
> About check-git-log, some tests cannot be possible outside of a git tree.
> Do you want to run it when checkpatches is run on git tree?
> Wouldn't it be confusing?
> 
Possibly, but we can print out a warning when not run, saying
"check-git-log skipped because we are not running on git tree". I think the
benefit of having all checks done by one script is greater than the
downside of a little confusion.

/Bruce

Re: [dpdk-dev] [PATCH] devtools: check commit log fixes syntax

[Bruce Richardson]

On Wed, Jan 30, 2019 at 12:31:21PM +0100, Thomas Monjalon wrote:
> 30/01/2019 12:29, Bruce Richardson:
> > On Wed, Jan 30, 2019 at 11:15:44AM +0000, Ferruh Yigit wrote:
> > > On 1/29/2019 8:41 PM, Thomas Monjalon wrote:
> > > > 29/01/2019 16:30, Ferruh Yigit:
> > > >> Fixes line commit id length defined as 12 in fixline alias:
> > > >> fixline = log -1 --abbrev=12 --format='Fixes: %h (\"%s\")%nCc: %ae'
> > > >>
> > > >> Check if the Fixes line commit id length matches the defined value.
> > > > 
> > > > This check was missing on purpose, in order to not be too strict.
> > > > I think it's OK if the length of the SHA1 is not always the same.
> > > 
> > > That is OK, if we don't want to be strict on this, I will update patch as rejected.
> > > 
> > I think having this check is still good. It's not enforcing the rule, just
> > warning when violated. In 99% of cases this warning should be fixed IMHO,
> > especially if you guys are fixing these manually anyway.
> 
> I don't fix it manually. I think SHA1 length has no importance.
> Why do you think it should be fixed?
> 
Neatness and consistency. Since we provide on the site the expected syntax
for the fixes line, why not check for its use?

/Bruce

Re: [dpdk-dev] [PATCH] devtools: check commit log fixes syntax

[Thomas Monjalon]

30/01/2019 12:35, Bruce Richardson:
> On Wed, Jan 30, 2019 at 12:31:21PM +0100, Thomas Monjalon wrote:
> > 30/01/2019 12:29, Bruce Richardson:
> > > On Wed, Jan 30, 2019 at 11:15:44AM +0000, Ferruh Yigit wrote:
> > > > On 1/29/2019 8:41 PM, Thomas Monjalon wrote:
> > > > > 29/01/2019 16:30, Ferruh Yigit:
> > > > >> Fixes line commit id length defined as 12 in fixline alias:
> > > > >> fixline = log -1 --abbrev=12 --format='Fixes: %h (\"%s\")%nCc: %ae'
> > > > >>
> > > > >> Check if the Fixes line commit id length matches the defined value.
> > > > > 
> > > > > This check was missing on purpose, in order to not be too strict.
> > > > > I think it's OK if the length of the SHA1 is not always the same.
> > > > 
> > > > That is OK, if we don't want to be strict on this, I will update patch as rejected.
> > > > 
> > > I think having this check is still good. It's not enforcing the rule, just
> > > warning when violated. In 99% of cases this warning should be fixed IMHO,
> > > especially if you guys are fixing these manually anyway.
> > 
> > I don't fix it manually. I think SHA1 length has no importance.
> > Why do you think it should be fixed?
> > 
> Neatness and consistency. Since we provide on the site the expected syntax
> for the fixes line, why not check for its use?

OK, I'm not against the check.

Re: [dpdk-dev] [PATCH] devtools: check commit log fixes syntax

[Thomas Monjalon]

30/01/2019 12:31, Bruce Richardson:
> On Wed, Jan 30, 2019 at 12:27:58PM +0100, Thomas Monjalon wrote:
> > 30/01/2019 12:17, Ferruh Yigit:
> > > On 1/30/2019 9:58 AM, David Marchand wrote:
> > > > The contributing guide does indicate you are supposed to run both
> > > > checkpatches.sh and check-git-log.sh.
> > > > I am pretty sure I missed this second step in the past..
> > > > 
> > > > How about calling check-git-log.sh from checkpatches.sh ?
> > > > check-git-log.sh does not support patch files as input, so it would need
> > > > support for it.
> > > 
> > > That sounds good idea to have single script to run.
> > 
> > It will never be only one script to run.
> > There are more scripts in devtools and I plan to add more.
> > Some checks can be done only once for a group of commits,
> > and will be really too slow if run for each patch when merging a branch.
> > 
> > About check-git-log, some tests cannot be possible outside of a git tree.
> > Do you want to run it when checkpatches is run on git tree?
> > Wouldn't it be confusing?
> > 
> Possibly, but we can print out a warning when not run, saying
> "check-git-log skipped because we are not running on git tree". I think the
> benefit of having all checks done by one script is greater than the
> downside of a little confusion.

OK