All of lore.kernel.org
 help / color / mirror / Atom feed
* git rebase allows branches to be checked out in multiple worktrees
@ 2020-02-20  0:29 Mike Hommey
  2020-02-23 10:14 ` [PATCH 0/2] git-rebase: refuse to switch to branch checked out elsewhere Eric Sunshine
  0 siblings, 1 reply; 5+ messages in thread
From: Mike Hommey @ 2020-02-20  0:29 UTC (permalink / raw)
  To: git

Hi,

I just noticed a quirk with git rebase vs. worktrees. While using e.g.
git checkout will prevent a same branch from being checked out from
multiple worktrees, that's not the case for git-rebase, so the following
scenario ends up with two checkouts of the same branch:

$ git checkout -b foo HEAD~
$ git commit --allow-empty -m foo 
$ git worktree add /tmp/bar master
$ cd /tmp/bar
$ git rebase master foo

Mike

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 0/2] git-rebase: refuse to switch to branch checked out elsewhere
  2020-02-20  0:29 git rebase allows branches to be checked out in multiple worktrees Mike Hommey
@ 2020-02-23 10:14 ` Eric Sunshine
  2020-02-23 10:14   ` [PATCH 1/2] t3400: make test clean up after itself Eric Sunshine
  2020-02-23 10:14   ` [PATCH 2/2] rebase: refuse to switch to branch already checked out elsewhere Eric Sunshine
  0 siblings, 2 replies; 5+ messages in thread
From: Eric Sunshine @ 2020-02-23 10:14 UTC (permalink / raw)
  To: git; +Cc: Mike Hommey, Eric Sunshine

git-switch, git-checkout, and git-worktree all refuse to switch to a
branch already checked out in some other worktree. Mike Hommey
reported[1] that git-rebase is not careful like those other commands.
This patch series fixes that shortcoming.

[1]: https://lore.kernel.org/git/20200220002932.5jws6qpnivlvxckw@glandium.org/

Eric Sunshine (2):
  t3400: make test clean up after itself
  rebase: refuse to switch to branch already checked out elsewhere

 builtin/rebase.c  |  5 +++--
 t/t3400-rebase.sh | 20 +++++++++++++++++++-
 2 files changed, 22 insertions(+), 3 deletions(-)

-- 
2.25.1.526.gf05a752211


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 1/2] t3400: make test clean up after itself
  2020-02-23 10:14 ` [PATCH 0/2] git-rebase: refuse to switch to branch checked out elsewhere Eric Sunshine
@ 2020-02-23 10:14   ` Eric Sunshine
  2020-02-24  9:19     ` Eric Sunshine
  2020-02-23 10:14   ` [PATCH 2/2] rebase: refuse to switch to branch already checked out elsewhere Eric Sunshine
  1 sibling, 1 reply; 5+ messages in thread
From: Eric Sunshine @ 2020-02-23 10:14 UTC (permalink / raw)
  To: git; +Cc: Mike Hommey, Eric Sunshine

This test intentionally creates a file which causes rebase to fail, thus
it is important that this file be deleted before subsequent tests are
run which are not expecting such a failure. In the past, the common way
to ensure cleanup (regardless of whether the test succeeded or failed)
was either for the next test to perform the previous test's cleanup as
its first step or to do the cleanup at global scope outside of any
tests. With the introduction of 'test_when_finished', however, tests can
be responsible for their own cleanup. Therefore, update this test to
clean up after itself.

A bit of history: This 'rm' invocation was moved from within the body of
the following test to global scope by bffd750adf (rebase: improve error
message when upstream argument is missing, 2010-05-31), which postdates,
by about a month, introduction of 'test_when_finished' in 3bf7886705
(test-lib: Let tests specify commands to be run at end of test,
2010-05-02).
---
 t/t3400-rebase.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 221b35f2df..6e746dca00 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -143,11 +143,11 @@ test_expect_success 'setup: recover' '
 
 test_expect_success 'Show verbose error when HEAD could not be detached' '
 	>B &&
+	test_when_finished "rm -f B" &&
 	test_must_fail git rebase topic 2>output.err >output.out &&
 	test_i18ngrep "The following untracked working tree files would be overwritten by checkout:" output.err &&
 	test_i18ngrep B output.err
 '
-rm -f B
 
 test_expect_success 'fail when upstream arg is missing and not on branch' '
 	git checkout topic &&
