5.3 compat#562
Open
andyneff wants to merge 11 commits into
Open
Conversation
Signed-off-by: Andy Neff <andy@visionsystemsinc.com>
Signed-off-by: Andy Neff <andy@visionsystemsinc.com>
Signed-off-by: Andy Neff <andy@visionsystemsinc.com>
Signed-off-by: Andy Neff <andy@visionsystemsinc.com>
Signed-off-by: Andy Neff <andy@visionsystemsinc.com>
Signed-off-by: Andy Neff <andy@visionsystemsinc.com>
Signed-off-by: Andy Neff <andy@visionsystemsinc.com>
Member
Author
|
Tests were passing in 1354be4 before I moved the file, so I think vsi_common main is being cloned by a integration test, and will fail until after this is merged in. So ignore the test failures. |
Signed-off-by: Andy Neff <andy@visionsystemsinc.com>
scott-vsi
reviewed
May 29, 2026
Comment on lines
+1040
to
+1042
| # "./_" is used so that when JUSTFILE and JUST_DOCKER_COMPOSE_DIR are unset, | ||
| # it will use that value that is dirnamed and throws away "/_" and results | ||
| # in pushd . to handle the bash 5.3 issue where pushd "" is now an error |
Contributor
There was a problem hiding this comment.
Suggested change
| # "./_" is used so that when JUSTFILE and JUST_DOCKER_COMPOSE_DIR are unset, | |
| # it will use that value that is dirnamed and throws away "/_" and results | |
| # in pushd . to handle the bash 5.3 issue where pushd "" is now an error | |
| # "./_" is used so that when JUSTFILE and JUST_DOCKER_COMPOSE_DIR are unset, | |
| # it will dirname that value, which throws away "/_" and results in pushd . | |
| # in order to handle the bash 5.3 issue where pushd "" is now an error |
| # | ||
| # In bash version 5.3 and older, caller returns the first line of a multiline command, whereas before it returned the last line. | ||
| # | ||
| # :Value: * ``0`` ``pushd ""`` returns 0 |
Contributor
There was a problem hiding this comment.
is this example right? I dont se a multi-line command
| # Guarantee a trailing / or empty string (this may be guaranteed by | ||
| # --show-prefix) | ||
| prefix="${prefix:+"${prefix%/}/"}" | ||
| local prefix=$("${GIT}" rev-parse --show-prefix) |
Contributor
There was a problem hiding this comment.
is the trailing / here guaranteed by --show-prefix or something?
Member
Author
There was a problem hiding this comment.
Yes and no. If it returns a non-empty value, it appears to be guaranteed. But if it's empty, then clearly, no slash
| # core.worktree isn't set). Fortunately, in this case, --git-dir is correct | ||
| local dir | ||
| dir="$("${GIT}" rev-parse --show-toplevel)" | ||
| dir="$("${GIT}" rev-parse --show-toplevel)" # Don't use git_helper_toplevel, logic reasons |
Contributor
There was a problem hiding this comment.
heh. I don't get it, but def a good comment
| # git submodule foreach must be run from the top-level working tree in | ||
| # git v1.8.3 | ||
| pushd "$("${GIT}" rev-parse --show-cdup)" &> /dev/null | ||
| local git_top_dir="$(git_helper_cdup)" |
Contributor
There was a problem hiding this comment.
should prob be called up_path or git_up_path. You have something called git_helper_toplevel
| # git submodule foreach must be run from the top-level working tree in | ||
| # git v1.8.3 | ||
| pushd "$("${GIT}" rev-parse --show-cdup)" &> /dev/null | ||
| local git_top_dir="$(git_helper_cdup)" |
| # git submodule foreach must be run from the top-level working tree in | ||
| # git v1.8.3 | ||
| pushd "$("${GIT}" rev-parse --show-cdup)" &> /dev/null | ||
| local git_top_dir="$(git_helper_cdup)" |
| # At some point, prefix was deprecated and removed in favor of displaypath | ||
| local submodule_paths | ||
| if [ "${submodule_foreach_rv}" -eq "0" ]; then | ||
| # The following logic works based off of these variables being empty instead of `./` for proper output |
Contributor
There was a problem hiding this comment.
Suggested change
| # The following logic works based off of these variables being empty instead of `./` for proper output | |
| # The following logic works based off of these variables being empty instead | |
| # of `./` (as they are in bash 5.3) |
| popd &> /dev/null | ||
| fi | ||
| popd &> /dev/null | ||
| # |
andyneff
commented
May 29, 2026
| # Guarantee a trailing / or empty string (this may be guaranteed by | ||
| # --show-prefix) | ||
| prefix="${prefix:+"${prefix%/}/"}" | ||
| local prefix=$("${GIT}" rev-parse --show-prefix) |
Member
Author
There was a problem hiding this comment.
Yes and no. If it returns a non-empty value, it appears to be guaranteed. But if it's empty, then clearly, no slash
Co-authored-by: Andy Neff <andyneff@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There are a few changes in bash 5.3 that lead to incompatibilities and failures in VSI Common.
The primary different is
pushd ""+popdno longer silently works exactly as intended. So many patches had to be added for that.