[1.8.0] git checkout refs/heads/foo checks out branch foo

[1.8.0] git checkout refs/heads/foo checks out branch foo

[Martin von Zweigbergk]

Proposal:

'git checkout refs/heads/foo' (or 'git checkout heads/foo' for that
matter) does not check out the branch, but instead detaches HEAD at
foo. This is quite counter-intuitive (at least to me) and the same
functionality can be achieved by using e.g. foo~0. Change the behavior
so that the branch is actually checked out. This also applies to
e.g. 'git rebase master refs/heads/topic', which currently rebases a
detached HEAD. There are probably other examples as well that I'm not
aware of.


Risks:

Existing scripts may depend on the current behavior. It seems unlikely
that many users depend on it. Most likely, they use foo~0 or foo^0
instead.


Migration plan:

Make 'git checkout refs/head/foo' emit a warning in the next 1.7.x
explaining that its semantics will change in 1.8.0. Then change the
behavior in 1.8.0 and remove the warning.

Re: [1.8.0] git checkout refs/heads/foo checks out branch foo

[Junio C Hamano]

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:

> Proposal:
>
> 'git checkout refs/heads/foo' (or 'git checkout heads/foo' for that
> matter) does not check out the branch, but instead detaches HEAD at
> foo.

Yes, and it is deliberately so as it is a guaranteed-sure way for scripts
to work around potential ref ambiguity when talking about the branch
called 'foo' to spell it out with leading refs/heads/.

As long as you do not break scripts that guard against breakage you are
proposing by saying "refs/heads/foo^0", we are Ok with the proposal, I
think.

> Migration plan:
>
> Make 'git checkout refs/head/foo' emit a warning in the next 1.7.x
> explaining that its semantics will change in 1.8.0. Then change the
> behavior in 1.8.0 and remove the warning.

Reasonable.

Thanks.

Re: [1.8.0] git checkout refs/heads/foo checks out branch foo

[Jeff King]

On Mon, Feb 07, 2011 at 06:01:51AM -0500, Martin von Zweigbergk wrote:

> 'git checkout refs/heads/foo' (or 'git checkout heads/foo' for that
> matter) does not check out the branch, but instead detaches HEAD at
> foo. This is quite counter-intuitive (at least to me) and the same
> functionality can be achieved by using e.g. foo~0. Change the behavior
> so that the branch is actually checked out. This also applies to
> e.g. 'git rebase master refs/heads/topic', which currently rebases a
> detached HEAD. There are probably other examples as well that I'm not
> aware of.

I have seen that behavior claimed as intentional, but I agree it is
unintuitive. In most other places referring to a ref by a short name or
a fully qualified name is equivalent (except with respect to
disambiguating short names, of course).

> Existing scripts may depend on the current behavior. It seems unlikely
> that many users depend on it. Most likely, they use foo~0 or foo^0
> instead.

As cool and clever as the foo^0 behavior is once you understand it, I
think it is a horribly confusing thing for non-experts. As part of this
proposal, should we perhaps offer "git checkout --detach" as the
easy-on-the-eyes way of intentionally detaching?

-Peff

Re: [1.8.0] git checkout refs/heads/foo checks out branch foo

[Heiko Voigt]

Hallo Martin,

On Mon, Feb 07, 2011 at 06:01:51AM -0500, Martin von Zweigbergk wrote:
> Proposal:
> 
> 'git checkout refs/heads/foo' (or 'git checkout heads/foo' for that
> matter) does not check out the branch, but instead detaches HEAD at
> foo. This is quite counter-intuitive (at least to me) and the same
> functionality can be achieved by using e.g. foo~0. Change the behavior
> so that the branch is actually checked out. This also applies to
> e.g. 'git rebase master refs/heads/topic', which currently rebases a
> detached HEAD. There are probably other examples as well that I'm not
> aware of.

Just to clarify: You are not proposing that 'git checkout origin/master'
would also not checkout to a detached head, right? Because that is a
feature I am using frequently to test branches that have been pushed by
another developer to a remote server. If that would create a new local
branch that would be confusing.

Cheers Heiko

Re: [1.8.0] git checkout refs/heads/foo checks out branch foo

[Sverre Rabbelier]

Heya,

On Mon, Feb 7, 2011 at 21:59, Jeff King <peff@peff.net> wrote:
> As cool and clever as the foo^0 behavior is once you understand it, I
> think it is a horribly confusing thing for non-experts. As part of this
> proposal, should we perhaps offer "git checkout --detach" as the
> easy-on-the-eyes way of intentionally detaching?

Now _that_ is an excellent usability improvement, assuming we want to
encourage detaching HEAD... do we?

-- 
Cheers,

Sverre Rabbelier

Re: [1.8.0] git checkout refs/heads/foo checks out branch foo

[Jonathan Nieder]

Sverre Rabbelier wrote:

> Now _that_ is an excellent usability improvement, assuming we want to
> encourage detaching HEAD... do we?

Yes.

-- 8<
Subject: commit: document --detach synonym for "git checkout foo^{commit}"

For example, one might use this when making a temporary merge to test
that two topics work well together.

This patch just documents the option.  It is not meant for application
without an implementation and tests for the option.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/git-checkout.txt |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 880763d..698ae6c 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -9,6 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git checkout' [-q] [-f] [-m] [<branch>]
+'git checkout' [-q] [-f] [-n] [--detach] [<commit>]
 'git checkout' [-q] [-f] [-m] [[-b|-B|--orphan] <new_branch>] [<start_point>]
 'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <paths>...
 'git checkout' --patch [<tree-ish>] [--] [<paths>...]
@@ -22,9 +23,10 @@ branch.
 
 'git checkout' [<branch>]::
 'git checkout' -b|-B <new_branch> [<start point>]::
