threads / patch / 25264

patchcommit: do not switch branch during a rebase unless -f is given

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

## tl;dr

12 messages between Sep 28, 2010 and Nov 9, 2010. Diffs are folded; open one to read it.

replies: 11people: 4as markdown or json

Nguyễn Thái Ngọc Duy· Sep 28, 2010, 04:52 UTC · lore

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(-)
Show changes to 2 files +29 −0

builtin/checkout.c, t/t2018-checkout-branch.sh

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
Nguyễn Thái Ngọc Duy· Sep 28, 2010, 04:55 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 2 files +29 −0

builtin/checkout.c, t/t2018-checkout-branch.sh

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
Jonathan Nieder· Sep 28, 2010, 05:33 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

(+cc: Frédéric, Dscho, Christian)
Nguyễn Thái Ngọc Duy wrote:
Show 6 quoted lines
> 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".
Nguyen Thai Ngoc Duy· Sep 28, 2010, 05:47 UTC · re: Jonathan Nieder · lore

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

2010/9/28 Jonathan Nieder <jrnieder@gmail.com>:
Show 13 quoted lines
> (+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.

Show 16 quoted lines
> 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
Nguyen Thai Ngoc Duy· Nov 9, 2010, 13:40 UTC · re: Nguyen Thai Ngoc Duy · lore

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

2010/9/28 Nguyen Thai Ngoc Duy <pclouds@gmail.com>:
Show 23 quoted lines
>> 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
Jonathan Nieder· Nov 9, 2010, 14:06 UTC · re: Nguyen Thai Ngoc Duy · lore

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

Nguyen Thai Ngoc Duy wrote:
Show 10 quoted lines
> 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.

Sverre Rabbelier· Nov 9, 2010, 14:11 UTC · re: Jonathan Nieder · lore

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

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
Jonathan Nieder· Nov 9, 2010, 14:36 UTC · re: Sverre Rabbelier · lore

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

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>
---
Show changes to 3 files +16 −0

Documentation/git-rebase.txt, git-rebase--interactive.sh, git-rebase.sh

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
Nguyen Thai Ngoc Duy· Nov 9, 2010, 14:15 UTC · re: Jonathan Nieder · lore

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

2010/11/9 Jonathan Nieder <jrnieder@gmail.com>:
Show 21 quoted lines
> 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
Jonathan Nieder· Nov 9, 2010, 14:38 UTC · re: Nguyen Thai Ngoc Duy · lore

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

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"?

Junio C Hamano· Sep 28, 2010, 05:38 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 8 quoted lines
> 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.

Sverre Rabbelier· Sep 28, 2010, 11:24 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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

← back to recent threads