Re: [PATCH v1 1/1] git stash needing mkdir deletes untracked file

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Torsten

Thanks for working on this. I've cc'd Junio for his unpack_trees() 
knowledge.

On 08/08/2023 18:26, tboegi@web.de wrote:
> From: Torsten Bögershausen <tboegi@web.de>
> 
> The following sequence leads to loss of work:
>   git init
>   mkdir README
>   touch README/README
>   git add .
>   git commit -m "Init project"
>   echo "Test" > README/README
>   mv README/README README2
>   rmdir README
>   mv README2 README
>   git stash
>   git stash pop
> 
> The problem is, that `git stash` needs to create the directory README/
> and to be able to do this, the file README needs to be removed.
> And this is, where the work was lost.
> There are different possibilities preventing this loss of work:
> a)
>    `git stash` does refuse the removel of the untracked file,
>     when a directory with the same name needs to be created
>    There is a small problem here:
>    In the ideal world, the stash would do nothing at all,
>    and not do anything but complain.
>    The current code makes this hard to achieve
>    An other solution could be to do as much stash work as possible,
>    but stop when the file/directory conflict is detected.
>    This would create some inconsistent state.
> 
> b) Create the directory as needed, but rename the file before doing that.
>    This would let the `git stash` proceed as usual and create a "new" file,
>    which may be surprising for some worlflows.
> 
> This change goes for b), as it seems the most intuitive solution for
> Git users.
> 
> Introdue a new function rename_to_untracked_or_warn() and use it
> in create_directories() in entry.c

Although this change is framed in terms of changes to "git stash push" I 
think the underlying issue and this patch actually affects all users of 
unpack_trees(). For example if "README" is untracked then

	git checkout <rev> README

will currently fail if <rev>:README is a blob but will succeed and 
remove the untracked file if <rev>:README is a tree.

I'm far from an expert in this area but I think we might want to 
understand why unpack_trees() sets state->force when it calls 
checkout_entry() before making any changes.

Best Wishes

Phillip

> Reported-by: Till Friebe <friebetill@gmail.com>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
>   entry.c          | 25 ++++++++++++++++++++++++-
>   t/t3903-stash.sh | 23 +++++++++++++++++++++++
>   2 files changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/entry.c b/entry.c
> index 43767f9043..76d8a0762d 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -15,6 +15,28 @@
>   #include "entry.h"
>   #include "parallel-checkout.h"
> 
> +static int rename_to_untracked_or_warn(const char *file)
> +{
> +	const size_t file_name_len = strlen(file);
> +	const static char *dot_untracked = ".untracked";
> +	const size_t dot_un_len = strlen(dot_untracked);
> +	struct strbuf sb;
> +	int ret;
> +
> +	strbuf_init(&sb, file_name_len + dot_un_len);
> +	strbuf_add(&sb, file, file_name_len);
> +	strbuf_add(&sb, dot_untracked, dot_un_len);
> +	ret = rename(file, sb.buf);
> +
> +	if (ret) {
> +		int saved_errno = errno;
> +		warning_errno(_("unable rename '%s' into '%s'"), file, sb.buf);
> +		errno = saved_errno;
> +	}
> +	strbuf_release(&sb);
> +	return ret;
> +}
> +
>   static void create_directories(const char *path, int path_len,
>   			       const struct checkout *state)
>   {
> @@ -48,7 +70,8 @@ static void create_directories(const char *path, int path_len,
>   		 */
>   		if (mkdir(buf, 0777)) {
>   			if (errno == EEXIST && state->force &&
> -			    !unlink_or_warn(buf) && !mkdir(buf, 0777))
> +			    !rename_to_untracked_or_warn(buf) &&
> +			    !mkdir(buf, 0777))
>   				continue;
>   			die_errno("cannot create directory at '%s'", buf);
>   		}
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 0b3dfeaea2..1a210f8a5a 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -1512,4 +1512,27 @@ test_expect_success 'restore untracked files even when we hit conflicts' '
>   	)
>   '
> 
> +test_expect_success 'stash mkdir README needed - README.untracked created' '
> +	git init mkdir_needed_file_untracked &&
> +	(
> +		cd mkdir_needed_file_untracked &&
> +		mkdir README &&
> +		touch README/README &&
> +		git add . &&
> +		git commit -m "Add README/README" &&
> +		echo Version2 > README/README &&
> +		mv README/README README2 &&
> +		rmdir README &&
> +		mv README2 README &&
> +		git stash &&
> +		test_path_is_file README.untracked &&
> +		echo Version2 >expect &&
> +		test_cmp expect README.untracked &&
> +		rm expect &&
> +		git stash pop &&
> +		test_path_is_file README.untracked &&
> +		echo Version2 >expect &&
> +		test_cmp expect README.untracked
> +	)
> +'
>   test_done
> --
> 2.41.0.394.ge43f4fd0bd
>