[v2,0/3] Fix D/F issues in stash

Message

Philippe Blain via GitGitGadget Sept. 10, 2021, 10:29 a.m. UTC

This series fixes a few D/F issues in the stash command. These were some
issues I found while working on unintentional removal of untracked
files/directories and the current working directory, and I'm just submitting
them separately.

Changes since v1:

 * Fix accidental creation of file named 'expect' (copy-paste problem...)
 * Documented the reason for adding is_path_a_directory() and not using
   is_directory()
 * Removed typo, fixed up confusing wording, and added a companion test to
   show that F->D and D->F have the same fix.

Elijah Newren (3):
  t3903: document a pair of directory/file bugs
  stash: avoid feeding directories to update-index
  stash: restore untracked files AFTER restoring tracked files

 builtin/stash.c  | 20 ++++++++++++++---
 t/t3903-stash.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+), 3 deletions(-)


base-commit: e0a2f5cbc585657e757385ad918f167f519cfb96
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1086%2Fnewren%2Fstash-df-fixes-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1086/newren/stash-df-fixes-v2
Pull-Request: https://github.com/git/git/pull/1086

Range-diff vs v1:

 1:  bc66a6ae75d ! 1:  5ddb70d332b t3903: document a pair of directory/file bugs
     @@ Metadata
       ## Commit message ##
          t3903: document a pair of directory/file bugs
      
     +    There are three tests here, because the second bug is documented with
     +    two tests: a file -> directory change and a directory -> file change.
     +    The reason for the two tests is just to verify that both are indeed
     +    broken but that both will be fixed by the same simple change (which will
     +    be provided in a subsequent patch).
     +
          Signed-off-by: Elijah Newren <[e-mail]>
      
       ## t/t3903-stash.sh ##
     @@ t/t3903-stash.sh: test_expect_success 'stash -c stash.useBuiltin=false warning '
      +		git rm filler &&
      +		mkdir filler &&
      +		echo contents >filler/file &&
     -+		cp filler/file expect &&
      +		git stash push
      +	)
      +'
      +
     -+test_expect_failure 'git stash can pop directory/file saved changes' '
     ++test_expect_failure 'git stash can pop file -> directory saved changes' '
      +	test_create_repo directory_file_switch_v2 &&
      +	(
      +		cd directory_file_switch_v2 &&
     @@ t/t3903-stash.sh: test_expect_success 'stash -c stash.useBuiltin=false warning '
      +		test_cmp expect filler/file
      +	)
      +'
     ++
     ++test_expect_failure 'git stash can pop directory -> file saved changes' '
     ++	test_create_repo directory_file_switch_v3 &&
     ++	(
     ++		cd directory_file_switch_v3 &&
     ++		test_commit init &&
     ++
     ++		mkdir filler &&
     ++		test_write_lines some words >filler/file1 &&
     ++		test_write_lines and stuff >filler/file2 &&
     ++		git add filler &&
     ++		git commit -m filler &&
     ++
     ++		git rm -rf filler &&
     ++		echo contents >filler &&
     ++		cp filler expect &&
     ++		git stash push --include-untracked &&
     ++		git stash apply --index &&
     ++		test_cmp expect filler
     ++	)
     ++'
      +
       test_done
 2:  c7f5ae66a92 ! 2:  31e38c6c33c stash: avoid feeding directories to update-index
     @@ builtin/stash.c: static int reset_head(void)
       
      +static int is_path_a_directory(const char *path)
      +{
     ++	/*
     ++	 * This function differs from abspath.c:is_directory() in that
     ++	 * here we use lstat() instead of stat(); we do not want to
     ++	 * follow symbolic links here.
     ++	 */
      +	struct stat st;
      +	return (!lstat(path, &st) && S_ISDIR(st.st_mode));
      +}
 3:  ac8ca07481d ! 3:  6254938948c stash: restore untracked files AFTER restoring tracked files
     @@ Commit message
          removed.  So, restore changes to tracked files before restoring
          untracked files.
      
     -    There is no similar problem to worry about in the opposite directory,
     -    because untracked files are always additive.  Said another way, there's
     -    no way to "stash a removal of an untracked file" because if an untracked
     -    file is removed, git simply doesn't know about it.
     +    There is no counterpart problem to worry about with the user deleting an
     +    untracked file and then add a tracked one in its place.  Git does not
     +    track untracked files, and so will not know the untracked file was
     +    deleted, and thus won't be able to stash the removal of that file.
      
          Signed-off-by: Elijah Newren <[e-mail]>
      
     @@ t/t3903-stash.sh: test_expect_success 'git stash succeeds despite directory/file
       	)
       '
       
     --test_expect_failure 'git stash can pop directory/file saved changes' '
     -+test_expect_success 'git stash can pop directory/file saved changes' '
     +-test_expect_failure 'git stash can pop file -> directory saved changes' '
     ++test_expect_success 'git stash can pop file -> directory saved changes' '
       	test_create_repo directory_file_switch_v2 &&
       	(
       		cd directory_file_switch_v2 &&
     +@@ t/t3903-stash.sh: test_expect_failure 'git stash can pop file -> directory saved changes' '
     + 	)
     + '
     + 
     +-test_expect_failure 'git stash can pop directory -> file saved changes' '
     ++test_expect_success 'git stash can pop directory -> file saved changes' '
     + 	test_create_repo directory_file_switch_v3 &&
     + 	(
     + 		cd directory_file_switch_v3 &&