From 913d026537769fa3592abcb3abddb0fffacfb79a Mon Sep 17 00:00:00 2001
From: Josh Rabinowitz <joshr@joshr.com>
Date: Tue, 1 Jan 2019 09:37:11 -0500
Subject: [PATCH] Fixes for 'changes' and trailing newlines, for #291 (#293)

* tests and comments about 'changes' for #291
* add 'changes' tests, improve diagnostic
* preserve trailing newlines in diff output
* use bash trickery to preserve trailing newlines in captured text
* test 'changes' on files without newlines and when called on a non-existant file
* improve comments and variable names
---
 src/_utils/_git_secret_tools.sh    |  2 ++
 src/commands/git_secret_changes.sh | 21 +++++++-------
 tests/_test_base.bash              | 11 +++++++-
 tests/test_cat.bats                |  1 -
 tests/test_changes.bats            | 45 ++++++++++++++++++++++++++----
 tests/test_expiration.bats         | 13 ++++-----
 tests/test_hide.bats               | 10 +++----
 tests/test_whoknows.bats           |  5 ++--
 8 files changed, 76 insertions(+), 32 deletions(-)

diff --git a/src/_utils/_git_secret_tools.sh b/src/_utils/_git_secret_tools.sh
index 76b96ed5..481ccb31 100644
--- a/src/_utils/_git_secret_tools.sh
+++ b/src/_utils/_git_secret_tools.sh
@@ -738,5 +738,7 @@ function _decrypt {
     local msg="problem decrypting file with gpg: exit code $exit_code: $filename"
     _warn_or_abort "$msg" "$exit_code" "$error_ok"
   fi
+
+  # at this point the file should be written to disk or output to stdout
 }
 
