From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 19 Mar 2019 11:24:53 GMT
Subject: Re: [PATCH] checkout.txt: note about losing staged changes with --merge
Message-ID: <f9e3bd68-935c-3bca-6d06-66f51ba15451@gmail.com>
In-Reply-To: <20190319093910.20229-1-pclouds@gmail.com>

Hi Duy

On 19/03/2019 09:39, Nguyễn Thái Ngọc Duy wrote:
> If you have staged changes in path A and perform 'checkout
> --merge' (which could result in conflicts in a totally unrelated path
> B), changes in A will be gone. Which is unexpected. We are supposed
> to keep all changes, or kick and scream otherwise.
> 
> This is the result of how --merge is implemented, from the very first
> day in 1be0659efc (checkout: merge local modifications while switching
> branches., 2006-01-12):
> 
> 1. a merge is done, unmerged entries are collected
> 2. a hard switch to a new branch is done, then unmerged entries added
>     back
> 
> There is no trivial fix for this. Going with 3-way merge one file at a
> time loses rename detection. Going with 3-way merge by trees requires
> teaching the algorithm to pick up staged changes. And even if we detect
> staged changes with --merge and abort for safety, an option to continue
> --merge is very weird. Such an option would keep worktree changes, but
> drop staged changes.
> 
> Because the problem has been with us since the introduction of --merge
> and everybody has been pretty happy (except Phillip, who found this
> problem), I'll just take a note here to acknowledge it and wait for
> merge wizards to come in and work their magic. There may be a way
> forward [1].
> 
> [1] CABPp-BFoL_U=bzON4SEMaQSKU2TKwnOgNqjt5MUaOejTKGUJxw@mail.gmail.com
> 
> Reported-by: Phillip Wood <phillip.wood123@gmail.com>

I try to use phillip.wood@dunelm.org.uk for git stuff as it shouldn't 
change in the future.

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>   This is my "fix" for Phillip's second problem. I chose to reply here
>   because this is where an actual fix was discussed. The test script to
>   demonstate it is here
> 
>   https://public-inbox.org/git/7d3742d6-73e4-2750-6ecb-9edf761d96dd@gmail.com/
> 
>   Documentation/git-checkout.txt | 2 ++
>   builtin/checkout.c             | 9 +++++++++
>   2 files changed, 11 insertions(+)
> 
> diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
> index f179b43732..877e5f503a 100644
> --- a/Documentation/git-checkout.txt
> +++ b/Documentation/git-checkout.txt
> @@ -242,6 +242,8 @@ should result in deletion of the path).
>   +
>   When checking out paths from the index, this option lets you recreate
>   the conflicted merge in the specified paths.
> ++
> +When switching branches with `--merge`, staged changes may be lost.
>   
>   --conflict=<style>::
>   	The same as --merge option above, but changes the way the
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 0e6037b296..f95e7975f7 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -726,6 +726,8 @@ static int merge_working_tree(const struct checkout_opts *opts,
>   			struct tree *result;
>   			struct tree *work;
>   			struct merge_options o;
> +			struct strbuf sb = STRBUF_INIT;
> +
>   			if (!opts->merge)
>   				return 1;
>   
> @@ -736,6 +738,13 @@ static int merge_working_tree(const struct checkout_opts *opts,
>   			if (!old_branch_info->commit)
>   				return 1;
>   
> +			if (repo_index_has_changes(the_repository,
> +						   get_commit_tree(old_branch_info->commit),
> +						   &sb))
> +				warning(_("staged changes in the following files may be lost: %s"),
> +					sb.buf);
> +			strbuf_release(&sb);

Thanks for doing this, I think having some sort of warning is a good 
idea, I wonder if this could be quite noisy though. I guess it depends 
on how many staged changes people have that don't match the new index. 
If we diff against the new tree and only print names that are in both 
lists does that give a definitive list of what will be lost? If it does 
then if there are a lot of files affected then it will still be noisy 
(using columns may help) but at least it will not contain false 
positives. It is more work though, maybe we should just say "staged 
changes may be lost" and leave it at that.

Best Wishes

Phillip

> +
>   			/* Do more real merge */
>   
>   			/*
>