Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix breakpoint hit in file sourced with relative path after script changed the working directory #61

Closed
wants to merge 2 commits into from

Conversation

jansorg
Copy link
Collaborator

@jansorg jansorg commented Dec 30, 2024

The script (shown below) of my integration tests of BashSupport Pro failed to hit a breakpoint in the function definition of include3.sh.
The breakpoint was not hit because the lookup in hook.sh was not using the current working directory of the script to find a path by relative path. In this case ../dir3/include3.sh was not looked inside the PWD $DIR/dir2 but only elsewhere.

This PR changes the file lookup to always use the configured lookup directories _Dbg_dir as fallback if the lookup of absolute and relative paths did not yield a valid result.
Also, the result of the lookup based on _Dbg_dir was not checked against $_Dbg_filenames like the lookup a few lines above.

Tests are passing and the breakpoint is now hit. But I'm not sure how to add a test case for this.

Question:
This code looks suspiciously similar to the code I changed. Should I change this, even though I'm not sure how it's used?

bashdb/lib/file.sh

Lines 104 to 118 in b78910a

# Resolve file using _Dbg_dir
typeset -i n=${#_Dbg_dir[@]}
typeset -i i
for (( i=0 ; i < n; i++ )) ; do
typeset dirname="${_Dbg_dir[i]}"
if [[ "$dirname" == '\$cdir' ]] ; then
dirname="$_Dbg_cdir"
elif [[ "$dirname" == '\$cwd' ]] ; then
dirname="$(pwd)"
fi
if [[ -f "$dirname/$find_file" ]] ; then
echo "$dirname/$find_file"
return 0
fi
done

Script of my test case

#!/bin/bash

DIR="$(CDPATH='' cd -- "$(dirname -- "$0")" && pwd -P)"

cd "$DIR/dir2"

. ../dir3/include3.sh
include3 # function defined in include3.sh

…rking directory

The current working directory of the debugged script must always be used as fallback, as it's the directory used by the script to locate files with a relative path.
@jansorg jansorg requested a review from rocky December 30, 2024 17:09
@rocky
Copy link
Collaborator

rocky commented Dec 30, 2024

Looks good to me.

For me, I'd probably add a comment or rather clarification as to why path searching is done outside of the case when the name starts with "."

The way I think of it is that there are explicit relative path names, those that start with "." (or "..") and implicit relative path names which do not start with either "." or "/". Testing for an absolute name was done above, so that leaves relative (both implicit and explicit). So the comment around the "elif" is misleading because that branch is about just one of the kinds of relative paths.

As for the duplicate code, yes, those two should be combined. I think the situation was more or less a result of parallel evolution.

The intent of _Dbg_is_file is to check whether a file exists. In that, it resolves the file name. _Dbg_resolve_expand_filename is part of the file caching module. I think its use was to resolve and expand a path before saving information about it.

@jansorg
Copy link
Collaborator Author

jansorg commented Dec 30, 2024

Thanks! I'll try to clean up the code tomorrow.
I don't think we should detect (implicit) relative paths by comparing the first char with ".". Anything not starting with a "/" is a relative path. Because we don't know to which directory it refers to, we're testing all the base dirs.

I think that we should test against the script's current working directory first, because that what it's (probably?) using itself. But I'll have to think a bit more about the possible edge-cases...

I'll also try to get rid of the duplicate code.

@rocky
Copy link
Collaborator

rocky commented Dec 30, 2024

I think that we should test against the script's current working directory first, because that what it's (probably?) using itself.

I think you are correct.

@jansorg
Copy link
Collaborator Author

jansorg commented Dec 31, 2024

Closing, as it doesn't seem to be the right fix. I'll create a new issue

@jansorg jansorg closed this Dec 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants