[PATCH] commit: do not switch branch during a rebase unless -f is given

[PATCH] commit: do not switch branch during a rebase unless -f is given

[Nguyễn Thái Ngọc Duy]

It does not make much sense to switch branch when you are in a middle
of a rebase. Sometimes you might want to switch away for a moment then
back with "git checkout - ". But I find myself so many times switching
away then forget that I was rebasing something.

Avoid doing that by default. Users can use -f if they really want to.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 I know there are other commands like rebase ("git am" comes to mind)
 but I don't use those. Feel free to put some more on top if somebody
 finds it a good thing to do.

 builtin/checkout.c         |    7 +++++++
 t/t2018-checkout-branch.sh |   22 ++++++++++++++++++++++
 2 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index cc622c0..7d8ba04 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -571,6 +571,13 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 	struct branch_info old;
 	unsigned char rev[20];
 	int flag;
+	struct stat st;
+
+	if (!opts->force &&
+	    ((!stat(git_path("rebase-merge"), &st) && S_ISDIR(st.st_mode)) ||
+	     (!stat(git_path("rebase-apply"), &st) && S_ISDIR(st.st_mode))))
+		die("You should not switch branch during a rebase. Use '-f' if you really want to.");
+
 	memset(&old, 0, sizeof(old));
 	old.path = resolve_ref("HEAD", rev, 0, &flag);
 	old.commit = lookup_commit_reference_gently(rev, 1);
diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index fa69016..95a1da8 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -169,4 +169,26 @@ test_expect_success 'checkout -f -B to an existing branch with mergeable changes
 	test_must_fail test_dirty_mergeable
 '
 
+test_expect_success 'checkout fails during rebase' '
+	git reset --hard &&
+	git checkout branch1 &&
+	mkdir .git/rebase-merge &&
+	test_must_fail git checkout branch2 &&
+	git checkout -f branch2
+'
+
+test_expect_success 'checkout fails during rebase (2)' '
+	rmdir .git/rebase-merge &&
+	git reset --hard &&
+	git checkout branch1 &&
+	mkdir .git/rebase-apply &&
+	test_must_fail git checkout branch2 &&
+	git checkout -f branch2
+'
+
+# this is to be incorporated to the next test
+test_expect_success 'cleanup' '
+	rmdir .git/rebase-apply
+'
+
 test_done
-- 
1.7.1.rc1.70.g788ca

[PATCH] checkout: do not switch branch during a rebase unless -f is given

[Nguyễn Thái Ngọc Duy]

It does not make much sense to switch branch when you are in a middle
of a rebase. Sometimes you might want to switch away for a moment then
back with "git checkout - ". But I find myself so many times switching
away then forget that I was rebasing something.

Avoid doing that by default. Users can use -f if they really want to.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Sorry, the previous patch had wrong subject line

 I know there are other commands like rebase ("git am" comes to mind)
 but I don't use those. Feel free to put some more on top if somebody
 finds it a good thing to do.

 builtin/checkout.c         |    7 +++++++
 t/t2018-checkout-branch.sh |   22 ++++++++++++++++++++++
 2 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index cc622c0..7d8ba04 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -571,6 +571,13 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 	struct branch_info old;
 	unsigned char rev[20];
 	int flag;
+	struct stat st;
+
+	if (!opts->force &&
+	    ((!stat(git_path("rebase-merge"), &st) && S_ISDIR(st.st_mode)) ||
+	     (!stat(git_path("rebase-apply"), &st) && S_ISDIR(st.st_mode))))
+		die("You should not switch branch during a rebase. Use '-f' if you really want to.");
+
 	memset(&old, 0, sizeof(old));
 	old.path = resolve_ref("HEAD", rev, 0, &flag);
 	old.commit = lookup_commit_reference_gently(rev, 1);
diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index fa69016..95a1da8 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -169,4 +169,26 @@ test_expect_success 'checkout -f -B to an existing branch with mergeable changes
 	test_must_fail test_dirty_mergeable
 '
 
