Re: [Buildroot] [RFC 14/14] utils/git_hooks: new script

From: "Yann E. MORIN" <yann.morin.1998@free.fr>
To: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: buildroot@buildroot.org
Subject: Re: [Buildroot] [RFC 14/14] utils/git_hooks: new script
Date: Sun, 23 Apr 2023 21:41:38 +0200	[thread overview]
Message-ID: <20230423194138.GO2696@scaer> (raw)
In-Reply-To: <20220731193521.1217825-15-ricardo.martincoski@gmail.com>

Ricardo, All,

On 2022-07-31 16:35 -0300, Ricardo Martincoski spake thusly:
> Add a pre-commit hook that can be used by any buildroot developer to
> prevent local commits that introduce warnings that check-package would
> catch.

The thing is, most users will not add that hook before comitting and
submitting patches.

> It can also be used by maintainers.

In fact, as maintainers, what we are more insterested in, is a
post-applypatch hook, not a pre-commit one. However, your script
does not work as a post-applypatch hook.

Furthermore, maintainers already have their hooks. For example, I have
been maintaining and publishing mines:
    https://git.buildroot.org/~ymorin/git/buildroot.git-hooks

Even if the history only dates back to 2021-11-14, I had those hooks
even before, and they are an aggregation of what Arnout and Peter
already had before and shared with me.

My hooks are a bit slow to run, as they run on all commits, even during
a rebase, or when applying a series of patches (which I do with a helper
around patchwork API). I am OK with the extra time it takes, but that is
not necessarily the case for other maintainers.

> This hook checks only the files added/changed by the commit, so it runs
> fast.
> It also skips the check when working in branches that do not have this
> infra.

> But when there is a change to check-package itself in the commit being
> performed, the shole tree is checked, since new warnings in the tree can
> occur.

Ah, this last part is nice, indeed.

But as a maintainer, when I see changes to, say, check-package, I do run
it to validate it, so changes in the ignore list, or new issues, or
similar, would get caught.

And if check-package et al. add new checks, either the issues it reports
were previously fixed (so we can run that new check unconditionally), or
the offenders have been added to the ignore list (or will be in the next
patch).

So, I am not sure this is important.

> At same time, add an option -T to utils/docker-run so docker can run
> without a tty, otherwise calling it from inside the commit hook would
> lead to this error:
>  the input device is not a TTY

I already applied a better patch, that automatically detects if a tty is
needed or not, see 3d8212c4b29d (utils/docker-run: allow running without
a tty).

In the end, I am not convinced we should have this hook in the tree...

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot