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.

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,13 @@ pull() {
     return "$EX_SUCCESS"
   fi
 
+  # this tag is exceedingly unlikely to already exist, but if it does, stop
+  # immediately with EX_USAGE and let the user resolve it
+  (cd "$repo" && git rev-parse --verify "refs/tags/$BEFORE_PULL_TAG" &>/dev/null) && \
+    err "$EX_USAGE" "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)
+
   local git_out
   git_out=$(cd "$repo" && git pull 2>&1) || \
     err "$EX_SOFTWARE" "Unable to pull $repo. Git says:" "$git_out"
@@ -35,13 +43,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="homeshick@example.com"
+  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="homeshick@example.com"
+  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,86 @@ teardown() {
   delete_test_dir
 }
 
+backdate_git_operations() {
+  # This function changes the time of all git operations in the current
+  # subshell to be several (first argument, defaults to 5) seconds in the past.
+  offset=${1:-5}
+  local timestamp
+  timestamp=$(( $(date +%s) - offset ))
+  # this is what is usually displayed by git log
+  export GIT_AUTHOR_DATE="@$timestamp"
+  # this is what most git commands actually care about (like @{1 second ago})
+  export GIT_COMMITTER_DATE="@$timestamp"
+}
+
+BEFORE_PULL_TAG=__homeshick-before-pull__
+assert_tag_is_removed() {
+  for castle in "$@"; do
+    (
+      cd "$HOME/.homesick/repos/$castle" || return $?
+      # show all the tags if the test fails
+      git show-ref --tags >&2 || true
+      # this tag should not exist
+      run git rev-parse --verify "refs/tags/$BEFORE_PULL_TAG" >&2 2>&-
+      assert_failure
+    )
+  done
+}
+
+reset_and_add_new_file() {
+  (
+    backdate_git_operations 3
+    cd "$HOME/.homesick/repos/pull-test" || return $?
+    git reset --hard "$1" >/dev/null
+
+    git config user.name "Homeshick user"
+    git config user.email "homeshick@example.com"
+
+    cat > home/.ignore <<EOF
+.DS_Store
+*.swp
+EOF
+    git add home/.ignore >/dev/null
+    git commit -m 'Added .ignore file' >/dev/null
+  )
+  homeshick link --batch pull-test >/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 +100,131 @@ 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' {
+  local castle=rc-files
+  (
+    # make these operations happen several seconds in the past, so that
+    # symlink_new_files can tell what commits are new
+    backdate_git_operations
+    castle "$castle"
+    (cd "$HOME/.homesick/repos/$castle" && git reset --hard HEAD~1 >/dev/null)
+    homeshick link --batch --quiet "$castle"
+  )
+
+  [ ! -e "$HOME/.gitignore" ]
+  run homeshick pull "$castle" <<<y
+  assert_success
+  assert_tag_is_removed "$castle"
+  expect_new_files "$castle" .gitignore
+  [ -f "$HOME/.gitignore" ]
+}
+
+@test 'pull prompts for symlinking with renamed files' {
+  local castle=pull-renamed
+  (
+    backdate_git_operations
+    castle "$castle"
+    # reset to before .bashrc-wrong-name was renamed to .bashrc
+    (cd "$HOME/.homesick/repos/$castle" && git reset --hard HEAD~2 >/dev/null)
+    homeshick link --batch --quiet "$castle"
+  )
+
+  [ ! -e "$HOME/.bashrc" ]
+  run homeshick pull "$castle" <<<y
+  assert_success
+  assert_tag_is_removed "$castle"
+  expect_new_files "$castle" .bashrc
+  [ -f "$HOME/.bashrc" ]
+}
+
+@test 'pull with no new files present' {
+  local castle=pull-test
+  (
+    backdate_git_operations
+    castle "$castle"
+    (cd "$HOME/.homesick/repos/$castle" && git reset --hard HEAD~1 >/dev/null)
+  )
+
+  run homeshick pull --batch "$castle"
+  assert_success
+  assert_tag_is_removed "$castle"
+  expect_no_new_files "$castle"
+}
+
+@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
+  local castle=rc-files
+  (
+    backdate_git_operations
+    castle "$castle"
+    (cd "$HOME/.homesick/repos/$castle" && git reset --hard HEAD~1 >/dev/null)
+    homeshick link --batch --quiet "$castle"
+    backdate_git_operations 2
+    homeshick pull --batch --force "$castle"
+  )
+
+  run homeshick pull --batch "$castle"
+  assert_success
+  assert_tag_is_removed "$castle"
+  expect_no_new_files "$castle"
+}
+
+@test 'pull a castle with a git conflict' {
+  local castle=pull-test
+  (
+    backdate_git_operations
+    castle "$castle"
+    reset_and_add_new_file HEAD~2
+    (cd "$HOME/.homesick/repos/$castle" && git config pull.rebase false && git config pull.ff only)
+  )
+
+  [ ! -e "$HOME/.gitignore" ]
+  run homeshick pull --batch "$castle"
+  assert_failure 70 # EX_SOFTWARE
+  assert_tag_is_removed "$castle"
+  [ ! -e "$HOME/.gitignore" ]
+  local red='\e[1;31m'
+  local cyan='\e[1;36m'
+  local reset='\e[0m'
+  {
+    echo -ne "$cyan         pull$reset $castle\r"
+    echo -ne  "$red         pull$reset $castle\n"
+    echo -ne  "$red        error$reset Unable to pull $HOME/.homesick/repos/$castle. Git says:"
+  } | assert_output -p -
+}
+
+@test 'pull a castle where the marker tag already exists' {
+  local castle=rc-files
+  local tag_before tag_after
+  tag_before=$(
+    backdate_git_operations
+    castle "$castle"
+    cd "$HOME/.homesick/repos/$castle" &&
+    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 "$castle"
+  assert_failure 64 # EX_USAGE
+  # tag should not be touched
+  tag_after=$(cd "$HOME/.homesick/repos/$castle" && git rev-parse "$BEFORE_PULL_TAG")
+  [ "$tag_before" == "$tag_after" ]
+  [ ! -e "$HOME/.gitignore" ]
+
+  local red='\e[1;31m'
+  local cyan='\e[1;36m'
+  local reset='\e[0m'
+  {
+    echo -ne "$cyan         pull$reset $castle\r"
+    echo -ne  "$red         pull$reset $castle\n"
+    echo -ne  "$red        error$reset Pull marker tag ($BEFORE_PULL_TAG) already exists in $HOME/.homesick/repos/$castle. Please resolve this before pulling."
+  } | assert_output -
+}