+test_expect_success 'checkout fails during rebase' '
+	git reset --hard &&
+	git checkout branch1 &&
+	mkdir .git/rebase-merge &&
+	test_must_fail git checkout branch2 &&
+	git checkout -f branch2
+'
+
+test_expect_success 'checkout fails during rebase (2)' '
+	rmdir .git/rebase-merge &&
+	git reset --hard &&
+	git checkout branch1 &&
+	mkdir .git/rebase-apply &&
+	test_must_fail git checkout branch2 &&
+	git checkout -f branch2
+'
+
+# this is to be incorporated to the next test
+test_expect_success 'cleanup' '
+	rmdir .git/rebase-apply
+'
+
 test_done
-- 
1.7.1.rc1.70.g788ca

Re: [PATCH] checkout: do not switch branch during a rebase unless -f is given

[Jonathan Nieder]

(+cc: Frédéric, Dscho, Christian)

Nguyễn Thái Ngọc Duy wrote:

> It does not make much sense to switch branch when you are in a middle
> of a rebase. Sometimes you might want to switch away for a moment then
> back with "git checkout - ". But I find myself so many times switching
> away then forget that I was rebasing something.
>
> Avoid doing that by default. Users can use -f if they really want to.

Nice! But I worry is that scripts that use "git checkout" porcelain
during a rebase would be broken.

Even rebase --interactive uses checkout from time to time:

 - for preserving merges
 - to move to the correct branch in response to "git rebase -i
<upstream> <branch>"
 - to move to the target in "git rebase -i --onto <new base> <upstream>"

Unfortunately I do not have any good advice. Would it make sense to

 - first, change these call sites in git to use checkout -f
 - teach checkout to warn (without erroring out) to give people time
to change their scripts
 - warn loudly about the upcoming change in the release notes
 - later, change checkout to error out when -f is not supplied

or am I being too paranoid?

>  I know there are other commands like rebase ("git am" comes to mind)
>  but I don't use those. Feel free to put some more on top if somebody
>  finds it a good thing to do.

Another interesting one is "git bisect".

Re: [PATCH] checkout: do not switch branch during a rebase unless -f is given

[Nguyen Thai Ngoc Duy]

2010/9/28 Jonathan Nieder <jrnieder@gmail.com>:
> (+cc: Frédéric, Dscho, Christian)
>
> Nguyễn Thái Ngọc Duy wrote:
>
>> It does not make much sense to switch branch when you are in a middle
>> of a rebase. Sometimes you might want to switch away for a moment then
>> back with "git checkout - ". But I find myself so many times switching
>> away then forget that I was rebasing something.
>>
>> Avoid doing that by default. Users can use -f if they really want to.
>
> Nice! But I worry is that scripts that use "git checkout" porcelain
> during a rebase would be broken.

Right. I used git-rebase from my installation, the new "git checkout"
from worktree and missed that as a result.

> Even rebase --interactive uses checkout from time to time:
>
>  - for preserving merges
>  - to move to the correct branch in response to "git rebase -i
> <upstream> <branch>"
>  - to move to the target in "git rebase -i --onto <new base> <upstream>"
>
> Unfortunately I do not have any good advice. Would it make sense to
>
>  - first, change these call sites in git to use checkout -f
>  - teach checkout to warn (without erroring out) to give people time
> to change their scripts
>  - warn loudly about the upcoming change in the release notes
>  - later, change checkout to error out when -f is not supplied
>
> or am I being too paranoid?

No. But I don't like the idea of making scripts use "checkout -f". My
intention was to stop users from doing that, not scripts. Putting "-f"
everywhere might have more negative side effects.

Maybe adding "--porcelain" to checkout first, updating scripts to use
it, then only check for rebase/bisect/am when --porcelain is missing.
-- 
Duy

Re: [PATCH] checkout: do not switch branch during a rebase unless -f is given

[Nguyen Thai Ngoc Duy]