+'git checkout' [--detach] [<commit>]::
 
 	This form switches branches by updating the index, working
-	tree, and HEAD to reflect the specified branch.
+	tree, and HEAD to reflect the specified branch or commit.
 +
 If `-b` is given, a new branch is created as if linkgit:git-branch[1]
 were called and then checked out; in this case you can
@@ -115,6 +117,13 @@ explicitly give a name with '-b' in such a case.
 	Create the new branch's reflog; see linkgit:git-branch[1] for
 	details.
 
+--detach::
+	Rather than checking out a branch to work on it, check out a
+	commit for inspection and discardable experiments.
+	This is the default behavior of "git checkout <commit>" when
+	<commit> is not a branch name.  See the "DETACHED HEAD" section
+	below for details.
+
 --orphan::
 	Create a new 'orphan' branch, named <new_branch>, started from
 	<start_point> and switch to it.  The first commit made on this
@@ -204,7 +213,7 @@ leave out at most one of `A` and `B`, in which case it defaults to `HEAD`.
 
 
 
-Detached HEAD
+DETACHED HEAD
 -------------
 
 It is sometimes useful to be able to 'checkout' a commit that is
-- 
1.7.4

Re: [1.8.0] git checkout refs/heads/foo checks out branch foo

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Sverre Rabbelier wrote:
>
>> Now _that_ is an excellent usability improvement, assuming we want to
>> encourage detaching HEAD... do we?
>
> Yes.
>
> -- 8<
> Subject: commit: document --detach synonym for "git checkout foo^{commit}"
>
> For example, one might use this when making a temporary merge to test
> that two topics work well together.
>
> This patch just documents the option.  It is not meant for application
> without an implementation and tests for the option.

On top of v1.7.3.5-1-g0cb6ad3 (uk/checkout-ambiguous-ref)...

 builtin/checkout.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 953abdd..141f6a3 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -685,6 +685,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	char *conflict_style = NULL;
 	int patch_mode = 0;
 	int dwim_new_local_branch = 1;