diff --git a/src/commands/git_secret_changes.sh b/src/commands/git_secret_changes.sh
index 6a1b5c18..84c79447 100644
--- a/src/commands/git_secret_changes.sh
+++ b/src/commands/git_secret_changes.sh
@@ -32,9 +32,6 @@ function changes {
   '
 
   for filename in "${filenames[@]}"; do
-    local decrypted
-    local diff_result
-
     local path # absolute path
     local normalized_path # relative to the .git dir
     local encrypted_filename
@@ -55,14 +52,18 @@ function changes {
         _abort "file not found. Consider using 'git secret reveal': $filename"
     fi
 
-    # Now we have all the data required:
-    decrypted=$(_decrypt "$path" "0" "0" "$homedir" "$passphrase")
+    # Now we have all the data required to do the last encryption and compare results:
+    # now do a two-step to protect trailing newlines from the $() construct.
+    local decrypted_x
+    local decrypted
+    decrypted_x=$(_decrypt "$path" "0" "0" "$homedir" "$passphrase"; echo x$?)
+    decrypted="${decrypted_x%x*}"
+    # we ignore the exit code because _decrypt will _abort if appropriate.
+
 
-    # Let's diff the result:
-    diff_result=$(diff -u <(echo "$decrypted") "$path") || true
-    # There was a bug in the previous version, since `diff` returns
-    # exit code `1` when the files are different.
     echo "changes in ${path}:"
-    echo "${diff_result}"
+    # diff the result:
+    # we have the '|| true' because `diff` returns error code if files differ.
+    diff -u <(echo -n "$decrypted") "$path" || true
   done
 }
diff --git a/tests/_test_base.bash b/tests/_test_base.bash
index 2fe94d6e..3921b3e6 100644
--- a/tests/_test_base.bash
+++ b/tests/_test_base.bash
@@ -214,7 +214,16 @@ function set_state_secret_tell {
 function set_state_secret_add {
   local filename="$1"
   local content="$2"
-  echo "$content" > "$filename"
+  echo "$content" > "$filename"      # we add a newline
+  echo "$filename" >> ".gitignore"
+
+  git secret add "$filename" > /dev/null 2>&1
+}
+
+function set_state_secret_add_without_newline {
+  local filename="$1"
+  local content="$2"
+  echo -n "$content" > "$filename"      # we do not add a newline
   echo "$filename" >> ".gitignore"
 
   git secret add "$filename" > /dev/null 2>&1
diff --git a/tests/test_cat.bats b/tests/test_cat.bats
index eab781fd..4de07937 100644
--- a/tests/test_cat.bats
+++ b/tests/test_cat.bats
@@ -33,7 +33,6 @@ function teardown {
   [ "$status" -eq 0 ]
 
   # $output is the output from 'git secret cat' above
-  # note that currently content may differ by a newline
   [ "$FILE_CONTENTS" == "$output" ]
 }
 
diff --git a/tests/test_changes.bats b/tests/test_changes.bats
index eee8ef9e..dee7cc7d 100644
--- a/tests/test_changes.bats
+++ b/tests/test_changes.bats
@@ -4,6 +4,8 @@ load _test_base
 
 FILE_TO_HIDE="$TEST_DEFAULT_FILENAME"
 SECOND_FILE_TO_HIDE="$TEST_SECOND_FILENAME"
+THIRD_FILE_TO_HIDE="$TEST_THIRD_FILENAME"
+FILE_NON_EXISTANT="NO-SUCH-FILE"
 FILE_CONTENTS="hidden content юникод"
 
 FINGERPRINT=""
@@ -29,6 +31,16 @@ function teardown {
   unset_current_state
 }
 
+@test "run 'changes' on one file with no file changed" {
+  local password=$(test_user_password "$TEST_DEFAULT_USER")
+  run git secret changes -d "$TEST_GPG_HOMEDIR" -p "$password" "$FILE_TO_HIDE"
+
+  [ "$status" -eq 0 ]
+
+  local num_lines=$(echo "$output" | wc -l)
+  [[ "$num_lines" -eq 1 ]]
+}
+
 
 @test "run 'changes' with one file changed" {
   local password=$(test_user_password "$TEST_DEFAULT_USER")
@@ -41,7 +53,12 @@ function teardown {
   # Testing that output has both filename and changes:
   local fullpath=$(_append_root_path "$FILE_TO_HIDE")
   [[ "$output" == *"changes in $fullpath"* ]]
+  [[ "$output" == *"hidden content юникод"* ]]
   [[ "$output" == *"+$new_content"* ]]
+
+  local num_lines=$(echo "$output" | wc -l)
+  [[ "$num_lines" -eq 6 ]]
+
 }
 
 @test "run 'changes' with source file missing" {
@@ -78,11 +95,16 @@ function teardown {
 }
 
 
-@test "run 'changes' without changes" {
+@test "run 'changes' on two files with no file changed" {
   local password=$(test_user_password "$TEST_DEFAULT_USER")
 
   run git secret changes -d "$TEST_GPG_HOMEDIR" -p "$password"
+
+
   [ "$status" -eq 0 ]
+
+  local num_lines=$(echo "$output" | wc -l)
+  [[ "$num_lines" -eq 2 ]]   
 }
 
 
@@ -96,12 +118,8 @@ function teardown {
   run git secret changes -d "$TEST_GPG_HOMEDIR" -p "$password"
   [ "$status" -eq 0 ]
 
-  #echo "# output is '$output'" >&3
-  #echo "# " >&3
-
   # Testing that output has both filename and changes:
   local fullpath=$(_append_root_path "$FILE_TO_HIDE")
-  #echo "# fullpath is $fullpath" >&3
 
   [[ "$output" == *"changes in $fullpath"* ]]
   [[ "$output" == *"+$new_content"* ]]
@@ -133,3 +151,20 @@ function teardown {
   [[ "$output" == *"changes in $second_path"* ]]
   [[ "$output" == *"+$second_new_content"* ]]
 }
+
+@test "run 'changes' on file that does not exist" {
+  run git secret changes -d "$TEST_GPG_HOMEDIR" -p "$password" "$FILE_NON_EXISTANT"
+  [ "$status" -ne 0 ]
+}
+
+@test "run 'changes' on one file without newlines" {
+  set_state_secret_add_without_newline "$THIRD_FILE_TO_HIDE" "$FILE_CONTENTS"
+  set_state_secret_hide
+
+  local password=$(test_user_password "$TEST_DEFAULT_USER")
+  run git secret changes -d "$TEST_GPG_HOMEDIR" -p "$password" "$THIRD_FILE_TO_HIDE"
+  [ "$status" -eq 0 ]
+
+  local num_lines=$(echo "$output" | wc -l)
+  [[ "$num_lines" -eq 1 ]]
+}
diff --git a/tests/test_expiration.bats b/tests/test_expiration.bats
index 1c549d07..25aa6b9e 100644
--- a/tests/test_expiration.bats
+++ b/tests/test_expiration.bats
@@ -26,7 +26,7 @@ function teardown {
   run git secret hide   
   # this will fail, because we're using an expired key
 
-  echo "# output of hide: $output" >&3
+  echo "$output" | sed "s/^/# '$BATS_TEST_DESCRIPTION' output: /" >&3
     # output will look like 'abort: problem encrypting file with gpg: exit code 2: space file'
   #echo "# status of hide: $status" >&3
 
@@ -39,10 +39,10 @@ function teardown {
   [ "$status" -eq 0 ]
 
   # diag output for bats-core
-  echo "# output of 'whoknows -l: $output" >&3
-  echo >&3
-    # output should look like 'abort: problem encrypting file with gpg: exit code 2: space file'
-  #echo "# status of hide: $status" >&3
+  echo "$output" | sed "s/^/# '$BATS_TEST_DESCRIPTION' output: /" >&3
+  # output should look like 'abort: problem encrypting file with gpg: exit code 2: space file'
+  
+  #echo "# $BATS_TEST_DESCRIPTION: $status" >&3
 
   # Now test the output, both users should be present:
   [[ "$output" == *"$TEST_EXPIRED_USER (expires: 2018-09-23)"* ]]
@@ -57,8 +57,7 @@ function teardown {
   run git secret whoknows -l
   [ "$status" -eq 0 ]
 
-  echo $output | sed 's/^/# whoknows -l: /' >&3
-  echo >&3
+  echo "$output" | sed "s/^/# '$BATS_TEST_DESCRIPTION' output: /" >&3
 
   # Now test the output, both users should be present:
   [[ "$output" == *"$TEST_DEFAULT_USER (expires: never)"* ]]
diff --git a/tests/test_hide.bats b/tests/test_hide.bats
index 721284d1..0baa3b1e 100644
--- a/tests/test_hide.bats
+++ b/tests/test_hide.bats
@@ -28,7 +28,7 @@ function teardown {
 @test "run 'hide' normally" {
   run git secret hide
 
-  #echo "# run hide normally: output: $output" >&3
+  #echo "$output" | sed "s/^/# '$BATS_TEST_DESCRIPTION' output: /" >&3
 
   # Command must execute normally:
   [ "$status" -eq 0 ]
@@ -46,7 +46,7 @@ function teardown {
 
   run git secret hide -P
 
-  #echo "# run hide with -P: output: $output" >&3
+  #echo "$output" | sed "s/^/# '$BATS_TEST_DESCRIPTION' output: /" >&3
 
   # Command must execute normally:
   [ "$status" -eq 0 ]
@@ -63,7 +63,7 @@ function teardown {
   file_perm=$(ls -l "$FILE_TO_HIDE" | cut -d' ' -f1)
 
   # text prefixed with '# ' and sent to file descriptor 3 is 'diagnostic' (debug) output for devs
-  #echo "# secret_perm: $secret_perm, file_perm: $file_perm" >&3
+  #echo "# '$BATS_TEST_DESCRIPTION': $secret_perm, file_perm: $file_perm" >&3
 
   [ "$secret_perm" = "$file_perm" ]
 
@@ -115,7 +115,7 @@ function teardown {
 
   # Now it should hide 2 files:
   run git secret hide
-  #echo "# run hide with multiple files: output: $output" >&3
+  #echo "$output" | sed "s/^/# '$BATS_TEST_DESCRIPTION' output: /" >&3
   [ "$status" -eq 0 ]
   [ "$output" = "done. 2 of 2 files are hidden." ]
 
@@ -145,7 +145,7 @@ function teardown {
   path_mappings=$(_get_secrets_dir_paths_mapping)
   run git secret hide -m
 
-  #echo "# run hide with -m twice: output: $output" >&3
+  echo "$output" | sed "s/^/# '$BATS_TEST_DESCRIPTION' output: /" >&3
 
   # Command must execute normally:
   [ "$status" -eq 0 ]
diff --git a/tests/test_whoknows.bats b/tests/test_whoknows.bats
index 0e529bc8..7221366b 100644
--- a/tests/test_whoknows.bats
+++ b/tests/test_whoknows.bats
@@ -35,10 +35,9 @@ function teardown {
   run git secret whoknows -l
   [ "$status" -eq 0 ]
 
-  echo "# output of 'whoknows -l: $output" >&3
-  echo >&3
+  echo "$output" | sed "s/^/# '$BATS_TEST_DESCRIPTION' output: /" >&3
     # output should look like 'abort: problem encrypting file with gpg: exit code 2: space file'
-  #echo "# status of hide: $status" >&3
+  #echo "# '$BATS_TEST_DESCRIPTION' status: $status" >&3
 
   # Now test the output, both users should be present and without expiration
   [[ "$output" == *"$TEST_DEFAULT_USER (expires: never)"* ]]