Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stash: do not return before restoring untracked files #1180

Closed
wants to merge 1 commit into from

Conversation

newren
Copy link
Contributor

@newren newren commented Jan 4, 2022

This fixes a regression in v2.34.1 relative to v2.33.0.

In commit bee8691 ("stash: restore untracked files AFTER restoring
tracked files", 2021-09-10), we correctly identified that we should
restore changes to tracked files before attempting to restore untracked
files, and accordingly moved the code for restoring untracked files a
few lines down in do_apply_stash().  Unfortunately, the intervening
lines had some early return statements meaning that we suddenly stopped
restoring untracked files in some cases.

Even before the previous commit, there was another possible issue with
the current code -- a post-stash-apply 'git status' that was intended
to be run after restoring the stash was skipped when we hit a conflict
(or other error condition), which seems slightly inconsistent.

Fix both issues by saving the return status, and letting other
functionality run before returning.

Reported-by: AJ Henderson
Test-case-by: Randall S. Becker <randall.becker@nexbridge.ca>
Signed-off-by: Elijah Newren <newren@gmail.com>
@newren
Copy link
Contributor Author

newren commented Jan 4, 2022

/submit

@gitgitgadget-git
Copy link

Submitted as pull.1180.git.git.1641337498996.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-git-1180/newren/fix-stash-restore-untracked-v1

To fetch this version to local tag pr-git-1180/newren/fix-stash-restore-untracked-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-git-1180/newren/fix-stash-restore-untracked-v1

@gitgitgadget-git
Copy link

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/builtin/stash.c b/builtin/stash.c
> index 18c812bbe03..397210168da 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -561,18 +561,19 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,
>  		if (index)
>  			fprintf_ln(stderr, _("Index was not unstashed."));
>  
> -		return ret;
> +		goto restore_untracked;
>  	}
>  
>  	if (has_index) {
>  		if (reset_tree(&index_tree, 0, 0))
> -			return -1;
> +			ret = -1;
>  	} else {
>  		unstage_changes_unless_new(&c_tree);
>  	}
>  
> +restore_untracked:
>  	if (info->has_u && restore_untracked(&info->u_tree))
> -		return error(_("could not restore untracked files from stash"));
> +		ret = error(_("could not restore untracked files from stash"));

In other words, instead of doing an early return, we try to
resurrect untracked ones and always show the "git status" before
returning.

I think that takes us in the right direction.  I looked at other
early returns in this function, and while there may be a room for
improvements when the --index stash does not apply well, I think it
can be left outside the topic of this patch.

Will queue.

Thanks.

>  	if (!quiet) {
>  		struct child_process cp = CHILD_PROCESS_INIT;
> @@ -592,7 +593,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,
>  		run_command(&cp);
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int apply_stash(int argc, const char **argv, const char *prefix)
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 2c66cfbc3b7..2a7d8e511db 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -1376,4 +1376,28 @@ test_expect_success 'git stash can pop directory -> file saved changes' '
>  	)
>  '
>  
> +test_expect_success 'restore untracked files even when we hit conflicts' '
> +	git init restore_untracked_after_conflict &&
> +	(
> +		cd restore_untracked_after_conflict &&
> +
> +		echo hi >a &&
> +		echo there >b &&
> +		git add . &&
> +		git commit -m first &&
> +		echo hello >a &&
> +		echo something >c &&
> +
> +		git stash push --include-untracked &&
> +
> +		echo conflict >a &&
> +		git add a &&
> +		git commit -m second &&
> +
> +		test_must_fail git stash pop &&
> +
> +		test_path_is_file c
> +	)
> +'
> +
>  test_done
>
> base-commit: 2ae0a9cb8298185a94e5998086f380a355dd8907

@gitgitgadget-git
Copy link

This branch is now known as en/stash-df-fix.

@gitgitgadget-git
Copy link

This patch series was integrated into seen via 072f1f4.

@gitgitgadget-git gitgitgadget-git bot added the seen label Jan 5, 2022
@gitgitgadget-git
Copy link

This patch series was integrated into seen via df1d068.

@gitgitgadget-git
Copy link

This patch series was integrated into seen via 6f937bd.

@gitgitgadget-git
Copy link

There was a status update in the "New Topics" section about the branch en/stash-df-fix on the Git mailing list:

"git stash apply" forgot to attempt restoring untracked files when
it failed to restore changes to tracked ones.

Will merge to 'next'.
source: <pull.1180.git.git.1641337498996.gitgitgadget@gmail.com>

@gitgitgadget-git
Copy link

This patch series was integrated into seen via b1d5a92.

@gitgitgadget-git
Copy link

This patch series was integrated into next via 22d0c6c.

@gitgitgadget-git gitgitgadget-git bot added the next label Jan 7, 2022
@gitgitgadget-git
Copy link

This patch series was integrated into seen via 6e22345.

@gitgitgadget-git
Copy link

This patch series was integrated into next via 6e22345.

@gitgitgadget-git
Copy link

This patch series was integrated into master via 6e22345.

@gitgitgadget-git
Copy link

Closed via 6e22345.

@newren newren deleted the fix-stash-restore-untracked branch January 13, 2022 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant