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.

Sign up for GitHub

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

Jump to bottom

Properly mkdir when file exists at the same path and refactor #83

Merged
merged 4 commits into from
Jun 28, 2023
Merged

Properly mkdir when file exists at the same path and refactor #83

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aac6565
Add tests to check deleted files to directories and summary
angelhof Jun 28, 2023
b85e524
Fix mkdir on top of existing file
angelhof Jun 28, 2023
6958d90
Clean up test
angelhof Jun 28, 2023
cec35bb
Refactor try change logic
angelhof Jun 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 39 additions & 4 deletions test/run_tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,14 +165,47 @@ test_summary()
touch expected1.txt
echo 'test' >expected2.txt

echo 'fail' >file_2.txt
touch target

try_example_dir=$(mktemp -d)
"$try" -D $try_example_dir "touch file_1.txt; echo test > file_2.txt; rm file.txt.gz"
"$try" -D $try_example_dir "touch file_1.txt; echo test > file_2.txt; rm file.txt.gz; rm target; mkdir target; mkdir new_dir"
"$try" summary $try_example_dir > summary.out

## Check that the summary correctly identifies every change
grep -x "$PWD/file_1.txt (modified/added)" <summary.out &&
grep -x "$PWD/file_2.txt (modified/added)" <summary.out &&
grep -x "$PWD/file.txt.gz (deleted)" <summary.out
grep -qx "$PWD/file_1.txt (added)" <summary.out &&
grep -qx "$PWD/file_2.txt (modified)" <summary.out &&
grep -qx "$PWD/file.txt.gz (deleted)" <summary.out &&
grep -qx "$PWD/target (replaced with dir)" <summary.out &&
grep -qx "$PWD/new_dir (created dir)" <summary.out
}

test_empty_summary()
{
local try_workspace=$1
cp $RESOURCE_DIR/file.txt.gz "$try_workspace/"
cd "$try_workspace/"

try_example_dir=$(mktemp -d)
"$try" -D $try_example_dir "echo hi" > /dev/null
"$try" summary $try_example_dir > summary.out

## We want to return true if the following line is not found!
! grep -q "Changes detected in the following files:" <summary.out
}

test_mkdir_on_file()
{
local try_workspace=$1
cp $RESOURCE_DIR/file.txt.gz "$try_workspace/"
cd "$try_workspace/"

## Set up expected output
touch target
mkdir expected

"$try" -y "rm target; mkdir target"
diff -qr expected target
}

# a test that deliberately fails (for testing CI changes)
Expand All @@ -195,6 +228,8 @@ if [ "$#" -eq 0 ]; then
run_test test_pipeline
run_test test_cmd_sbst_and_var
run_test test_summary
run_test test_empty_summary
run_test test_mkdir_on_file

# uncomment this to force a failure
# run_test test_fail
Expand Down
121 changes: 80 additions & 41 deletions try
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,35 +144,32 @@ summary() {
exit 1
fi

# We don't include directories here (like in commit) since that would be too verbose for the summary.
changed_files=$(find "$SANDBOX_DIR/upperdir/" -type f -or \( -type c -size 0 \) | ignore_changes)

if [ -z "$changed_files" ];
## Finds all potential changes
changed_files=$(find_upperdir_changes "$SANDBOX_DIR")
summary_output=$(process_changes "$SANDBOX_DIR" "$changed_files")

if [ -z "$summary_output" ];
then
return 1
fi

echo
echo "Changes detected in the following files:"
echo
while IFS= read -r changed_file; do
local_file="${changed_file#$SANDBOX_DIR/upperdir}"
## KK 2023-06-20 Could print local_file instead of changed file for
## cleaner output.
if [ -d "$changed_file" ] && ! [ -d "${local_file}" ]
then # new directory
## KK 2023-06-20 This is not reachable since the `type -d` option is not given to find above.
echo "$local_file (created)"
elif is_whiteout_file "$changed_file"
then # whiteout file
echo "$local_file (deleted)"
elif [ -f "$changed_file" ]
then # normal file
echo "$local_file (modified/added)"
fi

while IFS= read -r summary_line; do
local_file="$(echo "$summary_line" | cut -c 4- )"
case "$summary_line" in
(rd*) echo "$local_file (replaced with dir)";;
(md*) echo "$local_file (created dir)";;
(de*) echo "$local_file (deleted)";;
(mo*) echo "$local_file (modified)";;
(ad*) echo "$local_file (added)"
esac
done <<EOF
$changed_files
$summary_output
EOF

return 0
}

