Subject: Re: [PATCH v6 23/27] switch: reject if some operation is in progress

From: Phillip Wood <[email-masked]>
Date: Thu, 25 Apr 2019 11:33:14 +0100

On 29/03/2019 10:39, Nguyễn Thái Ngọc Duy wrote:
> Unless you know what you're doing, switching to another branch to do
> something then switching back could be confusing. Worse, you may even
> forget that you're in the middle of something. By the time you realize,
> you may have done a ton of work and it gets harder to go back.
> 
> A new option --ignore-in-progress was considered but dropped because it
> was not exactly clear what should happen. Sometimes you can switch away
> and get back safely and resume the operation. Sometimes not. And the
> git-checkout behavior is automatically clear merge/revert/cherry-pick,
> which makes it a bit even more confusing [1].
> 
> We may revisit and add this option in the future. But for now play it
> safe and not allow it (you can't even skip this check with --force).

I think this is a good compromise, lets see how it goes (I think I
broadly agree with Elijah's suggestion to allow the switch if we can
safely switch back again if we want to add --ignore-in-progress in the
future).

> The
> user is suggested to cancel the operation by themselves (and hopefully
> they do consider the consequences, not blindly type the command), or to
> create a separate worktree instead of switching. The third option is
> the good old "git checkout", but it's not mentioned.
> 
> [1] CACsJy8Axa5WsLSjiscjnxVK6jQHkfs-gH959=[email-masked]
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <[email-masked]>
> ---
>  builtin/checkout.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index f7967cdb7c..5f100c1552 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -24,6 +24,7 @@
>  #include "tree.h"
>  #include "tree-walk.h"
>  #include "unpack-trees.h"
> +#include "wt-status.h"
>  #include "xdiff-interface.h"
>  
>  static const char * const checkout_usage[] = {
> @@ -56,6 +57,7 @@ struct checkout_opts {
>  	int accept_pathspec;
>  	int switch_branch_doing_nothing_is_ok;
>  	int only_merge_on_switching_branches;
> +	int can_switch_when_in_progress;
>  
>  	const char *new_branch;
>  	const char *new_branch_force;
> @@ -1202,6 +1204,39 @@ static void die_expecting_a_branch(const struct branch_info *branch_info)
>  	die(_("a branch is expected, got '%s'"), branch_info->name);
>  }
>  
> +static void die_if_some_operation_in_progress(void)
> +{
> +	struct wt_status_state state;
> +
> +	memset(&state, 0, sizeof(state));
> +	wt_status_get_state(the_repository, &state, 0);
> +
> +	if (state.merge_in_progress)
> +		die(_("cannot switch branch while merging\n"
> +		      "Consider \"git merge --quit\" "
> +		      "or \"git worktree add\"."));

I'm not sure merge --quit exists, 'git grep \"quit origin/pu' shows
matches for builtin/{am.c,rebase.c,revert.c}. The --quit option for the
sequencer command does not touch the index or working tree (that's the
difference between --quit and --abort) so the switch can still fail due
changes in the index and worktree that would be overwritten by the switch.

Best Wishes

Phillip

> +	if (state.am_in_progress)
> +		die(_("cannot switch branch in the middle of an am session\n"
> +		      "Consider \"git am --quit\" "
> +		      "or \"git worktree add\"."));
> +	if (state.rebase_interactive_in_progress || state.rebase_in_progress)
> +		die(_("cannot switch branch while rebasing\n"
> +		      "Consider \"git rebase --quit\" "
> +		      "or \"git worktree add\"."));
> +	if (state.cherry_pick_in_progress)
> +		die(_("cannot switch branch while cherry-picking\n"
> +		      "Consider \"git cherry-pick --quit\" "
> +		      "or \"git worktree add\"."));
> +	if (state.revert_in_progress)
> +		die(_("cannot switch branch while reverting\n"
> +		      "Consider \"git revert --quit\" "
> +		      "or \"git worktree add\"."));
> +	if (state.bisect_in_progress)
> +		die(_("cannot switch branch while bisecting\n"
> +		      "Consider \"git bisect reset HEAD\" "
> +		      "or \"git worktree add\"."));
> +}
> +
>  static int checkout_branch(struct checkout_opts *opts,
>  			   struct branch_info *new_branch_info)
>  {
> @@ -1257,6 +1292,9 @@ static int checkout_branch(struct checkout_opts *opts,
>  	    !new_branch_info->path)
>  		die_expecting_a_branch(new_branch_info);
>  
> +	if (!opts->can_switch_when_in_progress)
> +		die_if_some_operation_in_progress();
> +
>  	if (new_branch_info->path && !opts->force_detach && !opts->new_branch &&
>  	    !opts->ignore_other_worktrees) {
>  		int flag;
> @@ -1514,6 +1552,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  	opts.only_merge_on_switching_branches = 0;
>  	opts.accept_pathspec = 1;
>  	opts.implicit_detach = 1;
> +	opts.can_switch_when_in_progress = 1;
>  
>  	options = parse_options_dup(checkout_options);
>  	options = add_common_options(&opts, options);
> @@ -1549,6 +1588,7 @@ int cmd_switch(int argc, const char **argv, const char *prefix)
>  	opts.switch_branch_doing_nothing_is_ok = 0;
>  	opts.only_merge_on_switching_branches = 1;
>  	opts.implicit_detach = 0;
> +	opts.can_switch_when_in_progress = 0;
>  
>  	options = parse_options_dup(switch_options);
>  	options = add_common_options(&opts, options);
>