linux-kernel - Re: [PATCH v4] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <87iksdk3ag.fsf@prevas.dk>
Date: Sat, 23 Nov 2024 23:12:39 +0100
From: Rasmus Villemoes <linux@...musvillemoes.dk>
To: Masahiro Yamada <masahiroy@...nel.org>
Cc: linux-kbuild@...r.kernel.org,  linux-kernel@...r.kernel.org,  Jeff King
 <peff@...f.net>,  Sean Christopherson <seanjc@...gle.com>,  Josh Poimboeuf
 <jpoimboe@...nel.org>
Subject: Re: [PATCH v4] setlocalversion: work around "git describe" performance

On Sat, Nov 23 2024, Masahiro Yamada <masahiroy@...nel.org> wrote:

> On Sat, Nov 23, 2024 at 12:01 AM Rasmus Villemoes
> <linux@...musvillemoes.dk> wrote:

>> v4:
>>
>> - Switch the logic to make use of the return values from try_tag,
>>   instead of asking whether $count has been set.
>
>
> No, please do not do this.
>
> As I replied in v3, my plan is to set -e, because otherwise
> the shell script is fragile.
>
> With this version, -e will not work in try_tag()
> because it is used in the if condition.

I'm confused. Previously you said that "either style is fine with
me". Now you've invented some necessity to add "set -e", which of course
yes, is then suppressed inside try_tag. But there is not a single
statement within that that would cause "set -e" to exit anyway: The only
one that is not a simple assignment or is itself a test is the "set --
$()", and git rev-list failing there does not cause set -e to trigger.

Aside from that, I'm skeptical to introducing set -e anyway, it's simply
too hard to reason about what it will actually
do. http://mywiki.wooledge.org/BashFAQ/105 . But you're the maintainer.

>> +try_tag() {
>> +       tag="$1"
>> +
>> +       # Is $tag an annotated tag?
>> +       [ "$(git cat-file -t "$tag" 2> /dev/null)" = tag ] || return 1
>> +
>> +       # Is it an ancestor of HEAD, and if so, how many commits are in $tag..HEAD?
>> +       # shellcheck disable=SC2046 # word splitting is the point here
>> +       set -- $(git rev-list --count --left-right "$tag"...HEAD 2> /dev/null)
>> +
>> +       # $1 is 0 if and only if $tag is an ancestor of HEAD. Use
>> +       # string comparison, because $1 is empty if the 'git rev-list'
>> +       # command somehow failed.
>> +       [ "$1" = 0 ] || return 1
>> +
>> +       # $2 is the number of commits in the range $tag..HEAD, possibly 0.
>> +       count="$2"
>
> Redundant double-quotes.

Perhaps, but sorry, I'm not code-golfing, and trying to remember when
quotes can be elided when variables are referenced is simply not
something I spend my very limited brain capacity on.

Feel free to make any adjustments you want and commit that, or drop
this, I'm not sending a v5 as that seems to be a waste of everybody's
time.

Rasmus