Re: [PATCH v4] setlocalversion: work around "git describe" performance

[Rasmus Villemoes <linux@rasmusvillemoes.dk>]

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

> On Sat, Nov 23, 2024 at 12:01 AM Rasmus Villemoes
> <linux@rasmusvillemoes.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