+	int force_detach = 0;
 	struct option options[] = {
 		OPT__QUIET(&opts.quiet),
 		OPT_STRING('b', NULL, &opts.new_branch, "branch",
@@ -692,6 +693,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		OPT_STRING('B', NULL, &opts.new_branch_force, "branch",
 			   "create/reset and checkout a branch"),
 		OPT_BOOLEAN('l', NULL, &opts.new_branch_log, "create reflog for new branch"),
+		OPT_BOOLEAN(0, "detach", &force_detach, "detach the HEAD at named commit"),
 		OPT_SET_INT('t', "track",  &opts.track, "set upstream info for new branch",
 			BRANCH_TRACK_EXPLICIT),
 		OPT_STRING(0, "orphan", &opts.new_orphan_branch, "new branch", "new unparented branch"),
@@ -726,6 +728,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	if (opts.new_branch && opts.new_branch_force)
 		die("-B cannot be used with -b");
 
+	if ((opts.new_branch || opts.new_orphan_branch) && force_detach)
+		die("--detach cannot be used with -b/-B/--orphan");
+
 	/* copy -B over to -b, so that we can just check the latter */
 	if (opts.new_branch_force)
 		opts.new_branch = opts.new_branch_force;
@@ -834,7 +839,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		new.name = arg;
 		setup_branch_path(&new);
 
-		if (check_ref_format(new.path) == CHECK_REF_FORMAT_OK &&
+		if (!force_detach &&
+		    check_ref_format(new.path) == CHECK_REF_FORMAT_OK &&
 		    resolve_ref(new.path, branch_rev, 1, NULL))
 			hashcpy(rev, branch_rev);
 		else

Re: [1.8.0] git checkout refs/heads/foo checks out branch foo

[Jeff King]

On Mon, Feb 07, 2011 at 03:37:15PM -0800, Junio C Hamano wrote:

> > Subject: commit: document --detach synonym for "git checkout foo^{commit}"
> >
> > For example, one might use this when making a temporary merge to test
> > that two topics work well together.
> >
> > This patch just documents the option.  It is not meant for application
> > without an implementation and tests for the option.
> 
> On top of v1.7.3.5-1-g0cb6ad3 (uk/checkout-ambiguous-ref)...

Well, I started on tests and your email came just as I was about to
actually implement. So here are the tests. We didn't seem to have any
explicit checkout-detached tests before, so I tried to cover existing
methods in addition to the new option (which means Martin will need to
tweak one of the tests below when implementing his proposal).

Jonathan, do you want to roll all of these up into a single patch?

---
 t/t2020-checkout-detach.sh |   62 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 62 insertions(+), 0 deletions(-)
 create mode 100755 t/t2020-checkout-detach.sh

diff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh
new file mode 100755
index 0000000..886e186
--- /dev/null
+++ b/t/t2020-checkout-detach.sh
@@ -0,0 +1,62 @@
+#!/bin/sh
+
+test_description='checkout into detached HEAD state'
+. ./test-lib.sh
+
+check_detached() {
+	! git symbolic-ref -q HEAD >/dev/null
+}
+
+check_not_detached() {
+	! check_detached
+}
+
+reset() {
+	git checkout master &&
+	check_not_detached
+}
+
+test_expect_success 'setup' '
+	test_commit one &&
+	test_commit two &&
+	git branch branch &&
+	git tag tag
+'
+
+test_expect_success 'checkout branch does not detach' '
+	reset &&
+	git checkout branch &&
+	check_not_detached
+'
+
+test_expect_success 'checkout tag detaches' '
+	reset &&
+	git checkout tag &&
+	check_detached
+'
+
+test_expect_success 'checkout branch by full name detaches' '
+	reset &&
+	git checkout refs/heads/branch &&
+	check_detached
+'
+
+test_expect_success 'checkout non-ref detaches' '
+	reset &&
+	git checkout branch^ &&
+	check_detached
+'
+
+test_expect_success 'checkout ref^0 detaches' '
+	reset &&
+	git checkout branch^0 &&
+	check_detached
+'
+
+test_expect_success 'checkout --detach detaches' '
+	reset &&
+	git checkout --detach branch &&
+	check_detached
+'
+
+test_done
-- 
1.7.4.rc2.27.gd0787

Re: [1.8.0] git checkout refs/heads/foo checks out branch foo

[Jonathan Nieder]

Jeff King wrote:

> Jonathan, do you want to roll all of these up into a single patch?

Sounds good.

> +++ b/t/t2020-checkout-detach.sh
[...]
> +check_detached() {
> +	! git symbolic-ref -q HEAD >/dev/null
> +}
> +
> +check_not_detached() {
> +	! check_detached
> +}

To be pedantic, I'll put

 check_detached () {
	test_must_fail git symbolic-ref -q HEAD >/dev/null
 }

 check_not_detached () {
	git symbolic-ref -q HEAD >/dev/null
 }

and add some more paranoid tests:

 test_expect_success 'checkout --detach without branch name detaches' '
        reset &&
	git checkout --detach &&
	check_detached
 '

 test_expect_success 'checkout --detach catches error in usage' '
	reset &&
	git checkout master &&
	test_must_fail git checkout --detach tag nonsense &&
	check_not_detached
 '

 test_expect_success 'checkout --detach moves HEAD' '
	reset &&
	git checkout one &&
	git checkout --detach two &&
	git diff --exit-code HEAD &&
	git diff --exit-code two
 '

Re: [1.8.0] git checkout refs/heads/foo checks out branch foo

[Martin von Zweigbergk]

On Mon, 7 Feb 2011, Heiko Voigt wrote:

> Hallo Martin,
> 
> On Mon, Feb 07, 2011 at 06:01:51AM -0500, Martin von Zweigbergk wrote:
> > Proposal:
> > 
> > 'git checkout refs/heads/foo' (or 'git checkout heads/foo' for that
> > matter) does not check out the branch, but instead detaches HEAD at
> > foo. This is quite counter-intuitive (at least to me) and the same
> > functionality can be achieved by using e.g. foo~0. Change the behavior
> > so that the branch is actually checked out. This also applies to
> > e.g. 'git rebase master refs/heads/topic', which currently rebases a
> > detached HEAD. There are probably other examples as well that I'm not
> > aware of.
> 
> Just to clarify: You are not proposing that 'git checkout origin/master'
> would also not checkout to a detached head, right? Because that is a
> feature I am using frequently to test branches that have been pushed by
> another developer to a remote server. If that would create a new local
> branch that would be confusing.

Nope, I'm not proposing that. I wouldn't want that either.

/Martin

Re: [1.8.0] git checkout refs/heads/foo checks out branch foo

[Jeff King]

On Mon, Feb 07, 2011 at 06:01:52PM -0600, Jonathan Nieder wrote:

> > Jonathan, do you want to roll all of these up into a single patch?
> 
> Sounds good.

Thanks.

> To be pedantic, I'll put
> [...]

All look good to me.

-Peff

Re: [1.8.0] git checkout refs/heads/foo checks out branch foo

[Martin von Zweigbergk]

On Mon, 7 Feb 2011, Jeff King wrote:

> On Mon, Feb 07, 2011 at 03:37:15PM -0800, Junio C Hamano wrote:
> 
> > > Subject: commit: document --detach synonym for "git checkout foo^{commit}"
> > >
> > > For example, one might use this when making a temporary merge to test
> > > that two topics work well together.
> > >
> > > This patch just documents the option.  It is not meant for application
> > > without an implementation and tests for the option.
> > 
> > On top of v1.7.3.5-1-g0cb6ad3 (uk/checkout-ambiguous-ref)...
> 
> Well, I started on tests and your email came just as I was about to
> actually implement. So here are the tests. We didn't seem to have any
> explicit checkout-detached tests before, so I tried to cover existing
> methods in addition to the new option (which means Martin will need to
> tweak one of the tests below when implementing his proposal).

Oops, I didn't know writing a proposal meant signing up to do it as
well ;-). But seriously, this seems like a relatively small change, so
it will hopefully be a good reason for me to push myself to get
started with the C code as well.

[PATCH/WIP] checkout: introduce --detach synonym for "git checkout foo^{commit}"

[Jonathan Nieder]

From: Junio C Hamano <gitster@pobox.com>

For example, one might use this when making a temporary merge to
test that two topics work well together.

Patch by Junio, tests from Jeff King.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jeff King wrote:

> Jonathan, do you want to roll all of these up into a single patch?

Okay, here it is.  Two of the new tests fail. :)

 Documentation/git-checkout.txt |   13 +++++-
 builtin/checkout.c             |    8 +++-
 t/t2020-checkout-detach.sh     |   89 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 107 insertions(+), 3 deletions(-)
 create mode 100755 t/t2020-checkout-detach.sh

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 22d3611..d162117 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -9,6 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git checkout' [-q] [-f] [-m] [<branch>]
+'git checkout' [-q] [-f] [-m] [--detach] [<commit>]
 'git checkout' [-q] [-f] [-m] [[-b|-B|--orphan] <new_branch>] [<start_point>]
 'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <paths>...
 'git checkout' --patch [<tree-ish>] [--] [<paths>...]
@@ -22,9 +23,10 @@ branch.
 
 'git checkout' [<branch>]::
 'git checkout' -b|-B <new_branch> [<start point>]::
+'git checkout' [--detach] [<commit>]::
 
 	This form switches branches by updating the index, working
-	tree, and HEAD to reflect the specified branch.
+	tree, and HEAD to reflect the specified branch or commit.
 +
 If `-b` is given, a new branch is created as if linkgit:git-branch[1]
 were called and then checked out; in this case you can
@@ -115,6 +117,13 @@ explicitly give a name with '-b' in such a case.
 	Create the new branch's reflog; see linkgit:git-branch[1] for
 	details.
 
+--detach::
+	Rather than checking out a branch to work on it, check out a
+	commit for inspection and discardable experiments.
+	This is the default behavior of "git checkout <commit>" when
+	<commit> is not a branch name.  See the "DETACHED HEAD" section
+	below for details.
+
 --orphan::
 	Create a new 'orphan' branch, named <new_branch>, started from
 	<start_point> and switch to it.  The first commit made on this
@@ -204,7 +213,7 @@ leave out at most one of `A` and `B`, in which case it defaults to `HEAD`.
 
 
 
-Detached HEAD
+DETACHED HEAD
 -------------
 
 It is sometimes useful to be able to 'checkout' a commit that is
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 953abdd..526abb9 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -685,6 +685,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	char *conflict_style = NULL;
 	int patch_mode = 0;
 	int dwim_new_local_branch = 1;
+	int force_detach = 0;
 	struct option options[] = {
 		OPT__QUIET(&opts.quiet),
 		OPT_STRING('b', NULL, &opts.new_branch, "branch",
@@ -692,6 +693,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		OPT_STRING('B', NULL, &opts.new_branch_force, "branch",
 			   "create/reset and checkout a branch"),
 		OPT_BOOLEAN('l', NULL, &opts.new_branch_log, "create reflog for new branch"),
+		OPT_BOOLEAN(0, "detach", &force_detach, "detach the HEAD at named commit"),
 		OPT_SET_INT('t', "track",  &opts.track, "set upstream info for new branch",
 			BRANCH_TRACK_EXPLICIT),
 		OPT_STRING(0, "orphan", &opts.new_orphan_branch, "new branch", "new unparented branch"),
@@ -726,6 +728,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	if (opts.new_branch && opts.new_branch_force)
 		die("-B cannot be used with -b");
 
+	if ((opts.new_branch || opts.new_orphan_branch) && force_detach)
+		die("--detach cannot be used with -b/-B/--orphan");
+
 	/* copy -B over to -b, so that we can just check the latter */
 	if (opts.new_branch_force)
 		opts.new_branch = opts.new_branch_force;
@@ -834,7 +839,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		new.name = arg;
 		setup_branch_path(&new);
 
-		if (check_ref_format(new.path) == CHECK_REF_FORMAT_OK &&
+		if (!force_detach &&
+		    check_ref_format(new.path) == CHECK_REF_FORMAT_OK &&
 		    resolve_ref(new.path, branch_rev, 1, NULL))
 			hashcpy(rev, branch_rev);
 		else
diff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh
new file mode 100755
index 0000000..e57f253
--- /dev/null
+++ b/t/t2020-checkout-detach.sh
@@ -0,0 +1,89 @@
+#!/bin/sh
+
+test_description='checkout into detached HEAD state'
+. ./test-lib.sh
+
+check_detached () {
+	test_must_fail git symbolic-ref -q HEAD >/dev/null
+}
+
+check_not_detached () {
+	git symbolic-ref -q HEAD >/dev/null
+}
+
+reset () {
+	git checkout master &&
+	check_not_detached
+}
+
+test_expect_success 'setup' '
+	test_commit one &&
+	test_commit two &&
+	git branch branch &&
+	git tag tag
+'
+
+test_expect_success 'checkout branch does not detach' '
+	reset &&
+	git checkout branch &&
+	check_not_detached
+'
+
+test_expect_success 'checkout tag detaches' '
+	reset &&
+	git checkout tag &&
+	check_detached
+'
+
+test_expect_success 'checkout branch by full name detaches' '
+	reset &&
+	git checkout refs/heads/branch &&
+	check_detached
+'
+
+test_expect_success 'checkout non-ref detaches' '
+	reset &&
+	git checkout branch^ &&
+	check_detached
+'
+
+test_expect_success 'checkout ref^0 detaches' '
+	reset &&
+	git checkout branch^0 &&
+	check_detached
+'
+
+test_expect_success 'checkout --detach detaches' '
+	reset &&
+	git checkout --detach branch &&
+	check_detached
+'
+
+test_expect_failure 'checkout --detach without branch name' '
+	reset &&
+	git checkout --detach &&
+	check_detached
+'
+
+test_expect_failure 'checkout --detach errors out for extra argument' '
+	reset &&
+	git checkout master &&
+	test_expect_code 129 git checkout --detach tag nonsense &&
+	check_not_detached
+'
+
+test_expect_success 'checkout --detached and -b are incompatible' '
+	check_not_detached &&
+	test_must_fail git checkout --detach -b newbranch tag &&
+	check_not_detached
+'
+
+test_expect_success 'checkout --detach moves HEAD' '
+	reset &&
+	git checkout one &&
+	git checkout --detach two &&
+	git diff --exit-code HEAD &&
+	git diff --exit-code two
+'
+
+test_done
-- 
1.7.4

Re: [PATCH/WIP] checkout: introduce --detach synonym for "git checkout foo^{commit}"

[Jonathan Nieder]

Jonathan Nieder wrote:

> Jeff King wrote:
>
>> Jonathan, do you want to roll all of these up into a single patch?
>
> Okay, here it is.

Ah, should have mentioned: this applies on top of a merge of
en/and-cascade-tests~25 (aka v1.7.4-rc0~65^2~19, test-lib: make
test_expect_code a test command, 2010-10-03) and
uk/checkout-ambiguous-ref.

[PATCH v2 0/3] checkout: introduce --detach synonym for "git checkout foo^{commit}"

[Jonathan Nieder]

Jonathan Nieder wrote:

> Two of the new tests fail. :)

... and here's a reroll fixing them.  Applies on top of the old
uk/checkout-ambiguous-ref (0cb6ad3, 2011-01-11).

Patch 1 is an irrelevant cleanup, for while I have your eyes on this
code. :)

Patch 2 introduces the --detach option, taking care to error out in
special cases.

Patch 3 is another cleanup, to make the effect of patch 2 more clear.

Thanks again for your help.
Jonathan Nieder (2):
  checkout: split off a function to peel away branchname arg
  checkout: rearrange update_refs_for_switch for clarity

Junio C Hamano (1):
  checkout: introduce --detach synonym for "git checkout foo^{commit}"

 Documentation/git-checkout.txt |   13 ++-
 builtin/checkout.c             |  265 +++++++++++++++++++++++----------------
 t/t2020-checkout-detach.sh     |   95 ++++++++++++++
 3 files changed, 262 insertions(+), 111 deletions(-)
 create mode 100755 t/t2020-checkout-detach.sh

[PATCH 1/3] checkout: split off a function to peel away branchname arg

[Jonathan Nieder]

The code to parse and consume the tree name and "--" in commands such
as "git checkout @{-1} -- '*.c'" is intimidatingly long.  Split it out
into a separate function and make it easier to skip on first reading
by making the data it uses and produces more explicit.

No functional change intended.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Just some code movement.

 builtin/checkout.c |  229 ++++++++++++++++++++++++++++++----------------------
 1 files changed, 131 insertions(+), 98 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 953abdd..0e7a6a3 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -675,11 +675,123 @@ static const char *unique_tracking_name(const char *name)
 	return NULL;
 }
 
+static int parse_branchname_arg(int argc, const char **argv,
+				int dwim_new_local_branch_ok,
+				struct branch_info *new,
+				struct tree **source_tree,
+				unsigned char rev[20],
+				const char **new_branch)
+{
+	int argcount = 0;
+	unsigned char branch_rev[20];
+	const char *arg;
+	int has_dash_dash;
+
+	/*
+	 * case 1: git checkout <ref> -- [<paths>]
+	 *
+	 *   <ref> must be a valid tree, everything after the '--' must be
+	 *   a path.
+	 *
+	 * case 2: git checkout -- [<paths>]
+	 *
+	 *   everything after the '--' must be paths.
+	 *
+	 * case 3: git checkout <something> [<paths>]
+	 *
+	 *   With no paths, if <something> is a commit, that is to
+	 *   switch to the branch or detach HEAD at it.  As a special case,
+	 *   if <something> is A...B (missing A or B means HEAD but you can
+	 *   omit at most one side), and if there is a unique merge base
+	 *   between A and B, A...B names that merge base.
+	 *
+	 *   With no paths, if <something> is _not_ a commit, no -t nor -b
+	 *   was given, and there is a tracking branch whose name is
+	 *   <something> in one and only one remote, then this is a short-hand
+	 *   to fork local <something> from that remote tracking branch.
+	 *
+	 *   Otherwise <something> shall not be ambiguous.
+	 *   - If it's *only* a reference, treat it like case (1).
+	 *   - If it's only a path, treat it like case (2).
+	 *   - else: fail.
+	 *
+	 */
+	if (!argc)
+		return 0;
+
+	if (!strcmp(argv[0], "--"))	/* case (2) */
+		return 1;
+
+	arg = argv[0];
+	has_dash_dash = (argc > 1) && !strcmp(argv[1], "--");
+
+	if (!strcmp(arg, "-"))
+		arg = "@{-1}";
+
+	if (get_sha1_mb(arg, rev)) {
+		if (has_dash_dash)          /* case (1) */
+			die("invalid reference: %s", arg);
+		if (dwim_new_local_branch_ok &&
+		    !check_filename(NULL, arg) &&
+		    argc == 1) {
+			const char *remote = unique_tracking_name(arg);
+			if (!remote || get_sha1(remote, rev))
+				return argcount;
+			*new_branch = arg;
+			arg = remote;
+			/* DWIMmed to create local branch */
+		} else {
+			return argcount;
+		}
+	}
+
+	/* we can't end up being in (2) anymore, eat the argument */
+	argcount++;
+	argv++;
+	argc--;
+
+	new->name = arg;
+	setup_branch_path(new);
+
+	if (check_ref_format(new->path) == CHECK_REF_FORMAT_OK &&
+	    resolve_ref(new->path, branch_rev, 1, NULL))
+		hashcpy(rev, branch_rev);
+	else
+		new->path = NULL; /* not an existing branch */
+
+	new->commit = lookup_commit_reference_gently(rev, 1);
+	if (!new->commit) {
+		/* not a commit */
+		*source_tree = parse_tree_indirect(rev);
+	} else {
+		parse_commit(new->commit);
+		*source_tree = new->commit->tree;
+	}
+
+	if (!*source_tree)                   /* case (1): want a tree */
+		die("reference is not a tree: %s", arg);
+	if (!has_dash_dash) {/* case (3 -> 1) */
+		/*
+		 * Do not complain the most common case
+		 *	git checkout branch
+		 * even if there happen to be a file called 'branch';
+		 * it would be extremely annoying.
+		 */
+		if (argc)
+			verify_non_filename(NULL, arg);
+	} else {
+		argcount++;
+		argv++;
+		argc--;
+	}
+
+	return argcount;
+}
+
 int cmd_checkout(int argc, const char **argv, const char *prefix)
 {
 	struct checkout_opts opts;
-	unsigned char rev[20], branch_rev[20];
-	const char *arg;
+	unsigned char rev[20];
 	struct branch_info new;
 	struct tree *source_tree = NULL;
 	char *conflict_style = NULL;
@@ -709,7 +821,6 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN },
 		OPT_END(),
 	};
