On Sun, Nov 24, 2024 at 7:12 AM Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx> wrote: > > On Sat, Nov 23 2024, Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote: > > > On Sat, Nov 23, 2024 at 12:01 AM Rasmus Villemoes > > <linux@xxxxxxxxxxxxxxxxxx> 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