-- 
2.25.1.526.gf05a752211


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 2/2] rebase: refuse to switch to branch already checked out elsewhere
  2020-02-23 10:14 ` [PATCH 0/2] git-rebase: refuse to switch to branch checked out elsewhere Eric Sunshine
  2020-02-23 10:14   ` [PATCH 1/2] t3400: make test clean up after itself Eric Sunshine
@ 2020-02-23 10:14   ` Eric Sunshine
  1 sibling, 0 replies; 5+ messages in thread
From: Eric Sunshine @ 2020-02-23 10:14 UTC (permalink / raw)
  To: git; +Cc: Mike Hommey, Eric Sunshine

The invocation "git rebase <upstream> <branch>" switches to <branch>
before performing the rebase operation. However, unlike git-switch,
git-checkout, and git-worktree which all refuse to switch to a branch
that is already checked out in some other worktree, git-rebase switches
to <branch> unconditionally. Curb this careless behavior by making
git-rebase also refuse to switch to a branch checked out elsewhere.

Reported-by: Mike Hommey <mh@glandium.org>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/rebase.c  |  5 +++--
 t/t3400-rebase.sh | 18 ++++++++++++++++++
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 6154ad8fa5..41b9372c0b 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1945,10 +1945,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		/* Is it a local branch? */
 		strbuf_reset(&buf);
 		strbuf_addf(&buf, "refs/heads/%s", branch_name);
-		if (!read_ref(buf.buf, &options.orig_head))
+		if (!read_ref(buf.buf, &options.orig_head)) {
+			die_if_checked_out(buf.buf, 1);
 			options.head_name = xstrdup(buf.buf);
 		/* If not is it a valid ref (branch or commit)? */
-		else if (!get_oid(branch_name, &options.orig_head))
+		} else if (!get_oid(branch_name, &options.orig_head))
 			options.head_name = NULL;
 		else
 			die(_("fatal: no such branch/commit '%s'"),
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 6e746dca00..9aa5268a06 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -377,4 +377,22 @@ test_expect_success 'rebase -c rebase.useBuiltin=false warning' '
 	test_must_be_empty err
 '
 
+test_expect_success 'switch to branch checked out here' '
+	git checkout master &&
+	git rebase master master
+'
+
+test_expect_success 'switch to branch not checked out' '
+	git checkout master &&
+	git branch other &&
+	git rebase master other
+'
+
+test_expect_success 'refuse to switch to branch checked out elsewhere' '
+	git checkout master &&
+	git worktree add wt &&
+	test_must_fail git -C wt rebase master master 2>err &&
+	test_i18ngrep "already checked out" err
+'
+
 test_done
-- 
2.25.1.526.gf05a752211


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] t3400: make test clean up after itself
  2020-02-23 10:14   ` [PATCH 1/2] t3400: make test clean up after itself Eric Sunshine
@ 2020-02-24  9:19     ` Eric Sunshine
  0 siblings, 0 replies; 5+ messages in thread
From: Eric Sunshine @ 2020-02-24  9:19 UTC (permalink / raw)
  To: Git List; +Cc: Mike Hommey

On Sun, Feb 23, 2020 at 5:15 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> This test intentionally creates a file which causes rebase to fail, thus
> it is important that this file be deleted before subsequent tests are
> run which are not expecting such a failure. In the past, the common way
> to ensure cleanup (regardless of whether the test succeeded or failed)
> was either for the next test to perform the previous test's cleanup as
> its first step or to do the cleanup at global scope outside of any
> tests. With the introduction of 'test_when_finished', however, tests can
> be responsible for their own cleanup. Therefore, update this test to
> clean up after itself.
>
> A bit of history: This 'rm' invocation was moved from within the body of
> the following test to global scope by bffd750adf (rebase: improve error
> message when upstream argument is missing, 2010-05-31), which postdates,
> by about a month, introduction of 'test_when_finished' in 3bf7886705
> (test-lib: Let tests specify commands to be run at end of test,
> 2010-05-02).
> ---

Missing sign-off, so here it is:

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-02-24  9:20 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-20  0:29 git rebase allows branches to be checked out in multiple worktrees Mike Hommey
2020-02-23 10:14 ` [PATCH 0/2] git-rebase: refuse to switch to branch checked out elsewhere Eric Sunshine
2020-02-23 10:14   ` [PATCH 1/2] t3400: make test clean up after itself Eric Sunshine
2020-02-24  9:19     ` Eric Sunshine
2020-02-23 10:14   ` [PATCH 2/2] rebase: refuse to switch to branch already checked out elsewhere Eric Sunshine

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.