-	int has_dash_dash;
 
 	memset(&opts, 0, sizeof(opts));
 	memset(&new, 0, sizeof(new));
@@ -766,108 +877,30 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		die("git checkout: -f and -m are incompatible");
 
 	/*
-	 * case 1: git checkout <ref> -- [<paths>]
-	 *
-	 *   <ref> must be a valid tree, everything after the '--' must be
-	 *   a path.
-	 *
-	 * case 2: git checkout -- [<paths>]
-	 *
-	 *   everything after the '--' must be paths.
-	 *
-	 * case 3: git checkout <something> [<paths>]
-	 *
-	 *   With no paths, if <something> is a commit, that is to
-	 *   switch to the branch or detach HEAD at it.  As a special case,
-	 *   if <something> is A...B (missing A or B means HEAD but you can
-	 *   omit at most one side), and if there is a unique merge base
-	 *   between A and B, A...B names that merge base.
-	 *
-	 *   With no paths, if <something> is _not_ a commit, no -t nor -b
-	 *   was given, and there is a tracking branch whose name is
-	 *   <something> in one and only one remote, then this is a short-hand
-	 *   to fork local <something> from that remote tracking branch.
-	 *
-	 *   Otherwise <something> shall not be ambiguous.
-	 *   - If it's *only* a reference, treat it like case (1).
-	 *   - If it's only a path, treat it like case (2).
-	 *   - else: fail.
-	 *
+	 * Extract branch name from command line arguments, so
+	 * all that is left is pathspecs.
+	 *
+	 * Handle
+	 *
+	 *  1) git checkout <tree> -- [<paths>]
+	 *  2) git checkout -- [<paths>]
+	 *  3) git checkout <something> [<paths>]
+	 *
+	 * including "last branch" syntax and DWIM-ery for names of
+	 * remote branches, erroring out for invalid or ambiguous cases.
 	 */
 	if (argc) {
-		if (!strcmp(argv[0], "--")) {       /* case (2) */
-			argv++;
-			argc--;
-			goto no_reference;
-		}
-
-		arg = argv[0];
-		has_dash_dash = (argc > 1) && !strcmp(argv[1], "--");
-
-		if (!strcmp(arg, "-"))
-			arg = "@{-1}";
-
-		if (get_sha1_mb(arg, rev)) {
-			if (has_dash_dash)          /* case (1) */
-				die("invalid reference: %s", arg);
-			if (!patch_mode &&
-			    dwim_new_local_branch &&
-			    opts.track == BRANCH_TRACK_UNSPECIFIED &&
-			    !opts.new_branch &&
-			    !check_filename(NULL, arg) &&
-			    argc == 1) {
-				const char *remote = unique_tracking_name(arg);
-				if (!remote || get_sha1(remote, rev))
-					goto no_reference;
-				opts.new_branch = arg;
-				arg = remote;
-				/* DWIMmed to create local branch */
-			}
-			else
-				goto no_reference;
-		}
-
-		/* we can't end up being in (2) anymore, eat the argument */
-		argv++;
-		argc--;
-
-		new.name = arg;
-		setup_branch_path(&new);
-
-		if (check_ref_format(new.path) == CHECK_REF_FORMAT_OK &&
-		    resolve_ref(new.path, branch_rev, 1, NULL))
-			hashcpy(rev, branch_rev);
-		else
-			new.path = NULL; /* not an existing branch */
-
-		if (!(new.commit = lookup_commit_reference_gently(rev, 1))) {
-			/* not a commit */
-			source_tree = parse_tree_indirect(rev);
-		} else {
-			parse_commit(new.commit);
-			source_tree = new.commit->tree;
-		}
-
-		if (!source_tree)                   /* case (1): want a tree */
-			die("reference is not a tree: %s", arg);
-		if (!has_dash_dash) {/* case (3 -> 1) */
-			/*
-			 * Do not complain the most common case
-			 *	git checkout branch
-			 * even if there happen to be a file called 'branch';
-			 * it would be extremely annoying.
-			 */
-			if (argc)
-				verify_non_filename(NULL, arg);
-		}
-		else {
-			argv++;
-			argc--;
-		}
+		int dwim_ok =
+			!patch_mode &&
+			dwim_new_local_branch &&
+			opts.track == BRANCH_TRACK_UNSPECIFIED &&
+			!opts.new_branch;
+		int n = parse_branchname_arg(argc, argv, dwim_ok,
+				&new, &source_tree, rev, &opts.new_branch);
+		argv += n;
+		argc -= n;
 	}
 
