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

* Add tests to check deleted files to directories and summary * Fix mkdir on top of existing file * Clean up test * Refactor try change logic
binpash · Jun 28, 2023 · 95471fe · 95471fe
1 parent 66c4506
commit 95471fe
Show file tree

Hide file tree

Showing 2 changed files with 119 additions and 45 deletions.
diff --git a/test/run_tests.sh b/test/run_tests.sh
@@ -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)
@@ -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

diff --git a/try b/try
@@ -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
 }
 
@@ -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-)"
+        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
@@ -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" ]
 }
@@ -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