Expand All @@ -181,24 +178,19 @@ EOF
################################################################################

commit() {
# This is different from the one in summary because it also includes all directories.
# TODO: Could be made more efficient by only appending directories to the already computed
# changed_files from summary.
changed_files=$(find "$SANDBOX_DIR/upperdir/" -type f -o \( -type c -size 0 \) -o -type d | ignore_changes)

while IFS= read -r changed_file; do
local_file="${changed_file#$SANDBOX_DIR/upperdir}"
if [ -d "$changed_file" ] && ! [ -d "${local_file}" ]
then # new directory
mkdir "${local_file}"
elif is_whiteout_file "$changed_file"
then # whiteout file
rm "${local_file}"
elif [ -f "$changed_file" ]
then # normal file
# -f to commit the file if the target file is readonly, and -a to preserve permissions
cp -fa "$changed_file" "${local_file}"
fi
changed_files=$(find_upperdir_changes "$SANDBOX_DIR")
summary_output=$(process_changes "$SANDBOX_DIR" "$changed_files")

while IFS= read -r summary_line; do
local_file="$(echo "$summary_line" | cut -c 4-)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is good, but could break with particularly nasty filenames (e.g., newlines). The most robust approach is to null terminate, à la find ... -print0 or xargs -0.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, will add an issue so that we can properly fix it (add the necessary tests). We can merge this for now (since it is a more impactful fix).

changed_file="$SANDBOX_DIR/upperdir$local_file"
case $summary_line in
(rd*) rm -f "$local_file"; mkdir "$local_file";;
(md*) mkdir "$local_file";;
(de*) rm "$local_file";;
(mo*) cp -fa "$changed_file" "$local_file";;
(ad*) cp -fa "$changed_file" "$local_file"
esac

if [ $? -ne 0 ]
then
Expand All @@ -207,13 +199,12 @@ commit() {
exit 1
fi
done <<EOF
$changed_files
$summary_output
EOF
}

## Checks if a file is an overlayfs whiteout file
is_whiteout_file()
{
is_whiteout_file() {
file="$1"
[ -c "$file" ] && ! [ -s "$file" ] && [ "$(stat -c %t,%T "$file")" = "0,0" ]
}
Expand All @@ -224,6 +215,54 @@ ignore_changes() {
grep -v -e .rkr -e Rikerfile
}

## Lists all upperdir changes in raw format
find_upperdir_changes() {
sandbox_dir="$1"
find "$sandbox_dir/upperdir/" -type f -o \( -type c -size 0 \) -o -type d | ignore_changes
}

## Processes upperdir changes to an internal format that can be processed by summary and commit
## Format:
## XX PATH
## where:
## XX is a two character code for the modification
## - rd: Replaced with a directory
## - md: Created a directory
## - de: Deleted a file
## - mo: Modified a file
## - ad: Added a file
## PATH is the local path
process_changes() {
sandbox_dir="$1"
changed_files="$2"
while IFS= read -r changed_file; do
local_file="${changed_file#$sandbox_dir/upperdir}"
if [ -d "$changed_file" ] && ! [ -d "${local_file}" ]
then # new directory
## If something exists there, we need to delete it first
if [ -e "$local_file" ]
then
echo "rd $local_file"
else
echo "md $local_file"
fi
elif is_whiteout_file "$changed_file"
then # whiteout file
echo "de $local_file"
elif [ -f "$changed_file" ]
then # normal file
if [ -e "$local_file" ]
then
echo "mo $local_file"
else
echo "ad $local_file"
fi
fi
done <<EOF
$changed_files
EOF
}


################################################################################
# Argument parsing
Expand Down