-no_reference:
-
 	if (opts.track == BRANCH_TRACK_UNSPECIFIED)
 		opts.track = git_branch_track;
 
-- 
1.7.4

[PATCH 2/3] checkout: introduce --detach synonym for "git checkout foo^{commit}"

[Jonathan Nieder]

From: Junio C Hamano <gitster@pobox.com>

For example, one might use this when making a temporary merge to
test that two topics work well together.

Patch by Junio, with tests from Jeff King.

[jn: with some extra checks for bogus commandline usage]

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/git-checkout.txt |   13 +++++-
 builtin/checkout.c             |   25 ++++++++--
 t/t2020-checkout-detach.sh     |   95 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 126 insertions(+), 7 deletions(-)
 create mode 100755 t/t2020-checkout-detach.sh

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 22d3611..d162117 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -9,6 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git checkout' [-q] [-f] [-m] [<branch>]
+'git checkout' [-q] [-f] [-m] [--detach] [<commit>]
 'git checkout' [-q] [-f] [-m] [[-b|-B|--orphan] <new_branch>] [<start_point>]
 'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <paths>...
 'git checkout' --patch [<tree-ish>] [--] [<paths>...]
@@ -22,9 +23,10 @@ branch.
 
 'git checkout' [<branch>]::
 'git checkout' -b|-B <new_branch> [<start point>]::
