Re: [PATCH v4] checkout: -m (--merge) uses autostash when switching branches

[Junio C Hamano <gitster@pobox.com>]

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> When switching branches with "git checkout -m", local modifications
> can block the switch.  Teach the -m flow to create a temporary stash
> before switching and reapply it after.  On success, only "Applied
> autostash." is shown.  If reapplying causes conflicts, the stash is
> kept and the user is told they can resolve and run "git stash drop",
> or run "git reset --hard" and later "git stash pop" to recover their
> changes.
>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
> ---
>     checkout: 'autostash' for branch switching
>     
>     cc: Phillip Wood phillip.wood123@gmail.com

I very much appreciate this, but somehow this "cc:" seems to have
been ignored.

>  Documentation/git-checkout.adoc |  56 +++++-----
>  Documentation/git-switch.adoc   |  26 ++---
>  builtin/checkout.c              | 179 +++++++++++++++++---------------
>  builtin/stash.c                 |  30 ++++--
>  sequencer.c                     |  67 +++++++++---
>  sequencer.h                     |   4 +
>  t/t3420-rebase-autostash.sh     |  24 +++--
>  t/t7201-co.sh                   | 160 ++++++++++++++++++++++++++++
>  t/t7600-merge.sh                |   4 +-
>  xdiff-interface.c               |  12 +++
>  xdiff-interface.h               |   1 +
>  11 files changed, 412 insertions(+), 151 deletions(-)
>
> diff --git a/Documentation/git-checkout.adoc b/Documentation/git-checkout.adoc
> index 43ccf47cf6..9d5f5c51ae 100644
> --- a/Documentation/git-checkout.adoc
> +++ b/Documentation/git-checkout.adoc
> @@ -251,20 +251,17 @@ working tree, by copying them from elsewhere, extracting a tarball, etc.
>  	are different between the current branch and the branch to
>  	which you are switching, the command refuses to switch
>  	branches in order to preserve your modifications in context.
> -	However, with this option, a three-way merge between the current
> -	branch, your working tree contents, and the new branch
> -	is done, and you will be on the new branch.
> -+
> -When a merge conflict happens, the index entries for conflicting
> -paths are left unmerged, and you need to resolve the conflicts
> -and mark the resolved paths with `git add` (or `git rm` if the merge
> -should result in deletion of the path).
> +	With this option, the conflicting local changes are
> +	automatically stashed before the switch and reapplied
> +	afterwards.  If the local changes do not overlap with the
> +	differences between branches, the switch proceeds without
> +	stashing.  If reapplying the stash results in conflicts, the
> +	entry is saved to the stash list so you can use `git stash
> +	pop` to recover and `git stash drop` when done.

The new conflict message in sequencer.c correctly points out that,
if the user wants to use `git stash pop`, they would first need to
clear the working tree (e.g. `git reset --hard`) because the
conflicted changes are already there.

The documentation here is a bit too simplified and may give the
impression that `git stash pop` is the immediate next step when
conflicts are present in the worktree, which is probably something
we want to avoid.

>  ------------
>  $ git checkout -m mytopic
> -Auto-merging frotz
> +Applied autostash.
>  ------------

I must be utterly confused, but the documentation _before_ this
patch does not seem to match the reality.  In the following
illustration, I am switching to another branch that has contents in
transport.c that is different from the version in HEAD:

    $ git checkout master
    $ echo >>transport.c
    $ git checkout jk/transport-color-leakfix
    error: Your local changes to the following files would be overwritten...
        transport.c
    Please commit your changes or stash ...
    Aborting
    $ git checkout -m jk/transport-color-leakfix
    M	transport.c
    Switched to branch 'jk/transport-color-leakfix'

Hmmmm.

The new behavior is silent about individual files, and instead
reports "Applied autostash."; even though it is consistent with how
other commands may behave when given the "--autostash" option, I am
not sure if it is even necessary.

Even in the documented version, the "Auto-merging" message is
presumably given only to the paths that needed real content-level
merges, and the paths that went through the trivial tree-level
merges (i.e., paths that are different from HEAD and mytopic but
without local changes) are not reported.  The behaviour I am seeing
does not say anything to either trivially merged or really merged
paths while branch switching is happening, and shows the local
changes at the end of merge_working_tree() by calling
show_local_changes(), which runs "git diff --name-status"
in-process.

