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: <20241106212337.GA956489@coredump.intra.peff.net>
Date: Wed, 6 Nov 2024 16:23:37 -0500
From: Jeff King <peff@...f.net>
To: Rasmus Villemoes <linux@...musvillemoes.dk>
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 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. But I do note that
your "|| return 1" won't catch a failure from "rev-list", if that was
the intent. 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.

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

-Peff

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