2010/9/28 Nguyen Thai Ngoc Duy <pclouds@gmail.com>:
>> Even rebase --interactive uses checkout from time to time:
>>
>>  - for preserving merges
>>  - to move to the correct branch in response to "git rebase -i
>> <upstream> <branch>"
>>  - to move to the target in "git rebase -i --onto <new base> <upstream>"
>>
>> Unfortunately I do not have any good advice. Would it make sense to
>>
>>  - first, change these call sites in git to use checkout -f
>>  - teach checkout to warn (without erroring out) to give people time
>> to change their scripts
>>  - warn loudly about the upcoming change in the release notes
>>  - later, change checkout to error out when -f is not supplied
>>
>> or am I being too paranoid?
>
> No. But I don't like the idea of making scripts use "checkout -f". My
> intention was to stop users from doing that, not scripts. Putting "-f"
> everywhere might have more negative side effects.
>
> Maybe adding "--porcelain" to checkout first, updating scripts to use
> it, then only check for rebase/bisect/am when --porcelain is missing.

Another approach is to let checkout work as usual, but refuse update refs:

 - after rebase starts, HEAD can only be updated either by rebase, or
any commands that keep HEAD a headless ref.
 - the branch being rebased is locked. No commands but rebase can update it.

I think the second point is good for all interactive commands like
rebase. Create a .lock file with a signature inside (e.g. command
name). If update_ref() callers do not give correct signature, refuse
to update.
-- 
Duy

Re: [PATCH] checkout: do not switch branch during a rebase unless -f is given

[Jonathan Nieder]

Nguyen Thai Ngoc Duy wrote:

> Another approach is to let checkout work as usual, but refuse update refs:
> 
>  - after rebase starts, HEAD can only be updated either by rebase, or
> any commands that keep HEAD a headless ref.
>  - the branch being rebased is locked. No commands but rebase can update it.
> 
> I think the second point is good for all interactive commands like
> rebase. Create a .lock file with a signature inside (e.g. command
> name). If update_ref() callers do not give correct signature, refuse
> to update.

I like it.  Would it be possible to make sure the (widespread?) practice
of using

	rm -fr .git/rebase-merge

to terminate a rebase without going back to the original branch
still works?  I think it should be.

Re: [PATCH] checkout: do not switch branch during a rebase unless -f is given

[Sverre Rabbelier]

Heya,

2010/11/9 Jonathan Nieder <jrnieder@gmail.com>:
> I like it.  Would it be possible to make sure the (widespread?) practice
> of using
>
>        rm -fr .git/rebase-merge

Awr, and here I was, all hopeful reading your message up to here.

> to terminate a rebase without going back to the original branch
> still works?  I think it should be.

And then here you smash my hopes that you were suggesting we stop
asking the user to mess around in the .git directory :(. After all, if
we let that rm be handled by some 'git rebase' mode, it could also
unlock any branches it has locked.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] checkout: do not switch branch during a rebase unless -f is given

[Jonathan Nieder]

Sverre Rabbelier wrote:

> And then here you smash my hopes that you were suggesting we stop
> asking the user to mess around in the .git directory :(. After all, if
> we let that rm be handled by some 'git rebase' mode, it could also
> unlock any branches it has locked.

Well, I think that is worth doing, too.  It's just that it's hard to
change existing practices and much easier to change the tools to
keep supporting them.

I don't think the manual advertises .git/rebase-merge anywhere.  I
have witnessed people succumbing to the temptation to delete it when
another rebase fails.

Maybe "git rebase" should have a --override-rebase-in-progress option?
The following is simpler, sort of imitating the "git bisect reset"
command.  With it applied, if you do

	git rebase --abort HEAD

then the rebase will abort without changing the current HEAD (yes,
I know this interface is stupid; please feel free to pick it up
and do better).

Other practices worth abstracting away:

	- Reading .git/rebase-merge/patch and
	  .git/rebase-merge/message when a patch had conflicts
	- Checking for the existence of .git/rebase-merge in $PS1
	  prompt
	- Tweaking .git/rebase-merge/todo to reflect a change of plan

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 30e5c0e..3a587a1 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -227,6 +227,9 @@ leave out at most one of A and B, in which case it defaults to HEAD.
 --abort::
 	Restore the original branch and abort the rebase operation.
 
+--abort HEAD::
+	Abort the rebase operation, without changing the current branch.
+
 --skip::
 	Restart the rebasing process by skipping the current patch.
 
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index a27952d..2e1c074 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -772,6 +772,14 @@ first and then run 'git rebase --continue' again."
 		do_rest
 		;;
 	--abort)
