| Message ID | 20190329103919.15642-24-pclouds@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add new command 'switch' | expand |
On Fri, Mar 29, 2019 at 3:42 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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). 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. I think these safety checks are pretty important for checkout too... > > [1] CACsJy8Axa5WsLSjiscjnxVK6jQHkfs-gH959=YtUvQkWriAk5w@mail.gmail.com > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > 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\".")); > + 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; I think this should be 0 too; this check is good for both checkout and switch. And if people really do want to use it during in-progress operations because it is sometimes safe enough, then both operations deserve some kind of override flag that checks for the appropriate safety conditions (as we're discussing in the other thread) and then allows or rejects it. However, I'm totally fine with proposing another patch after your series lands to do all of this; this patch is fine as-is for now. > 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); > -- > 2.21.0.479.g47ac719cd3
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=YtUvQkWriAk5w@mail.gmail.com > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > 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); >
On Thu, Apr 25, 2019 at 5:33 PM Phillip Wood <phillip.wood123@gmail.com> wrote: > > 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). I probably will revisit this topic much sooner than I thought. I did a bisect today and found out "git switch" would not let me choose some "random" commit to test, which I suspected more likely where the problem was, or at least helped reduce the bisect steps. I had to go back to "git checkout" and was not so happy. This probably falls under the "safe to switch" (and not even back) category, as long as switching does not destroy any data, since bisect is basically jumping between commits with a clean worktree/index until you find the right one. > > +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. Eck! Let me check if --abort is the same thing there or we need to add --quit...
On 29/04/2019 10:16, Duy Nguyen wrote: > On Thu, Apr 25, 2019 at 5:33 PM Phillip Wood <phillip.wood123@gmail.com> wrote: >> >> 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). > > I probably will revisit this topic much sooner than I thought. I did a > bisect today and found out "git switch" would not let me choose some > "random" commit to test, which I suspected more likely where the > problem was, or at least helped reduce the bisect steps. I had to go > back to "git checkout" and was not so happy. Oh that's a pain, but should be safe as you describe below. > This probably falls under the "safe to switch" (and not even back) > category, as long as switching does not destroy any data, since bisect > is basically jumping between commits with a clean worktree/index until > you find the right one. >>> +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. > > Eck! Let me check if --abort is the same thing there or we need to add --quit... I think the --quit options generally leave the index and worktree alone, they just remove the state files whereas abort resets the index and worktree (with reset --mixed for merge and cherry-pick/revert, I think rebase does reset --hard) and also rewinds HEAD for rebases and sequences of cherry-picks and revert but that wont apply to merges as they are one-shot operations. Best Wishes Phillip
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\".")); + 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);
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). 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=YtUvQkWriAk5w@mail.gmail.com Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- builtin/checkout.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)