+'git checkout' [--detach] [<commit>]::
 
 	This form switches branches by updating the index, working
-	tree, and HEAD to reflect the specified branch.
+	tree, and HEAD to reflect the specified branch or commit.
 +
 If `-b` is given, a new branch is created as if linkgit:git-branch[1]
 were called and then checked out; in this case you can
@@ -115,6 +117,13 @@ explicitly give a name with '-b' in such a case.
 	Create the new branch's reflog; see linkgit:git-branch[1] for
 	details.
 
+--detach::
+	Rather than checking out a branch to work on it, check out a
+	commit for inspection and discardable experiments.
+	This is the default behavior of "git checkout <commit>" when
+	<commit> is not a branch name.  See the "DETACHED HEAD" section
+	below for details.
+
 --orphan::
 	Create a new 'orphan' branch, named <new_branch>, started from
 	<start_point> and switch to it.  The first commit made on this
@@ -204,7 +213,7 @@ leave out at most one of `A` and `B`, in which case it defaults to `HEAD`.
 
 
 
-Detached HEAD
+DETACHED HEAD
 -------------
 
 It is sometimes useful to be able to 'checkout' a commit that is
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 0e7a6a3..51ec977 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -30,6 +30,7 @@ struct checkout_opts {
 	int quiet;
 	int merge;
 	int force;
+	int force_detach;
 	int writeout_stage;
 	int writeout_error;
 
@@ -563,7 +564,7 @@ static void update_refs_for_switch(struct checkout_opts *opts,
 			if (!file_exists(ref_file) && file_exists(log_file))
 				remove_path(log_file);
 		}
-	} else if (strcmp(new->name, "HEAD")) {
+	} else if (opts->force_detach || strcmp(new->name, "HEAD")) {
 		update_ref(msg.buf, "HEAD", new->commit->object.sha1, NULL,
 			   REF_NODEREF, DIE_ON_ERR);
 		if (!opts->quiet) {
@@ -574,7 +575,8 @@ static void update_refs_for_switch(struct checkout_opts *opts,
 	}
 	remove_branch_state();
 	strbuf_release(&msg);
-	if (!opts->quiet && (new->path || !strcmp(new->name, "HEAD")))
+	if (!opts->quiet &&
+	    (new->path || (!opts->force_detach && !strcmp(new->name, "HEAD"))))
 		report_tracking(new);
 }
 
@@ -677,6 +679,7 @@ static const char *unique_tracking_name(const char *name)
 
 static int parse_branchname_arg(int argc, const char **argv,
 				int dwim_new_local_branch_ok,
+				int force_detach,
 				struct branch_info *new,
 				struct tree **source_tree,
 				unsigned char rev[20],
@@ -753,7 +756,8 @@ static int parse_branchname_arg(int argc, const char **argv,
 	new->name = arg;
 	setup_branch_path(new);
 
-	if (check_ref_format(new->path) == CHECK_REF_FORMAT_OK &&
+	if (!force_detach &&
+	    check_ref_format(new->path) == CHECK_REF_FORMAT_OK &&
 	    resolve_ref(new->path, branch_rev, 1, NULL))
 		hashcpy(rev, branch_rev);
 	else
@@ -804,6 +808,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		OPT_STRING('B', NULL, &opts.new_branch_force, "branch",
 			   "create/reset and checkout a branch"),
 		OPT_BOOLEAN('l', NULL, &opts.new_branch_log, "create reflog for new branch"),
+		OPT_BOOLEAN(0, "detach", &opts.force_detach, "detach the HEAD at named commit"),
 		OPT_SET_INT('t', "track",  &opts.track, "set upstream info for new branch",
 			BRANCH_TRACK_EXPLICIT),
 		OPT_STRING(0, "orphan", &opts.new_orphan_branch, "new branch", "new unparented branch"),
@@ -842,9 +847,15 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		opts.new_branch = opts.new_branch_force;
 
 	if (patch_mode && (opts.track > 0 || opts.new_branch
-			   || opts.new_branch_log || opts.merge || opts.force))
+			   || opts.new_branch_log || opts.merge || opts.force
+			   || opts.force_detach))
 		die ("--patch is incompatible with all other options");
 
+	if (opts.force_detach && (opts.new_branch || opts.new_orphan_branch))
+		die("--detach cannot be used with -b/-B/--orphan");
+	if (opts.force_detach && 0 < opts.track)
+		die("--detach cannot be used with -t");
+
 	/* --track without -b should DWIM */
 	if (0 < opts.track && !opts.new_branch) {
 		const char *argv0 = argv[0];
@@ -895,7 +906,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 			dwim_new_local_branch &&
 			opts.track == BRANCH_TRACK_UNSPECIFIED &&
 			!opts.new_branch;
-		int n = parse_branchname_arg(argc, argv, dwim_ok,
+		int n = parse_branchname_arg(argc, argv,
+				dwim_ok, opts.force_detach,
 				&new, &source_tree, rev, &opts.new_branch);
 		argv += n;
 		argc -= n;
@@ -922,6 +934,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 			}
 		}
 
+		if (opts.force_detach)
+			die("git checkout: --detach does not take a path argument");
+
 		if (1 < !!opts.writeout_stage + !!opts.force + !!opts.merge)
 			die("git checkout: --ours/--theirs, --force and --merge are incompatible when\nchecking out of the index.");
 
diff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh
new file mode 100755
index 0000000..0042145
--- /dev/null
+++ b/t/t2020-checkout-detach.sh
@@ -0,0 +1,95 @@
+#!/bin/sh
+
+test_description='checkout into detached HEAD state'
+. ./test-lib.sh
+
+check_detached () {
+	test_must_fail git symbolic-ref -q HEAD >/dev/null
+}
+
+check_not_detached () {
+	git symbolic-ref -q HEAD >/dev/null
+}
+
+reset () {
+	git checkout master &&
+	check_not_detached
+}
+
+test_expect_success 'setup' '
+	test_commit one &&
+	test_commit two &&
+	git branch branch &&
+	git tag tag
+'
+
+test_expect_success 'checkout branch does not detach' '
+	reset &&
+	git checkout branch &&
+	check_not_detached
+'
+
+test_expect_success 'checkout tag detaches' '
+	reset &&
+	git checkout tag &&
+	check_detached
+'
+
+test_expect_success 'checkout branch by full name detaches' '
+	reset &&
+	git checkout refs/heads/branch &&
+	check_detached
+'
+
+test_expect_success 'checkout non-ref detaches' '
+	reset &&
+	git checkout branch^ &&
+	check_detached
+'
+
+test_expect_success 'checkout ref^0 detaches' '
+	reset &&
+	git checkout branch^0 &&
+	check_detached
+'
+
+test_expect_success 'checkout --detach detaches' '
+	reset &&
+	git checkout --detach branch &&
+	check_detached
+'
+
+test_expect_success 'checkout --detach without branch name' '
+	reset &&
+	git checkout --detach &&
+	check_detached
+'
+
+test_expect_success 'checkout --detach errors out for non-commit' '
+	reset &&
+	test_must_fail git checkout --detach one^{tree} &&
+	check_not_detached
+'
+
+test_expect_success 'checkout --detach errors out for extra argument' '
+	reset &&
+	git checkout master &&
+	test_must_fail git checkout --detach tag one.t &&
+	check_not_detached
+'
+
+test_expect_success 'checkout --detached and -b are incompatible' '
+	reset &&
+	test_must_fail git checkout --detach -b newbranch tag &&
+	check_not_detached
+'
+
+test_expect_success 'checkout --detach moves HEAD' '
+	reset &&
+	git checkout one &&
+	git checkout --detach two &&
+	git diff --exit-code HEAD &&
+	git diff --exit-code two
+'
+
+test_done
-- 
1.7.4

[PATCH 3/3] checkout: rearrange update_refs_for_switch for clarity

[Jonathan Nieder]

Take care of simple, exceptional cases before the meat of the "check
out by branch name" code begins.  After this change, the function
vaguely follows the following pseudocode:

	if (-B or -b)
		create branch;
	if (plain "git checkout" or "git checkout HEAD")
		;
	else if (--detach or checking out by non-branch commit name)
		detach HEAD;
	else if (checking out by branch name)
		attach HEAD;

One nice side benefit is to make it possible to remove handling of
the --detach option from outside switch_branches.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
That's the end of the series.  Thoughts welcome, as always.

'night,
Jonathan

 builtin/checkout.c |   27 +++++++++++++--------------
 1 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 51ec977..179d047 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -542,7 +542,17 @@ static void update_refs_for_switch(struct checkout_opts *opts,
 	strbuf_addf(&msg, "checkout: moving from %s to %s",
 		    old_desc ? old_desc : "(invalid)", new->name);
 
-	if (new->path) {
+	if (!strcmp(new->name, "HEAD") && !new->path && !opts->force_detach) {
+		/* Nothing to do. */
+	} else if (opts->force_detach || !new->path) {	/* No longer on any branch. */
+		update_ref(msg.buf, "HEAD", new->commit->object.sha1, NULL,
+			   REF_NODEREF, DIE_ON_ERR);
+		if (!opts->quiet) {
+			if (old->path && advice_detached_head)
+				detach_advice(old->path, new->name);
+			describe_detached_head("HEAD is now at", new->commit);
+		}
+	} else if (new->path) {	/* Switch branches. */
 		create_symref("HEAD", new->path, msg.buf);
 		if (!opts->quiet) {
 			if (old->path && !strcmp(new->path, old->path))
@@ -564,14 +574,6 @@ static void update_refs_for_switch(struct checkout_opts *opts,
 			if (!file_exists(ref_file) && file_exists(log_file))
 				remove_path(log_file);
 		}
-	} else if (opts->force_detach || strcmp(new->name, "HEAD")) {
-		update_ref(msg.buf, "HEAD", new->commit->object.sha1, NULL,
-			   REF_NODEREF, DIE_ON_ERR);
-		if (!opts->quiet) {
-			if (old->path && advice_detached_head)
-				detach_advice(old->path, new->name);
-			describe_detached_head("HEAD is now at", new->commit);
-		}
 	}
 	remove_branch_state();
 	strbuf_release(&msg);
@@ -679,7 +681,6 @@ static const char *unique_tracking_name(const char *name)
 
 static int parse_branchname_arg(int argc, const char **argv,
 				int dwim_new_local_branch_ok,
-				int force_detach,
 				struct branch_info *new,
 				struct tree **source_tree,
 				unsigned char rev[20],
@@ -756,8 +757,7 @@ static int parse_branchname_arg(int argc, const char **argv,
 	new->name = arg;
 	setup_branch_path(new);
 
-	if (!force_detach &&
-	    check_ref_format(new->path) == CHECK_REF_FORMAT_OK &&
+	if (check_ref_format(new->path) == CHECK_REF_FORMAT_OK &&
 	    resolve_ref(new->path, branch_rev, 1, NULL))
 		hashcpy(rev, branch_rev);
 	else
@@ -906,8 +906,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 			dwim_new_local_branch &&
 			opts.track == BRANCH_TRACK_UNSPECIFIED &&
 			!opts.new_branch;
-		int n = parse_branchname_arg(argc, argv,
-				dwim_ok, opts.force_detach,
+		int n = parse_branchname_arg(argc, argv, dwim_ok,
 				&new, &source_tree, rev, &opts.new_branch);
 		argv += n;
 		argc -= n;
-- 
1.7.4