linux-kernel - Re: [PATCH] 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: <87jzdfgvu0.fsf@prevas.dk>
Date: Thu, 07 Nov 2024 11:52:07 +0100
From: Rasmus Villemoes <ravi@...vas.dk>
To: Jeff King <peff@...f.net>
Cc: Josh Poimboeuf <jpoimboe@...nel.org>,  Masahiro Yamada
 <masahiroy@...nel.org>,  linux-kernel@...r.kernel.org
Subject: Re: [PATCH] setlocalversion: work around "git describe" performance

On Wed, Nov 06 2024, Jeff King <peff@...f.net> wrote:

> On Wed, Nov 06, 2024 at 02:28:38AM +0100, Rasmus Villemoes wrote:
>
>> This has been acknowledged as a performance bug in git [1]. However,
>> no solution is yet in git.git, and even when one lands, it will take
>> quite a while before it finds its way to a release and for
>> $random_kernel_developer to pick that up.
>
> I just posted patches in:
>
>   https://lore.kernel.org/git/20241106192236.GC880133@coredump.intra.peff.net/
>
> but I agree it is worth dealing with in the interim. And also, I really
> suspect that this new code may end up faster than git-describe in some
> cases anyway.
>
>> +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?
>> +	read left right <<EOF || return 1
>> +$(git rev-list --count --left-right "$tag"...HEAD 2> /dev/null)
>> +EOF
>> +
>> +	# $left is 0 if and only if $tag is an ancestor of HEAD
>> +	[ "$left" -eq 0 ] || return 1
>> +
>> +	# $right is the number of commits in the range $tag..HEAD, possibly 0.
>> +	count="$right"
>> +
>> +	return 0
>> +}
>
> The git parts all look good to me here (and not knowing much about the
> kernel's scripts I'll refrain from even looking closely at the other
> parts).
>
> The use of the here-doc is a little funny, but I guess you can't just
> pipe to read since that would put it in a subshell.

Exactly. I try to avoid string manipulation using "$(echo ... | cut
...)" etc. when I can get away with doing it in the shell itself, which
sometimes leads to some awkward constructions.

> But I do note that your "|| return 1" won't catch a failure from
> "rev-list", if that was the intent.

Well, not a failure from rev-list itself, but I thought that when the
command ended up producing nothing on stdout for read to consume, read
would fail. But you are right, that doesn't work as I expected.

  $ printf '' | read left right ; echo "$?"
  1
  $ printf '\n' | read left right ; echo "$?"
  0

I'd need for the inner $() to not strip a newline, and for the heredoc
to not implicitly add one, for read to actually fail.

>I think you'd have to use a real tempfile to catch it, or
> play horrid tricks with echoing $? into the "read" stream. I guess the
> explicit check for "$left -eq 0" will catch most failures anyway.

Yes, except that I should probably then use string comparison = instead
of -eq, since $left will be empty when rev-list hasn't produced any
output.

Maybe

  set -- $(git rev-list ... 2> /dev/null)
  left="$1"
  right="$2"

is simpler. There's also

  left_right="$(git rev-list ...)"
  left="${left_right%	*}"
  right="${left_right#*	}"

where the whitespace are actually tab characters, but that's probably
too subtle.

> If you use "<<-" you can get rid of the awkward indentation.

Ack, thanks for the reminder. I seemed to remember that as a bash
extension, but it's not.

Rasmus