Message-ID: <CAK7LNASq+iTgjhCYdwkmgDmyPTbtORgU6UWrs889S4x1xkCHxg@mail.gmail.com>
Date: Sun, 24 Nov 2024 11:10:46 +0900
From: Masahiro Yamada <masahiroy@...nel.org>
To: Rasmus Villemoes <linux@...musvillemoes.dk>
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 Sun, Nov 24, 2024 at 7:12 AM Rasmus Villemoes
<linux@...musvillemoes.dk> wrote:
>
> 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".

That was my comment in v2.

At that time, I was not aware that the -e option was missing
in this script.

Sorry, I changed my mind.

In v3, I commented what I like this script to look like
when turning on the -e option.
Then, you came back with a different approach.


> 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.

This is correct, but think about the resiliency when someone
adds more code to try_tag() in the future.


> 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.



First, set -e is almost always useful because it is not realistic
to add '|| exit 1' for every command.

For example, see commit bc53d3d777f81385c1bb08b07bd1c06450ecc2c1
how the -e catches an error.

Second, the link you reference is exaggerating.

For example, the article mentioned the quirks in the Bash mode.
This argument is not applicable in our case because
Bash runs in the POSIX mode when it is invoked as 'sh'.
Since this script is invoked by #!/bin/sh,
'set -e' follows the POSIX behavior whether /bin/sh is a
symlink to bash or dash.
The -e option is propagated to subshell executions.
(It would not if the shebang had been #!/bin/bash,
but Bash still provides "set -o posix" to cater to this case).

In the referred link, I do not find a good reason to
avoid using the -e option.

When a function returns a value (yes/no question just like
try_tag()), you need to be careful about the possible
third case.

1) yes
2) no
3) error


I hope the following provides even more clarification.

Let's say, is_ancestor_tag() checks if the given tag is
an ancestor or not.



[Bad Code]

set -e

if is_ancestor_tag "${tag}"; then
        # Any error in is_ancestor_tag() is ignored.
        # "Yes, ... " may be printed even if  an error occurs.
        echo "Yes, ${tag} is an ancestor"
else
        echo "No, ${tag} is not an ancestor"
fi


[Good Code 1]

set -e

ret=$(is_ancestor_tag "${tag}")
# If any error occurs in is_ancestor_tag()
# the script is terminated here.

if [ "${ret}" = yes ]; then
        echo "Yes, ${tag} is an ancestor"
else
        echo "No, ${tag} is not an ancestor"
fi


[Good Code 2]

set -e

# is_ancestor_tag() sets 'ret' in the function body
is_ancestor_tag "${tag}"
if [ "${ret}" = yes ]; then
        echo "Yes, ${tag} is an ancestor"
else
        echo "No, ${tag} is not an ancestor"
fi


V3 is [Good Code 2], as the return value, 'count' is assigned
within the try_tag() function, and the caller checks it.








> >> +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



--
Best Regards
Masahiro Yamada

Powered by blists - more mailing lists