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

[Rasmus Villemoes]

On Sat, Nov 16 2024, Masahiro Yamada <masahiroy@xxxxxxxxxx> 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.

>>
>> Reported-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
>
>
> Maybe, the comprehensive tag list looks like this?
>
> Reported-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> Closes: https://lore.kernel.org/lkml/ZPtlxmdIJXOe0sEy@xxxxxxxxxx/
> Reported-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> Closes: https://lore.kernel.org/lkml/309549cafdcfe50c4fceac3263220cc3d8b109b2.1730337435.git.jpoimboe@xxxxxxxxxx/

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.

> 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). Still, I see no reason to make
the early returns do "return 0".

Rasmus