[libcamera-devel] Problem with utils/checkstyle.py in a pre-commit hook?

[Naushir Patuck]

Hi Laurent,



On Thu, 21 Jan 2021, 12:46 pm Laurent Pinchart, <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Naush,
>
> On Thu, Jan 21, 2021 at 09:06:18AM +0000, Naushir Patuck wrote:
> > Hi Umang,
> >
> > On Wed, 20 Jan 2021 at 16:52, Umang Jain <email at uajain.com> wrote:
> >
> > > Hi Naushir
> > >
> > > Yes, I have hit this issue with pre-commit and your diagonsis is right.
> >
> > Thanks for confirming!  I'm glad it's not only me :-) I'm positive this
> has
> > only recently started happening.
> >
> > > On 1/20/21 4:29 PM, Naushir Patuck wrote:
> > > On Wed, 20 Jan 2021 at 10:52, Naushir Patuck wrote:
> > >
> > >> Hi all,
> > >>
> > >> I have started noticing a problem with the utils/checkstyle.py script
> > >> when run in the provided pre-commit hook (utils/hooks/pre-commit).
> Here is
> > >> an example output when I do a "git commit --amend --fixup=xxxx"
> command:
> > >
> > > Sorry, my git command has a typo above, it is  "git commit -a
> --fixup=xxxx"
> > >
> > >> fatal: ambiguous argument '': unknown revision or path not in the
> working
> > >> tree.
> > >> Use '--' to separate paths from revisions, like this:
> > >> 'git <command> [<revision>...] -- [<file>...]'
> > >> Traceback (most recent call last):
> > >>   File "./utils/checkstyle.py", line 875, in <module>
> > >>     sys.exit(main(sys.argv))
> > >>   File "./utils/checkstyle.py", line 850, in main
> > >>     commits.append(StagedChanges())
> > >>   File "./utils/checkstyle.py", line 237, in __init__
> > >>     Commit.__init__(self, '')
> > >>   File "./utils/checkstyle.py", line 206, in __init__
> > >>     self.__parse()
> > >>   File "./utils/checkstyle.py", line 215, in __parse
> > >>     self.__title = files[0]
> > >> IndexError: list index out of range
> > >>
> > >> This used to certainly work in the past, so I can only assume either
> > >> something in the script, or my environment has changed.  The reason
> behind
> > >> the git command failure seems to be related to the last argument in
> the
> > >> subprocess call:
> > >>
> > >> ret = subprocess.run(['git', 'show', '--pretty=oneline',
> '--name-status',
> > >>                                   self.commit],
> > >>
> > >> stdout=subprocess.PIPE).stdout.decode('utf-8')
> > >>
> > >> where self.commit is an empty string.
> > >>
> > >> This may have been introduced by commit 097720840 ('utils:
> checkstyle.py:
> > >> Move commit handling to a separate section').  A fix that works for
> me is
> > >> as follows:
>
> I think the issue has been introduced in commit 4f5d17f3a4f5 ("utils:
> checkstyle.py: Make title and files properties of commit class"). I have
> sent a tentative fix to the mailing list and CC'ed you. Could you please
> test it ?
>

Thanks for the fix. Will test later and report back.

Regards,
Naush

>
> > >> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> > >> index 0e9659e98518..cb7b2d151412 100755
> > >> --- a/utils/checkstyle.py
> > >> +++ b/utils/checkstyle.py
> > >> @@ -207,9 +207,11 @@ class Commit:
> > >>
> > >>      def __parse(self):
> > >>          # Get the commit title and list of files.
> > >> -        ret = subprocess.run(['git', 'show', '--pretty=oneline',
> > >> '--name-status',
> > >> -                              self.commit],
> > >> -
> > >> stdout=subprocess.PIPE).stdout.decode('utf-8')
> > >> +        args = ['git', 'show', '--pretty=oneline', '--name-status']
> > >> +        if self.commit != '':
> > >> +            args.append(self.commit)
> > >> +
> > >> +        ret = subprocess.run(args,
> > >> stdout=subprocess.PIPE).stdout.decode('utf-8')
> > >>          files = ret.splitlines()
> > >>          self.__files = [CommitFile(f) for f in files[1:]]
> > >>
> > >> I think this will parse the HEAD commit and not the diff/staged
> changes.
> > >
> > > I haven't looked at it closely but that's my understanding. I have seen
> > > mostly using the hook as a post-commit by other devs and that (in my
> > > testing) works fine.
> >
> > Perhaps - my fix above worked on the (seemingly correct) assumption that
> an
> > empty string appended to the end of the argument list would break the
> > command.  If there is a commit hash in the self.commit string, it will
> > append it as before.
> > If the checkpatch script is not meant to be used as a pre-commit hook,
> > perhaps we should remove the utils/hooks/pre-commit script entirely so
> > others don't encounter this problem?
> >
> > >> I was just wondering if I have something strange in my environment
> that is
> > >> causing these issues, or are other folks seeing this as well?
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210121/b611a939/attachment-0001.htm>