Fix pull not symlinking new and renamed files

pull creates a temporary tag in each castle before running git pull, and
uses it to determine which files have been added (and renamed) in
symlink_new_files, which also deletes it.

bin/homeshick

@@ -182,7 +182,7 @@ case $cmd in
         refresh)
           (refresh $threshhold "$param") ;;
         pull)
-          (pull "$param") ;;
+          (pull "$param") && pull_completed+=("$param") ;;
         symlink|link)
           symlink "$param" ;;
         track)
@@ -202,7 +202,7 @@ case $cmd in
       refresh)
         pull_outdated $threshhold "${params[@]}" ;;
       pull)
-        symlink_new_files "${params[@]}" ;;
+        symlink_new_files "${pull_completed[@]}" ;;
     esac
     result=$?
     if [[ $exit_status == 0 && $result != 0 ]]; then

lib/commands/pull.sh

@@ -1,5 +1,6 @@
 #!/usr/bin/env bash
 
+BEFORE_PULL_TAG=__homeshick-before-pull__
 pull() {
   [[ ! $1 ]] && help_err pull
   local castle=$1
@@ -13,6 +14,15 @@ pull() {
     return "$EX_SUCCESS"
   fi
 
+  # this tag is exceedingly unlikely to already exist, but if it does, error
+  # out and let the user resolve it
+  (cd "$repo" && git rev-parse --verify "refs/tags/$BEFORE_PULL_TAG" &>/dev/null) && \
+    err "$EX_DATAERR" "Pull marker tag ($BEFORE_PULL_TAG) already exists in $repo. Please resolve this before pulling."
+  # make a tag at the current commit, so we can compare against it below
+  (cd "$repo" && git tag "$BEFORE_PULL_TAG" 2>&1)
+  # remove the tag if one of the git operations fails
+  trap '{ cd "$repo" && git tag -d "$BEFORE_PULL_TAG" &>/dev/null; }' EXIT
+
   local git_out
   git_out=$(cd "$repo" && git pull 2>&1) || \
     err "$EX_SOFTWARE" "Unable to pull $repo. Git says:" "$git_out"
@@ -26,6 +36,7 @@ pull() {
       err "$EX_SOFTWARE" "Unable update submodules for $repo. Git says:" "$git_out"
   fi
   success
+  trap - EXIT
   return "$EX_SUCCESS"
 }
 
@@ -35,13 +46,15 @@ symlink_new_files() {
     local castle=$1
     shift
     local repo="$repos/$castle"
+    local before_pull
+    if ! before_pull=$(cd "$repo" && git rev-parse "refs/tags/$BEFORE_PULL_TAG" && git tag -d "$BEFORE_PULL_TAG" >/dev/null); then
+      continue
+    fi
     if [[ ! -d $repo/home ]]; then
       continue;
     fi
     local git_out
-    local now
-    now=$(date +%s)
-    if ! git_out=$(cd "$repo" && git diff --name-only --diff-filter=A "HEAD@{(($now-$T_START+1)).seconds.ago}" HEAD -- home 2>/dev/null | wc -l 2>&1); then
+    if ! git_out=$(cd "$repo" && git diff --name-only --diff-filter=AR "$before_pull" HEAD -- home 2>/dev/null | wc -l 2>&1); then
       continue  # Ignore errors, this operation is not mission critical
     fi
     if [[ $git_out -gt 0 ]]; then

test/fixtures/pull-renamed.sh

@@ -0,0 +1,32 @@
+#!/usr/bin/env bash
+
+# shellcheck disable=2164
+fixture_pull_renamed() {
+  local git_username="Homeshick user"
+  local git_useremail="[email protected]"
+  local repo="$REPO_FIXTURES/pull-renamed"
+  git init "$repo"
+  cd "$repo"
+  git config user.name "$git_username"
+  git config user.email "$git_useremail"
+  mkdir home
+  cd home
+
+  cat > .bashrc-wrong-name <<EOF
+#!/usr/bin/env bash
+PS1='\[33[01;32m\]\u@\h\[33[00m\]:\[33[01;34m\]\w\'
+EOF
+  git add .bashrc-wrong-name
+  git commit -m '.bashrc file for my new repo'
+
+  git mv .bashrc-wrong-name .bashrc
+  git commit -m 'fixed .bashrc file name'
+
+  cat >> .bashrc <<EOF
+export IMPORTANT_VARIABLE=1
+EOF
+  git add .bashrc
+  git commit -m 'Modified .bashrc to set IMPORTANT_VARIABLE'
+}
+
+fixture_pull_renamed  > /dev/null

test/fixtures/pull-test.sh

@@ -0,0 +1,36 @@
+#!/usr/bin/env bash
+
+# shellcheck disable=2164
+fixture_pull_test() {
+  local git_username="Homeshick user"
+  local git_useremail="[email protected]"
+  local repo="$REPO_FIXTURES/pull-test"
+  git init "$repo"
+  cd "$repo"
+  git config user.name "$git_username"
+  git config user.email "$git_useremail"
+  mkdir home
+  cd home
+
+  cat > .bashrc <<EOF
+#!/usr/bin/env bash
+PS1='\[33[01;32m\]\u@\h\[33[00m\]:\[33[01;34m\]\w\'
+EOF
+  git add .bashrc
+  git commit -m '.bashrc file for my new pull-test repo'
+
+  cat > .gitignore <<EOF
+.DS_Store
+*.swp
+EOF
+  git add .gitignore
+  git commit -m 'Added .gitignore file'
+
+  cat >> .bashrc <<EOF
+export IMPORTANT_VARIABLE=1
+EOF
+  git add .bashrc
+  git commit -m 'Modified .bashrc to set IMPORTANT_VARIABLE'
+}
+
+fixture_pull_test  > /dev/null

test/suites/pull.bats

@@ -12,6 +12,92 @@ teardown() {
   delete_test_dir
 }
 
+BEFORE_PULL_TAG=__homeshick-before-pull__
+assert_tag_is_removed() {
+  local castle
+  for castle in "$@"; do
+    (
+      cd "$HOME/.homesick/repos/$castle" || return $?
+      # show all the tags if the test fails
+      {
+        echo "all tags in $castle:"
+        git show-ref --tags || true
+        echo
+      } >&2
+      # this tag should not exist
+      run git rev-parse --verify "refs/tags/$BEFORE_PULL_TAG" >&2 2>&-
+      assert_failure
+    )
+  done
+}
+
+assert_tag_points_to() {
+  (
+    castle="$1"
+    tag_before="$2"
+    cd "$HOME/.homesick/repos/$castle" || return $?
+    # show all the tags if the test fails
+    {
+      echo "all tags in $castle:"
+      git show-ref --tags || true
+      echo
+    } >&2
+    tag_after=$(git rev-parse "refs/tags/$BEFORE_PULL_TAG")
+    [ "$tag_before" == "$tag_after" ]
+  )
+}
+
+reset_and_add_new_file() {
+  # takes castle name as first argument and commit to reset to as second
+  cd "$HOME/.homesick/repos/$1" || return $?
+  git reset --hard "$2" >/dev/null
+
+  git config user.name "Homeshick user"
+  git config user.email "[email protected]"
+
+  cat > home/.ignore <<EOF
+.DS_Store
+*.swp
+EOF
+  git add home/.ignore >/dev/null
+  git commit -m 'Added .ignore file' >/dev/null
+}
+
+expect_new_files() {
+  # takes castle name as first argument, and new files as remaining arguments
+  local castle="$1"
+  shift
+  local green='\e[1;32m'
+  local cyan='\e[1;36m'
+  local white='\e[1;37m'
+  local reset='\e[0m'
+  # these variables are intended to be parsed by printf
+  # shellcheck disable=SC2059
+  {
+    printf  "$cyan         pull$reset %s\r" "$castle"
+    printf "$green         pull$reset %s\n" "$castle"
+    printf "$white      updates$reset The castle %s has new files.\n" "$castle"
+    printf  "$cyan     symlink?$reset [yN] y\r"
+    printf "$green     symlink?$reset [yN] \n"
+    for file in "$@"; do
+    printf  "$cyan      symlink$reset %s\r" "$file"
+    printf "$green      symlink$reset %s\n" "$file"
+    done
+  } | assert_output -
+}
+
+expect_no_new_files() {
+  # takes castle name as first argument
+  local castle="$1"
+  local green='\e[1;32m'
+  local cyan='\e[1;36m'
+  local reset='\e[0m'
+  {
+    printf  "$cyan         pull$reset %s\r" "$castle"
+    printf "$green         pull$reset %s\n" "$castle"
+  } | assert_output -
+}
+
 @test 'pull skips castles with no upstream remote' {
   castle 'rc-files'
   castle 'dotfiles'
@@ -20,6 +106,157 @@ teardown() {
   (cd "$HOME/.homesick/repos/rc-files" && git remote rm origin)
   run homeshick pull rc-files dotfiles
   [ $status -eq 0 ] # EX_SUCCESS
+  assert_tag_is_removed rc-files dotfiles
   # dotfiles FETCH_HEAD should exist if the castle was pulled
   [ -e "$HOME/.homesick/repos/dotfiles/.git/FETCH_HEAD" ]
 }
+
+@test 'pull prompts for symlinking if new files are present' {
+  castle 'rc-files'
+  (cd "$HOME/.homesick/repos/rc-files" && git reset --hard HEAD~1 >/dev/null)
+  homeshick link --batch --quiet rc-files
+
+  [ ! -e "$HOME/.gitignore" ]
+  run homeshick pull rc-files <<<y
+  assert_success
+  assert_tag_is_removed rc-files
+  expect_new_files rc-files .gitignore
+  [ -f "$HOME/.gitignore" ]
+}
+
+@test 'pull prompts for symlinking with renamed files' {
+  castle 'pull-renamed'
+  # reset to before .bashrc-wrong-name was renamed to .bashrc
+  (cd "$HOME/.homesick/repos/pull-renamed" && git reset --hard HEAD~2 >/dev/null)
+
+  [ ! -e "$HOME/.bashrc" ]
+  run homeshick pull pull-renamed <<<y
+  assert_success
+  assert_tag_is_removed pull-renamed
+  expect_new_files pull-renamed .bashrc
+  [ -f "$HOME/.bashrc" ]
+}
+
+@test 'pull with no new files present' {
+  castle 'pull-test'
+  (cd "$HOME/.homesick/repos/pull-test" && git reset --hard HEAD~1 >/dev/null)
+
+  run homeshick pull --batch pull-test
+  assert_success
+  assert_tag_is_removed pull-test
+  expect_no_new_files pull-test
+}
+
+@test 'pull a recently-pulled castle again' {
+  # this checks that we don't try to link files again if the last operation was
+  # a pull
+  castle 'rc-files'
+  (cd "$HOME/.homesick/repos/rc-files" && git reset --hard HEAD~1 >/dev/null)
+  homeshick pull --batch --force rc-files
+
+  run homeshick pull --batch rc-files
+  assert_success
+  assert_tag_is_removed rc-files
+  expect_no_new_files rc-files
+}
+
+@test 'pull a castle with a git conflict' {
+  local castle=pull-test
+  castle 'pull-test'
+  (reset_and_add_new_file pull-test HEAD~2)
+  (cd "$HOME/.homesick/repos/pull-test" && git config pull.rebase false && git config pull.ff only)
+
+  [ ! -e "$HOME/.gitignore" ]
+  run homeshick pull --batch pull-test
+  assert_failure 70 # EX_SOFTWARE
+  assert_tag_is_removed pull-test
+  [ ! -e "$HOME/.gitignore" ]
+  local red='\e[1;31m'
+  local cyan='\e[1;36m'
+  local reset='\e[0m'
+  {
+    echo -ne "$cyan         pull$reset pull-test\r"
+    echo -ne  "$red         pull$reset pull-test\n"
+    echo -ne  "$red        error$reset Unable to pull $HOME/.homesick/repos/pull-test. Git says:\n"
+  } | assert_output -p -
+  # possibly some git advice
+  assert_output -p "fatal: Not possible to fast-forward, aborting."
+}
+
+@test 'pull cleans up any marker tags after a git error' {
+  castle 'dotfiles'
+  castle 'pull-test'
+  castle 'nodirs'
+  (reset_and_add_new_file pull-test HEAD~2)
+  (cd "$HOME/.homesick/repos/pull-test" && git config pull.rebase false && git config pull.ff only)
+
+  run homeshick pull --batch dotfiles pull-test nodirs
+  assert_failure 70 # EX_SOFTWARE
+  # tags in all castles should be cleaned up
+  assert_tag_is_removed dotfiles pull-test nodirs
+  local green='\e[1;32m'
+  local red='\e[1;31m'
+  local cyan='\e[1;36m'
+  local reset='\e[0m'
+  {
+    echo -ne  "$cyan         pull$reset dotfiles\r"
+    echo -ne "$green         pull$reset dotfiles\n"
+    echo -ne  "$cyan         pull$reset pull-test\r"
+    echo -ne   "$red         pull$reset pull-test\n"
+    echo -ne   "$red        error$reset Unable to pull $HOME/.homesick/repos/pull-test. Git says:\n"
+  } | assert_output -p -
+  # possibly some git advice
+  {
+    echo "fatal: Not possible to fast-forward, aborting."
+    echo -ne  "$cyan         pull$reset nodirs\r"
+    echo -ne "$green         pull$reset nodirs\n"
+  } | assert_output -p -
+}
+
+@test 'pull a castle where the marker tag already exists' {
+  castle 'rc-files'
+  local tag_before
+  tag_before=$(
+    cd "$HOME/.homesick/repos/rc-files" &&
+    git reset --hard HEAD~1 >/dev/null &&
+    git tag "$BEFORE_PULL_TAG" HEAD^ &&
+    git rev-parse "$BEFORE_PULL_TAG"
+  )
+
+  [ ! -e "$HOME/.gitignore" ]
+  run homeshick pull --batch rc-files
+  assert_failure 65 # EX_DATAERR
+  # tag should not be touched
+  assert_tag_points_to rc-files "$tag_before"
+  [ ! -e "$HOME/.gitignore" ]
+
+  local red='\e[1;31m'
+  local cyan='\e[1;36m'
+  local reset='\e[0m'
+  {
+    echo -ne "$cyan         pull$reset rc-files\r"
+    echo -ne  "$red         pull$reset rc-files\n"
+    echo -ne  "$red        error$reset Pull marker tag ($BEFORE_PULL_TAG) already exists in $HOME/.homesick/repos/rc-files. Please resolve this before pulling."
+  } | assert_output -
+}
+
+@test 'pull only cleans up the marker tag if we created it' {
+  castle 'dotfiles'
+  castle 'rc-files'
+  local tag_before
+  tag_before=$(
+    cd "$HOME/.homesick/repos/rc-files" &&
+    git reset --hard HEAD~1 >/dev/null &&
+    git tag "$BEFORE_PULL_TAG" HEAD^ &&
+    git rev-parse "refs/tags/$BEFORE_PULL_TAG"
+  )
+
+  [ ! -e "$HOME/.gitignore" ]
+  run homeshick pull --batch dotfiles rc-files
+  assert_failure 65 # EX_DATAERR
+  # tag in dotfiles should be cleaned up
+  assert_tag_is_removed dotfiles
+  # tag in rc-files should not be touched
+  assert_tag_points_to rc-files "$tag_before"
+  [ ! -e "$HOME/.gitignore" ]
+}