+		if test "$2" = HEAD
+		then
+			shift
+			is_standalone "$@" || usage
+			get_saved_options
+			rm -rf "$DOTEST"
+			exit
+		fi
 		is_standalone "$@" || usage
 		get_saved_options
 		comment_for_reflog abort
diff --git a/git-rebase.sh b/git-rebase.sh
index e5df23b..0f88a00 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -273,6 +273,11 @@ do
 		test -d "$dotest" -o -d "$GIT_DIR"/rebase-apply ||
 			die "No rebase in progress?"
 
+		if test "$2" = HEAD
+		then
+			rm -r "$dotest"
+			exit
+		fi
 		git rerere clear
 		if test -d "$dotest"
 		then

Re: [PATCH] checkout: do not switch branch during a rebase unless -f is given

[Nguyen Thai Ngoc Duy]

2010/11/9 Jonathan Nieder <jrnieder@gmail.com>:
> Nguyen Thai Ngoc Duy wrote:
>
>> Another approach is to let checkout work as usual, but refuse update refs:
>>
>>  - after rebase starts, HEAD can only be updated either by rebase, or
>> any commands that keep HEAD a headless ref.
>>  - the branch being rebased is locked. No commands but rebase can update it.
>>
>> I think the second point is good for all interactive commands like
>> rebase. Create a .lock file with a signature inside (e.g. command
>> name). If update_ref() callers do not give correct signature, refuse
>> to update.
>
> I like it.  Would it be possible to make sure the (widespread?) practice
> of using
>
>        rm -fr .git/rebase-merge
>
> to terminate a rebase without going back to the original branch
> still works?  I think it should be.
>

I don't know. If we can place the branch lock inside
.git/rebase-merge, then yes removing that directory will also remove
the lock. But how does git find the locks if they are everywhere in
.git?
-- 
Duy

Re: [PATCH] checkout: do not switch branch during a rebase unless -f is given

[Jonathan Nieder]

Nguyen Thai Ngoc Duy wrote:

> I don't know. If we can place the branch lock inside
> .git/rebase-merge, then yes removing that directory will also remove
> the lock. But how does git find the locks if they are everywhere in
> .git?

Could the lock signature say something to the effect that "this lock
is only valid against commands that don't declare this signature, and
only so long as .git/rebase-merge/ref has such-and-such content"?

Re: [PATCH] commit: do not switch branch during a rebase unless -f is given

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> It does not make much sense to switch branch when you are in a middle
> of a rebase. Sometimes you might want to switch away for a moment then
> back with "git checkout - ". But I find myself so many times switching
> away then forget that I was rebasing something.
>
> Avoid doing that by default. Users can use -f if they really want to.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

I think you need to retitle the patch.

You should realize that overloading "-f" and conflating its totally
different meaning, which is to discard local changes, with this new usage
to say "override the 'you are in rebase' switch" closes the door to future
improvements we could make to rebase so that it does not to require a
totally clean working tree.  It would be Ok with today's rebase, as you
wouldn't have local changes when it runs, but we might regret this 9
months down the road.

>  I know there are other commands like rebase ("git am" comes to mind)
>  but I don't use those. Feel free to put some more on top if somebody
>  finds it a good thing to do.

If you envision something similar to be used during "git am", then you
really shouldn't be reusing "-f", as "git am" is designed to be usable in
a dirty working tree.

Re: [PATCH] commit: do not switch branch during a rebase unless -f is given

[Sverre Rabbelier]

Heya,

2010/9/28 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
> +               die("You should not switch branch during a rebase. Use '-f' if you really want to.");

On top of Junio's comments, I think it makes sense here to add "or use
'git rebase --abort' to abort the rebase that is in progress".

-- 
Cheers,

Sverre Rabbelier