In either case, the stashing is a mere implementation detail of
carrying the local changes with you when you move to a different
branch, and moving with your local changes is what "Switching"
branch is about in this system, so I am not sure if we need to say
anything more than the current "Switched to branch 'mytopic'" in the
successful case.

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 1d1667fa4c..1968e46ae9 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -17,7 +17,6 @@
>  #include "merge-ll.h"
>  #include "lockfile.h"
>  #include "mem-pool.h"
> -#include "merge-ort-wrappers.h"
>  #include "object-file.h"
>  #include "object-name.h"
>  #include "odb.h"
> @@ -30,6 +29,7 @@
>  #include "repo-settings.h"
>  #include "resolve-undo.h"
>  #include "revision.h"
> +#include "sequencer.h"
>  #include "setup.h"
>  #include "submodule.h"
>  #include "symlinks.h"
> @@ -845,83 +845,8 @@ static int merge_working_tree(const struct checkout_opts *opts,
>  
>  		ret = unpack_trees(2, trees, &topts);
>  		clear_unpack_trees_porcelain(&topts);
> -		if (ret == -1) {
> -...
> -		}
> +		if (ret == -1)
> +			return 1;
>  	}
>  
>  	if (!cache_tree_fully_valid(the_repository->index->cache_tree))
> @@ -930,9 +855,6 @@ static int merge_working_tree(const struct checkout_opts *opts,
>  	if (write_locked_index(the_repository->index, &lock_file, COMMIT_LOCK))
>  		die(_("unable to write new index file"));
>  
> -	if (!opts->discard_changes && !opts->quiet && new_branch_info->commit)
> -		show_local_changes(&new_branch_info->commit->object, &opts->diff_options);

Ah, this is what you lost.  I do not see a reason why we would want
to lose it.

> +static int checkout_would_clobber_changes(struct branch_info *old_branch_info,
> +					  struct branch_info *new_branch_info)
> +{
> +...
> +	init_tree_desc(&trees[0], &old_tree->object.oid,
> +		       old_tree->buffer, old_tree->size);
> +	init_tree_desc(&trees[1], &new_tree->object.oid,
> +		       new_tree->buffer, new_tree->size);
> +
> +	ret = unpack_trees(2, trees, &topts);
> +	discard_index(&tmp_index);

In my original outline, I did suggest 

 - check if the local changes overlap with paths that differ between
   HEAD and the new branch, and if not, we need no stashing;
   otherwise stash.

 - then switch branches with two-way merge

 - then unstash.

But my untold assumption was that the initial check was done by
running a moral equivalent of "git diff --name-only HEAD <branch>"
to obtain the list of paths that differ between branches, and
a "git diff HEAD" to obtain the list of paths with local changes.

If you are using the real branch switching machinery in the
"dry-run" mode, then it is a waste to discard when we find out that
there is no overlap.  So if I knew you would use the unpack-trees in
the dry-run mode, instead of two diffs, the outline would have been
more like

 - Try unpack-trees in dry-run mode to see if there would get any
   failure from overlap.

 - If unpack-trees would succeed, we already have the right contents
   in the index after the above dry-run.  We did not touch the
   working tree so far, but if we kept track of which paths we would
   have written out to the working tree during our dry-run, we can
   write them out of the index at this point and we are done (we of
   course need to update HEAD to point at the new branch, give the
   "Switched to branch 'foo'" message, etc., but the real work of
   switching branches is done).

 - If not, discard the dry-run index, stash, switch branches the
   usual way, and unstash.

which might be more performant in a large tree.

Having said all that, what we see above, while running the same
unpack-trees once in dry-run mode and another for real, may be
performant enough and complicating the dry-run code might not be
worth it.

> @@ -1165,6 +1136,8 @@ static int switch_branches(const struct checkout_opts *opts,
>  	struct object_id rev;
>  	int flag, writeout_error = 0;
>  	int do_merge = 1;
> +	struct strbuf old_commit_shortname = STRBUF_INIT;
> +	const char *stash_label_ancestor = NULL;
>  
>  	trace2_cmd_mode("branch");
>  
> @@ -1202,10 +1175,34 @@ static int switch_branches(const struct checkout_opts *opts,
>  			do_merge = 0;
>  	}
>  
> +	if (old_branch_info.name)
> +		stash_label_ancestor = old_branch_info.name;
> +	else if (old_branch_info.commit) {
> +		strbuf_add_unique_abbrev(&old_commit_shortname,
> +					 &old_branch_info.commit->object.oid,
> +					 DEFAULT_ABBREV);
> +		stash_label_ancestor = old_commit_shortname.buf;
> +	}
> +
> +	if (opts->merge) {
> +		if (repo_read_index(the_repository) < 0)
> +			die(_("index file corrupt"));
> +		if (checkout_would_clobber_changes(&old_branch_info,
> +						   new_branch_info))
> +			create_autostash_ref_silent(the_repository,
> +						   "CHECKOUT_AUTOSTASH");
> +	}
> +
>  	if (do_merge) {
>  		ret = merge_working_tree(opts, &old_branch_info, new_branch_info, &writeout_error);
>  		if (ret) {
> +			apply_autostash_ref_with_labels(the_repository,
> +						       "CHECKOUT_AUTOSTASH",
> +						       new_branch_info->name,
> +						       "local",
> +						       stash_label_ancestor);
>  			branch_info_release(&old_branch_info);
> +			strbuf_release(&old_commit_shortname);
>  			return ret;
>  		}
>  	}

Nice changes to preserve the conflict marker labels.

> diff --git a/builtin/stash.c b/builtin/stash.c
> index e79d612e57..1016d88e52 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c

This can and should become a separate patch that is a preliminary
preparation step, [1/2], with its own documentation and test, only
about teaching "git stash" command a set of new options to use
custom conflict labels, I think.  And on top of it, patch [2/2] that
updates "checkout -m" to use the improved "git stash" with custom
labels support is written.  It would give us a clean logical
progression of the series.

> diff --git a/sequencer.c b/sequencer.c
> index aafd0bc959..53e04d8a94 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4632,7 +4632,8 @@ static enum todo_command peek_command(struct todo_list *todo_list, int offset)
>  
>  static void create_autostash_internal(struct repository *r,
>  				      const char *path,
> -				      const char *refname)
> +				      const char *refname,
> +				      int silent)
>  {
>  	struct strbuf buf = STRBUF_INIT;
>  	struct lock_file lock_file = LOCK_INIT;
> @@ -4677,7 +4678,8 @@ static void create_autostash_internal(struct repository *r,
>  					&oid, null_oid(the_hash_algo), 0, UPDATE_REFS_DIE_ON_ERR);
>  		}
>  
> -		printf(_("Created autostash: %s\n"), buf.buf);
> +		if (!silent)
> +			fprintf(stderr, _("Created autostash: %s\n"), buf.buf);
>  		if (reset_head(r, &ropts) < 0)
>  			die(_("could not reset --hard"));
>  		discard_index(r->index);
> @@ -4689,15 +4691,22 @@ static void create_autostash_internal(struct repository *r,
>  
>  void create_autostash(struct repository *r, const char *path)
>  {
> -	create_autostash_internal(r, path, NULL);
> +	create_autostash_internal(r, path, NULL, 0);
>  }
>  
>  void create_autostash_ref(struct repository *r, const char *refname)
>  {
> -	create_autostash_internal(r, NULL, refname);
> +	create_autostash_internal(r, NULL, refname, 0);
>  }
>  
> -static int apply_save_autostash_oid(const char *stash_oid, int attempt_apply)
> +void create_autostash_ref_silent(struct repository *r, const char *refname)
> +{
> +	create_autostash_internal(r, NULL, refname, 1);
> +}

Changes around here to optionally squelch message(s) is better
presented as yet another preliminary preparation step, I think.

> @@ -4752,7 +4775,8 @@ static int apply_save_autostash(const char *path, int attempt_apply)
>  	}
>  	strbuf_trim(&stash_oid);
>  
> -	ret = apply_save_autostash_oid(stash_oid.buf, attempt_apply);
> +	ret = apply_save_autostash_oid(stash_oid.buf, attempt_apply,
> +				      NULL, NULL, NULL);
>  
>  	unlink(path);
>  	strbuf_release(&stash_oid);
> @@ -4771,11 +4795,13 @@ int apply_autostash(const char *path)
>  
>  int apply_autostash_oid(const char *stash_oid)
>  {
> -	return apply_save_autostash_oid(stash_oid, 1);
> +	return apply_save_autostash_oid(stash_oid, 1, NULL, NULL, NULL);
>  }
>  
>  static int apply_save_autostash_ref(struct repository *r, const char *refname,
> -				    int attempt_apply)
> +				    int attempt_apply,
> +				    const char *label1, const char *label2,
> +				    const char *label_ancestor)
>  {
>  	struct object_id stash_oid;
>  	char stash_oid_hex[GIT_MAX_HEXSZ + 1];
> @@ -4791,7 +4817,8 @@ static int apply_save_autostash_ref(struct repository *r, const char *refname,
>  		return error(_("autostash reference is a symref"));
>  
>  	oid_to_hex_r(stash_oid_hex, &stash_oid);
> -	ret = apply_save_autostash_oid(stash_oid_hex, attempt_apply);
> +	ret = apply_save_autostash_oid(stash_oid_hex, attempt_apply,
> +				       label1, label2, label_ancestor);
>  
>  	refs_delete_ref(get_main_ref_store(r), "", refname,
>  			&stash_oid, REF_NO_DEREF);
>
> @@ -4801,12 +4828,20 @@ static int apply_save_autostash_ref(struct repository *r, const char *refname,
>  
>  int save_autostash_ref(struct repository *r, const char *refname)
>  {
> -	return apply_save_autostash_ref(r, refname, 0);
> +	return apply_save_autostash_ref(r, refname, 0, NULL, NULL, NULL);
>  }
>  
>  int apply_autostash_ref(struct repository *r, const char *refname)
>  {
> -	return apply_save_autostash_ref(r, refname, 1);
> +	return apply_save_autostash_ref(r, refname, 1, NULL, NULL, NULL);
> +}
> +
> +int apply_autostash_ref_with_labels(struct repository *r, const char *refname,
> +				    const char *label1, const char *label2,
> +				    const char *label_ancestor)
> +{
> +	return apply_save_autostash_ref(r, refname, 1,
> +					label1, label2, label_ancestor);
>  }
>  
>  static int checkout_onto(struct repository *r, struct replay_opts *opts,

So are these, to teach the machinery to take optional conflict
marker labels.

Thanks.