linux-kernel - Re: [PATCH v2] setlocalversion: work around "git describe" performance

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAK7LNATqG2bediFSzC3MvL5_qErPrJswi0A4QP7V-pspQpfb=A@mail.gmail.com>
Date: Sun, 17 Nov 2024 23:29:10 +0900
From: Masahiro Yamada <masahiroy@...nel.org>
To: Rasmus Villemoes <linux@...musvillemoes.dk>
Cc: Josh Poimboeuf <jpoimboe@...nel.org>, linux-kernel@...r.kernel.org, 
	Jeff King <peff@...f.net>
Subject: Re: [PATCH v2] setlocalversion: work around "git describe" performance

On Sun, Nov 17, 2024 at 9:20 PM Rasmus Villemoes
<linux@...musvillemoes.dk> wrote:
>
> On Sat, Nov 16 2024, Masahiro Yamada <masahiroy@...nel.org> wrote:
>
> > This patch was not sent to linux-kbuild ML
> > (and it can be one reason when a patch falls into a crack),
> > but I guess I am expected to review and pick it.
>
> Sorry, but get_maintainer.pl doesn't tell one to cc linux-kbuild.


Ah, I just realized that MAINTAINERS file does not cover this file.

KERNEL BUILD + files below scripts/ (unless maintained elsewhere)


So, I randomly pick up patches for the scripts/ directory.



> >>
> >> Reported-by: Josh Poimboeuf <jpoimboe@...nel.org>
> >
> >
> > Maybe, the comprehensive tag list looks like this?
> >
> > Reported-by: Sean Christopherson <seanjc@...gle.com>
> > Closes: https://lore.kernel.org/lkml/ZPtlxmdIJXOe0sEy@google.com/
> > Reported-by: Josh Poimboeuf <jpoimboe@...nel.org>
> > Closes: https://lore.kernel.org/lkml/309549cafdcfe50c4fceac3263220cc3d8b109b2.1730337435.git.jpoimboe@kernel.org/
>
> Fine by me.
>
> >>
> >> +try_tag() {
> >> +       tag="$1"
> >> +
> >> +       # Is $tag an annotated tag?
> >> +       [ "$(git cat-file -t "$tag" 2> /dev/null)" = "tag" ] || return 1
> >
> > The double-quotes for tag are unneeded.
> >
> > "tag"  --> tag
> >
>
> OK. The current script isn't consistent here, though (--no-local and +
> are quoted where they need not be), and I find having the quotes on both
> sides of = more visually appealing. Not a hill I'm gonna die on.

My personal preference is to not add unnecessary quotes.

In contrast, necessary quotes are missing.

So, shellcheck shows warnings.


In scripts/setlocalversion line 79:
desc=$(git describe --match=$tag 2>/dev/null)
                                            ^--^ SC2086 (info): Double
quote to prevent globbing and word splitting.


If you contribute for consistency, it is appreciated too.


> > This function returns either 1 or 0, but how is it used?
> >
>
> Well, you're right that it's not used currently, but I might as well let
> the return value reflect whether it succeeded or not. I played around
> with some variation of
>
>   if [ -n "${file_localversion#-}" ] && try_tag "${file_localversion#-}" ; then
>     :
>   elif [ -n "${file_localversion}" ] && try_tag "${version_tag}${file_localversion}" ; then
>     :
>   elif try_tag "${version_tag}"
>     :
>   else
>     count=""
>   fi
>
> but in the end decided to keep the current logic of testing some shell
> variable (previously $desc, not $count).

Either style is fine with me.



> Still, I see no reason to make
> the early returns do "return 0".






-- 
Best Regards
Masahiro Yamada