git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* Bug: "git checkout -b" should be allowed in empty repo
@ 2012-01-29  6:09 Michael Haggerty
  2012-01-29  6:56 ` Junio C Hamano
  0 siblings, 1 reply; 34+ messages in thread
From: Michael Haggerty @ 2012-01-29  6:09 UTC (permalink / raw
  To: git

When starting a new repo, git seems to insist that the first commit be
made on a branch named "master":

    $ git --version
    git version 1.7.9
    $ git init git-test
    Initialized empty Git repository in /home/mhagger/tmp/git-test/.git/
    $ cd git-test
    $ git checkout -b foo
    fatal: You are on a branch yet to be born

I would call this a bug; the last command should be allowed.  The
plumbing allows it:

    $ git symbolic-ref HEAD refs/heads/foo

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-01-29  6:09 Bug: "git checkout -b" should be allowed in empty repo Michael Haggerty
@ 2012-01-29  6:56 ` Junio C Hamano
  2012-01-29  7:50   ` Junio C Hamano
  2012-01-30  6:38   ` Michael Haggerty
  0 siblings, 2 replies; 34+ messages in thread
From: Junio C Hamano @ 2012-01-29  6:56 UTC (permalink / raw
  To: Michael Haggerty; +Cc: git

Michael Haggerty <mhagger@alum.mit.edu> writes:

> When starting a new repo, git seems to insist that the first commit be
> made on a branch named "master":
>
>     $ git --version
>     git version 1.7.9
>     $ git init git-test
>     Initialized empty Git repository in /home/mhagger/tmp/git-test/.git/
>     $ cd git-test
>     $ git checkout -b foo
>     fatal: You are on a branch yet to be born
>
> I would call this a bug; the last command should be allowed.  The
> plumbing allows it:
>
>     $ git symbolic-ref HEAD refs/heads/foo

Your last sentence is nonsense.  The plumbing equivalent of that command
is *not* what you wrote above, but is more like [*1*]:

	git update-ref refs/heads/foo $(git rev-parse --verify HEAD) &&
        git symbolic-ref HEAD refs/heads/foo

And the first step will fail the same way.  While I share the sense of
annoyance with you, I do not think that it is a bug in "checkout -b".

When you are on an unborn branch, what the "symbolic-ref HEAD" command
reports does *not* appear in the output from the "for-each-ref refs/heads"
command (similarly, that branch name does not appear in the output from
the "git branch" command).

Such a behaviour indeed is *curious* and very *different* from the normal
case of being on an existing branch, but is that a bug?

You need to first admit that the state immediately after "git init" (or
for that matter, "checkout --orphan") where you are on an unborn branch
*is* special.  Some things that would normally make sense would not.


[Footnote]

*1* Because you are not switching to a different commit, there won't be a
need for the third step which is "git read-tree -m -u HEAD@{1} HEAD" that
would usually be necessary if you are giving a starting commit that is
different from HEAD.

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-01-29  6:56 ` Junio C Hamano
@ 2012-01-29  7:50   ` Junio C Hamano
  2012-01-30  6:38   ` Michael Haggerty
  1 sibling, 0 replies; 34+ messages in thread
From: Junio C Hamano @ 2012-01-29  7:50 UTC (permalink / raw
  To: Michael Haggerty; +Cc: git

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

> ...
> Such a behaviour indeed is *curious* and very *different* from the normal
> case of being on an existing branch, but is that a bug?
>
> You need to first admit that the state immediately after "git init" (or
> for that matter, "checkout --orphan") where you are on an unborn branch
> *is* special.  Some things that would normally make sense would not.

[sorry for having sent an incomplete message without conclusion]

The question then becomes this: what do you want to do about it with this
specific case, and more importantly what do you want to do about other
commands and options that would not make sense when HEAD knows what branch
the user wants to put the first commit on but there is no commit yet?

For some commands, we _do_ try to come up with a special case codepath so
that a command issued in the unborn state mimics the behaviour of the
command issued in the normal case to various different degrees. "git pull"
into an unborn branch simply resets to the other branch, for example, and
while technically speaking that is not merging the other branch into the
current commit (which does not exist), we do so because it was deemed to
be the most sensible behaviour to parallel the normal case.

I am not sure "git checkout -b foo" (without explict HEAD [*1*]) should
special case and degenerate to "symbolic-ref HEAD refs/heads/foo" when
HEAD points to a nonexistent branch.  The mimicking does not go far enough
to satisfy people who are pedantic enough to expect "git checkout -b foo"
to work when you haven't even instantiated your current branch (when you
are on an already instantiated branch, after "git checkout -b foo", "git
branch" output will show both foo and the branch you were on, but if you
start from an unborn branch, the behaviour will be different and a pedant
will notice the difference).

It may make sense to let

    $ git branch -m trunk

or even

    $ git branch -m master trunk

move away from an unborn "master'"after "git init", with a special case
codepath.  When you start from an instanticated branch, after a successful
such renaming, the original branch will not exist, and the new branch will
exist.  This property would also hold true if you start from an unborn one,
so it would be much better mimickery than "git checkout -b foo" case you
brought up in this thread.


[Footnote]

*1* Given HEAD or $(git symbolic-ref HEAD) as the starting commit
explicitly, the command should error out.

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-01-29  6:56 ` Junio C Hamano
  2012-01-29  7:50   ` Junio C Hamano
@ 2012-01-30  6:38   ` Michael Haggerty
  2012-01-30 18:48     ` Junio C Hamano
  1 sibling, 1 reply; 34+ messages in thread
From: Michael Haggerty @ 2012-01-30  6:38 UTC (permalink / raw
  To: Junio C Hamano; +Cc: git

On 01/29/2012 07:56 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> When starting a new repo, git seems to insist that the first commit be
>> made on a branch named "master":
>>
>>     $ git --version
>>     git version 1.7.9
>>     $ git init git-test
>>     Initialized empty Git repository in /home/mhagger/tmp/git-test/.git/
>>     $ cd git-test
>>     $ git checkout -b foo
>>     fatal: You are on a branch yet to be born
>>
>> I would call this a bug; the last command should be allowed.  The
>> plumbing allows it:
>>
>>     $ git symbolic-ref HEAD refs/heads/foo
> 
> Your last sentence is nonsense.  The plumbing equivalent of that command
> is *not* what you wrote above, but is more like [*1*]:
> 
> 	git update-ref refs/heads/foo $(git rev-parse --verify HEAD) &&
>         git symbolic-ref HEAD refs/heads/foo

All that I meant is that the one command is the equivalent of *what the
user wants and expects* in the *particular* situation that I described
[1].  I should have been clearer.

> And the first step will fail the same way.  While I share the sense of
> annoyance with you, I do not think that it is a bug in "checkout -b".
> 
> When you are on an unborn branch, what the "symbolic-ref HEAD" command
> reports does *not* appear in the output from the "for-each-ref refs/heads"
> command (similarly, that branch name does not appear in the output from
> the "git branch" command).
> 
> Such a behaviour indeed is *curious* and very *different* from the normal
> case of being on an existing branch, but is that a bug?

When git behaves differently than a typical user would expect for no
good reason, that is a bug (albeit a UI bug).  The fact that somebody
who knows the internals of git can find an excuse for the inconsistency
might be an explanation for how the bug arose but it doesn't make it
less of a bug.

> You need to first admit that the state immediately after "git init" (or
> for that matter, "checkout --orphan") where you are on an unborn branch
> *is* special.  Some things that would normally make sense would not.

ISTM that this state is more special than it needs to be due to an
design flaw of git [2].  But even given the fact that this case is
special *internal* to git, there is no reason to let that specialness
leak out to the user more than necessary.

> [...]
> I am not sure "git checkout -b foo" (without explict HEAD [*1*]) should
> special case and degenerate to "symbolic-ref HEAD refs/heads/foo" when
> HEAD points to a nonexistent branch.  The mimicking does not go far enough
> to satisfy people who are pedantic enough to expect "git checkout -b foo"
> to work when you haven't even instantiated your current branch (when you
> are on an already instantiated branch, after "git checkout -b foo", "git
> branch" output will show both foo and the branch you were on, but if you
> start from an unborn branch, the behaviour will be different and a pedant
> will notice the difference).

For me, "git checkout -b foo" means "leave the old branch in its current
state and move to a new branch that is in the same state."  If the old
branch was unborn, then it should remain unborn after the command, and I
should be moved to a new unborn branch.  Since an unborn branch in git
is not a branch, I would have no expectation that the old branch exists
after the command [3].

> It may make sense to let
> 
>     $ git branch -m trunk
> 
> or even
> 
>     $ git branch -m master trunk
> 
> move away from an unborn "master'"after "git init", with a special case
> codepath.  When you start from an instanticated branch, after a successful
> such renaming, the original branch will not exist, and the new branch will
> exist.  This property would also hold true if you start from an unborn one,
> so it would be much better mimickery than "git checkout -b foo" case you
> brought up in this thread.

It makes sense that "git branch -m" can *also* be used to escape an
unborn master, but this command won't necessarily occur to people
accustomed to using "git checkout -b" for creating new branches.

Michael

[1] Of course, here "the user" means me :-) but I predict that other
users would feel the same.

[2] Namely that "orphan" commits have no parents, instead of having an
"empty repository" commit (something like "000000*") as parent.  By
contrast, when a new Subversion repository is created, it automatically
gets a pseudo "r0" commit that represents the empty repository.  The r0
commit can be used in the UI most places that a "real" commit can be
used.  If the 0000000 commit could be used in the same way in git, it
would remove a lot of special casing.  For example, an "unborn branch"
could be initialized pointing at 0000000.  Even if there is some deeper
reason why such a design wouldn't have worked, perhaps such a concept
could be faked for the user interface.

[3] If commit 0000000 were treated specially, then there would be no
unborn branches but only branches pointing at the empty commit.  In that
case, my expectation would change--the old branch should be left
pointing at 0000000.  But currently git has no concept of an unborn
branch that is not HEAD.

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-01-30  6:38   ` Michael Haggerty
@ 2012-01-30 18:48     ` Junio C Hamano
  2012-01-30 20:10       ` Junio C Hamano
                         ` (2 more replies)
  0 siblings, 3 replies; 34+ messages in thread
From: Junio C Hamano @ 2012-01-30 18:48 UTC (permalink / raw
  To: Michael Haggerty; +Cc: git

Michael Haggerty <mhagger@alum.mit.edu> writes:

> [3] If commit 0000000 were treated specially, then there would be no
> unborn branches but only branches pointing at the empty commit.  In that
> case, my expectation would change--the old branch should be left
> pointing at 0000000.  But currently git has no concept of an unborn
> branch that is not HEAD.

And it probably is not a good thing to add such. Under that constraints,
HEAD that says refs/heads/foo where foo does not exist yet needs to be
special cased at places where it matters.

For that matter, even if we artificially created refs/heads/foo before any
commit is made and made it point at 0{40}, you would need to add special
cases to other parts of the system (e.g. "commit" needs to notice that the
result should be a root, not a child of 0{40}; "checkout other_branch"
needs to notice that it should refrain from running the equivalent of
"read-tree -m HEAD other_branch" because HEAD does not point at a real
tree; etc.), so it does not change the fact that the unborn branch is case
is special.

Note that I am not saying that we shouldn't add support for special cases
with special case codepaths.

Perhaps we would need to sprinkle more special case magic like this (this
is for the special case that arises from the same cause)?

 builtin/branch.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 7095718..0997e75 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -640,6 +640,13 @@ static int edit_branch_description(const char *branch_name)
 	struct strbuf buf = STRBUF_INIT;
 	struct strbuf name = STRBUF_INIT;
 
+	strbuf_addf(&name, "refs/heads/%s", branch_name);
+	if (!ref_exists(name.buf)) {
+		strbuf_reset(&name);
+		return error("No such branch '%s'.", branch_name);
+	}
+	strbuf_reset(&name);
+
 	read_branch_desc(&buf, branch_name);
 	if (!buf.len || buf.buf[buf.len-1] != '\n')
 		strbuf_addch(&buf, '\n');

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-01-30 18:48     ` Junio C Hamano
@ 2012-01-30 20:10       ` Junio C Hamano
  2012-01-30 21:50         ` Jeff King
  2012-01-30 21:48       ` Jeff King
  2012-01-31  8:57       ` Bug: "git checkout -b" should be allowed in empty repo Michael Haggerty
  2 siblings, 1 reply; 34+ messages in thread
From: Junio C Hamano @ 2012-01-30 20:10 UTC (permalink / raw
  To: Michael Haggerty; +Cc: git

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

> Note that I am not saying that we shouldn't add support for special cases
> with special case codepaths.
>
> Perhaps we would need to sprinkle more special case magic like this (this
> is for the special case that arises from the same cause)?
> ...

And the special case for "checkout -b" may look like this.

The early part of switch_branches() that computes old is probably be
better moved to the caller cmd_checkout() and used in the new code that
detects the "unborn" case, and passed as to switch_branches() as the third
parameter.  Such improvements, adding tests and pleasant commit log
message is left as an exercise for the interested and motivated, as usual
;-)



 builtin/checkout.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index f1984d9..195c40b 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -922,6 +922,19 @@ static int parse_branchname_arg(int argc, const char **argv,
 	return argcount;
 }
 
+static int switch_unborn_to_new_branch(struct checkout_opts *opts, const char *old_ref)
+{
+	int status;
+	struct strbuf branch_ref = STRBUF_INIT;
+
+	strbuf_addf(&branch_ref, "refs/heads/%s", opts->new_branch);
+	warning(_("Leaving the unborn branch '%s' behind..."),
+		skip_prefix(old_ref, "refs/heads/"));
+	status = create_symref("HEAD", branch_ref.buf, "checkout -b");
+	strbuf_reset(&branch_ref);
+	return status;
+}
+
 int cmd_checkout(int argc, const char **argv, const char *prefix)
 {
 	struct checkout_opts opts;
@@ -1093,5 +1106,16 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	if (opts.writeout_stage)
 		die(_("--ours/--theirs is incompatible with switching branches."));
 
+	if (!new.commit) {
+		unsigned char rev[20];
+		int flag, status;
+		char *old_ref = resolve_refdup("HEAD", rev, 0, &flag);
+
+		if ((flag & REF_ISSYMREF) && is_null_sha1(rev)) {
+			status = switch_unborn_to_new_branch(&opts, old_ref);
+			free(old_ref);
+			return status;
+		}
+	}
 	return switch_branches(&opts, &new);
 }

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-01-30 18:48     ` Junio C Hamano
  2012-01-30 20:10       ` Junio C Hamano
@ 2012-01-30 21:48       ` Jeff King
  2012-02-06  1:26         ` [PATCH] branch --edit-description: protect against mistyped branch name Junio C Hamano
  2012-01-31  8:57       ` Bug: "git checkout -b" should be allowed in empty repo Michael Haggerty
  2 siblings, 1 reply; 34+ messages in thread
From: Jeff King @ 2012-01-30 21:48 UTC (permalink / raw
  To: Junio C Hamano; +Cc: Michael Haggerty, git

On Mon, Jan 30, 2012 at 10:48:54AM -0800, Junio C Hamano wrote:

> Note that I am not saying that we shouldn't add support for special cases
> with special case codepaths.
> 
> Perhaps we would need to sprinkle more special case magic like this (this
> is for the special case that arises from the same cause)?

I like your patch better than trying to pass around "0{40}", but:

> diff --git a/builtin/branch.c b/builtin/branch.c
> index 7095718..0997e75 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -640,6 +640,13 @@ static int edit_branch_description(const char *branch_name)
>  	struct strbuf buf = STRBUF_INIT;
>  	struct strbuf name = STRBUF_INIT;
>  
> +	strbuf_addf(&name, "refs/heads/%s", branch_name);
> +	if (!ref_exists(name.buf)) {
> +		strbuf_reset(&name);
> +		return error("No such branch '%s'.", branch_name);
> +	}
> +	strbuf_reset(&name);
> +

I wonder if this conditional should have:

  unsigned char sha1[20];
  const char *head_points_at = resolve_ref_unsafe("HEAD", sha1, 1, NULL);
  if (!head_points_at || strcmp(head_points_at, name.buf))
          return error("No such branch '%s'.", branch_name);

to special-case unborn branches that we are actually pointing to.

IOW, the problem with the current code is that it allows typos and other
arbitrary bogus names to be silently described, even though doing so is
probably an error. But since this branch is already in use (even though
its ref does not technically exist yet), it's probably not an error.

As an aside, the strbuf_reset inside the conditional should be
strbuf_release, no? Otherwise we are leaking. And probably the one
outside, too. Even though we release the memory later, there are error
code-paths that do not. (And yes, I know this was a quick sketch and not
a real patch, but I wanted to point it out in case it turns into a real
one).

-Peff

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-01-30 20:10       ` Junio C Hamano
@ 2012-01-30 21:50         ` Jeff King
  2012-02-06  2:06           ` Junio C Hamano
  0 siblings, 1 reply; 34+ messages in thread
From: Jeff King @ 2012-01-30 21:50 UTC (permalink / raw
  To: Junio C Hamano; +Cc: Michael Haggerty, git

On Mon, Jan 30, 2012 at 12:10:08PM -0800, Junio C Hamano wrote:

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index f1984d9..195c40b 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -922,6 +922,19 @@ static int parse_branchname_arg(int argc, const char **argv,
>  	return argcount;
>  }
>  
> +static int switch_unborn_to_new_branch(struct checkout_opts *opts, const char *old_ref)
> +{
> +	int status;
> +	struct strbuf branch_ref = STRBUF_INIT;
> +
> +	strbuf_addf(&branch_ref, "refs/heads/%s", opts->new_branch);
> +	warning(_("Leaving the unborn branch '%s' behind..."),
> +		skip_prefix(old_ref, "refs/heads/"));
> +	status = create_symref("HEAD", branch_ref.buf, "checkout -b");
> +	strbuf_reset(&branch_ref);
> +	return status;
> +}

Is it really worth warning? After all, by definition you are not leaving
any commits or useful work behind.

Also, this has the same strbuf reset/release leak as the last patch. :)

-Peff

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-01-30 18:48     ` Junio C Hamano
  2012-01-30 20:10       ` Junio C Hamano
  2012-01-30 21:48       ` Jeff King
@ 2012-01-31  8:57       ` Michael Haggerty
  2012-01-31 10:01         ` Johannes Sixt
  2012-01-31 10:09         ` Jakub Narebski
  2 siblings, 2 replies; 34+ messages in thread
From: Michael Haggerty @ 2012-01-31  8:57 UTC (permalink / raw
  To: Junio C Hamano; +Cc: git

On 01/30/2012 07:48 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> [3] If commit 0000000 were treated specially, then there would be no
>> unborn branches but only branches pointing at the empty commit.  In that
>> case, my expectation would change--the old branch should be left
>> pointing at 0000000.  But currently git has no concept of an unborn
>> branch that is not HEAD.
> 
> And it probably is not a good thing to add such. Under that constraints,
> HEAD that says refs/heads/foo where foo does not exist yet needs to be
> special cased at places where it matters.
> 
> For that matter, even if we artificially created refs/heads/foo before any
> commit is made and made it point at 0{40}, you would need to add special
> cases to other parts of the system

No, the idea is to avoid special casing by making 0{40} into a real (but
empty) revision.

> (e.g. "commit" needs to notice that the
> result should be a root, not a child of 0{40};

No, commits that were previously generated as orphans *would* now be
generated as children of the special 0{40} commit.

> "checkout other_branch"
> needs to notice that it should refrain from running the equivalent of
> "read-tree -m HEAD other_branch" because HEAD does not point at a real
> tree;

No, it would merge the 0{40} commit with other_branch like usual,
resulting in the same contents as other_branch.  Indeed, if other_branch
is also ultimately a descendant of 0{40}, this would be like a
fast-forward merge.

> etc.

This "etc" might include problems.

> so it does not change the fact that the unborn branch is case
> is special.

On the contrary, I believe that much special casing could be eliminated
and the UI made more uniform by treating everything as a descendant of a
special "NULL" commit.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-01-31  8:57       ` Bug: "git checkout -b" should be allowed in empty repo Michael Haggerty
@ 2012-01-31 10:01         ` Johannes Sixt
  2012-01-31 10:11           ` demerphq
  2012-01-31 10:09         ` Jakub Narebski
  1 sibling, 1 reply; 34+ messages in thread
From: Johannes Sixt @ 2012-01-31 10:01 UTC (permalink / raw
  To: Michael Haggerty; +Cc: Junio C Hamano, git

Am 1/31/2012 9:57, schrieb Michael Haggerty:
> No, the idea is to avoid special casing by making 0{40} into a real (but
> empty) revision.

But then why not just have git init perform the equivalent of

  c=$(echo "Start" | git commit-tree $empty_tree_sha1) &&
  git update-ref refs/heads/master $c

People who dislike an empty initial commit can always use "git commit
--amend" for the first "real" commit.

-- Hannes

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-01-31  8:57       ` Bug: "git checkout -b" should be allowed in empty repo Michael Haggerty
  2012-01-31 10:01         ` Johannes Sixt
@ 2012-01-31 10:09         ` Jakub Narebski
  2012-01-31 16:32           ` Michael Haggerty
  1 sibling, 1 reply; 34+ messages in thread
From: Jakub Narebski @ 2012-01-31 10:09 UTC (permalink / raw
  To: Michael Haggerty; +Cc: Junio C Hamano, git

Michael Haggerty <mhagger@alum.mit.edu> writes:
> On 01/30/2012 07:48 PM, Junio C Hamano wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> 
>>> [3] If commit 0000000 were treated specially, then there would be no
>>> unborn branches but only branches pointing at the empty commit.  In that
>>> case, my expectation would change--the old branch should be left
>>> pointing at 0000000.  But currently git has no concept of an unborn
>>> branch that is not HEAD.
>> 
>> And it probably is not a good thing to add such. Under that constraints,
>> HEAD that says refs/heads/foo where foo does not exist yet needs to be
>> special cased at places where it matters.
>> 
>> For that matter, even if we artificially created refs/heads/foo before any
>> commit is made and made it point at 0{40}, you would need to add special
>> cases to other parts of the system
> 
> No, the idea is to avoid special casing by making 0{40} into a real (but
> empty) revision.
> 
>> (e.g. "commit" needs to notice that the
>> result should be a root, not a child of 0{40};
> 
> No, commits that were previously generated as orphans *would* now be
> generated as children of the special 0{40} commit.

You would still have to have quite a bit of special cases about 0{40}
NUL commit.  Perhaps less special cases, but new special cases.
 
[...]
>> so it does not change the fact that the unborn branch is case
>> is special.
> 
> On the contrary, I believe that much special casing could be eliminated
> and the UI made more uniform by treating everything as a descendant of a
> special "NULL" commit.

I don't see how this can be done in backward-compatibile way.

Please note that in Git it is quite natural to have more than one root
(parentless) commit, even without presence of disconnected / orphan
branches.  They are result of joining originally separate projects.
git.git has quite a few of them (more than 6, IIRC).

-- 
Jakub Narebski

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-01-31 10:01         ` Johannes Sixt
@ 2012-01-31 10:11           ` demerphq
  0 siblings, 0 replies; 34+ messages in thread
From: demerphq @ 2012-01-31 10:11 UTC (permalink / raw
  To: Johannes Sixt; +Cc: Michael Haggerty, Junio C Hamano, git

On 31 January 2012 11:01, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Am 1/31/2012 9:57, schrieb Michael Haggerty:
>> No, the idea is to avoid special casing by making 0{40} into a real (but
>> empty) revision.
>
> But then why not just have git init perform the equivalent of
>
>  c=$(echo "Start" | git commit-tree $empty_tree_sha1) &&
>  git update-ref refs/heads/master $c
>
> People who dislike an empty initial commit can always use "git commit
> --amend" for the first "real" commit.

Because it would then violate a system invariant that all commits are
descendants of the root commit.

You can model a git commit graph as a pathway through multidimensional
space of all possible commit trees. From that perspective it makes
sense that every pathway starts at the origin point of the
multidimensional space, which is conceptually the same as the proposed
root commit.

Anyway, I am not saying it should change, just that from some point of
views it makes a lot of sense.

Yves

-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-01-31 10:09         ` Jakub Narebski
@ 2012-01-31 16:32           ` Michael Haggerty
  0 siblings, 0 replies; 34+ messages in thread
From: Michael Haggerty @ 2012-01-31 16:32 UTC (permalink / raw
  To: Jakub Narebski; +Cc: Junio C Hamano, git, demerphq

On 01/31/2012 11:09 AM, Jakub Narebski wrote:
> I don't see how this can be done in backward-compatibile way.

Yes, backwards compatibility would probably prevent the NULL commit idea
from ever being implemented in a literal way.

But it is conceivable that it could be faked with some strategic

    if sha1 == '0'*40:
        treat_as_special_null_commit
    elif len(parents) == 0:
        parents = ['0'*40]

In other words, include a little special case fakery in the data
structures near root commits (an O(1) amount of work) to avoid special
cases in all commands that can touch root commits (an O(number of
commands) amount of work).

Alternatively, the NULL commit could be a UI construct that has no
manifestation in the object model.  This would not save implementation
work, but would perhaps give a more consistent way to deal with root
commits in the UI than the current array of --orphan etc. options.

> Please note that in Git it is quite natural to have more than one root
> (parentless) commit, even without presence of disconnected / orphan
> branches.  They are result of joining originally separate projects.
> git.git has quite a few of them (more than 6, IIRC).

I don't see the problem, unless you mean that it would be difficult to
merge repositories that don't link back to a NULL commit with
hypothetical future repositories that do include a NULL commit.  But a
world in which two kinds of repositories have to be supported is
pointless anyway, because then the git code would have to include *both*
kinds of special cases and nothing would be gained.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

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

* [PATCH] branch --edit-description: protect against mistyped branch name
  2012-01-30 21:48       ` Jeff King
@ 2012-02-06  1:26         ` Junio C Hamano
  2012-02-06  1:27           ` Junio C Hamano
  2012-02-06  4:20           ` Jeff King
  0 siblings, 2 replies; 34+ messages in thread
From: Junio C Hamano @ 2012-02-06  1:26 UTC (permalink / raw
  To: git; +Cc: Michael Haggerty, Jeff King

It is very easy to mistype the branch name when editing its description,
e.g.

	$ git checkout -b my-topic master
	: work work work
	: now we are at a good point to switch working something else
	$ git checkout master
	: ah, let's write it down before we forget what we were doing
	$ git branch --edit-description my-tpoic

The command does not notice that branch 'my-tpoic' does not exist.  It is
not lost (it becomes description of an unborn my-tpoic branch), but is not
very useful.  So detect such a case and error out to reduce the grief
factor from this common mistake.

This incidentally also errors out --edit-description when the HEAD points
at an unborn branch (immediately after "init", or "checkout --orphan"),
because at that point, you do not even have any commit that is part of
your history and there is no point in describing how this particular
branch is different from the branch it forked off of, which is the useful
bit of information the branch description is designed to capture.

We may want to special case the unborn case later, but that is outside the
scope of this patch to prevent more common mistakes before 1.7.9 series
gains too much widespread use.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

  Jeff King <peff@peff.net> writes:

  > IOW, the problem with the current code is that it allows typos and other
  > arbitrary bogus names to be silently described, even though doing so is
  > probably an error...

 builtin/branch.c  |   15 +++++++++++++++
 t/t3200-branch.sh |   41 +++++++++++++++++++++++++++++++++++++----
 2 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 7095718..0c1784f 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -768,6 +768,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 				      with_commit, argv);
 	else if (edit_description) {
 		const char *branch_name;
+		struct strbuf branch_ref = STRBUF_INIT;
+
 		if (detached)
 			die("Cannot give description to detached HEAD");
 		if (!argc)
@@ -776,6 +778,19 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			branch_name = argv[0];
 		else
 			usage_with_options(builtin_branch_usage, options);
+
+		strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
+		if (!ref_exists(branch_ref.buf)) {
+			strbuf_reset(&branch_ref);
+
+			if (!argc)
+				return error("No commit on branch '%s' yet.",
+					     branch_name);
+			else
+				return error("No such branch '%s'.", branch_name);
+		}
+		strbuf_reset(&branch_ref);
+
 		if (edit_branch_description(branch_name))
 			return 1;
 	} else if (rename) {
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index ea82424..dd1aceb 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -3,11 +3,8 @@
 # Copyright (c) 2005 Amos Waterland
 #
 
-test_description='git branch --foo should not create bogus branch
+test_description='git branch assorted tests'
 
-This test runs git branch --help and checks that the argument is properly
-handled.  Specifically, that a bogus branch is not created.
-'
 . ./test-lib.sh
 
 test_expect_success \
@@ -620,4 +617,40 @@ test_expect_success 'use set-upstream on the current branch' '
 
 '
 
+test_expect_success 'use --edit-description' '
+	write_script editor <<-\EOF &&
+		echo "New contents" >"$1"
+	EOF
+	EDITOR=./editor git branch --edit-description &&
+		write_script editor <<-\EOF &&
+		git stripspace -s <"$1" >"EDITOR_OUTPUT"
+	EOF
+	EDITOR=./editor git branch --edit-description &&
+	echo "New contents" >expect &&
+	test_cmp EDITOR_OUTPUT expect
+'
+
+test_expect_success 'detect typo in branch name when using --edit-description' '
+	write_script editor <<-\EOF &&
+		echo "New contents" >"$1"
+	EOF
+	(
+		EDITOR=./editor &&
+		export EDITOR &&
+		test_must_fail git branch --edit-description no-such-branch
+	)
+'
+
+test_expect_success 'refuse --edit-description on unborn branch for now' '
+	write_script editor <<-\EOF &&
+		echo "New contents" >"$1"
+	EOF
+	git checkout --orphan unborn &&
+	(
+		EDITOR=./editor &&
+		export EDITOR &&
+		test_must_fail git branch --edit-description
+	)
+'
+
 test_done
-- 
1.7.9.172.ge26ae

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

* Re: [PATCH] branch --edit-description: protect against mistyped branch name
  2012-02-06  1:26         ` [PATCH] branch --edit-description: protect against mistyped branch name Junio C Hamano
@ 2012-02-06  1:27           ` Junio C Hamano
  2012-02-06  4:20           ` Jeff King
  1 sibling, 0 replies; 34+ messages in thread
From: Junio C Hamano @ 2012-02-06  1:27 UTC (permalink / raw
  To: Junio C Hamano; +Cc: git, Michael Haggerty, Jeff King

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

> +		if (!ref_exists(branch_ref.buf)) {
> +			strbuf_reset(&branch_ref);
> +
> +			if (!argc)
> +				return error("No commit on branch '%s' yet.",
> +					     branch_name);
> +			else
> +				return error("No such branch '%s'.", branch_name);
> +		}
> +		strbuf_reset(&branch_ref);
> +

Of course these should be strbuf_release().

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-01-30 21:50         ` Jeff King
@ 2012-02-06  2:06           ` Junio C Hamano
  2012-02-06  2:08             ` Junio C Hamano
  2012-02-06  4:30             ` Jeff King
  0 siblings, 2 replies; 34+ messages in thread
From: Junio C Hamano @ 2012-02-06  2:06 UTC (permalink / raw
  To: git; +Cc: Michael Haggerty, Jeff King

Jeff King <peff@peff.net> writes:

> Is it really worth warning? After all, by definition you are not leaving
> any commits or useful work behind.

I actually do not know if this change itself is worth doing, but if we
were to do this, then I think the user benefits from the warning.

The patch is made on maint-1.7.6 track for no good reason, so it may have
some merge conflicts around "resolve_ref()" vs "resolve_refdup()" if we
were to apply it on a more modern codebase, but the resolution should be
trivial.

---
Subject: [PATCH] git checkout -b: allow switching out of an unborn branch

Running "git checkout -b another" immediately after "git init" when you do
not even have a commit on 'master' is forbidden, with a readable message:

    $ git checkout -b another
    fatal: You are on a branch yet to be born

It is readable but not easily understandable unless the user knows what
"yet to be born" really means.

So let's try allowing it and see what happens. I strongly suspect that
this may just shift the confusion one step further without adding much
value to the resulting system, because the next question that would come
to somebody who does not understand what "yet to be born" is is "why don't
I see 'master' in the output from 'git branch' command?", and the new
warning may not be descriptive enough to explain what the user is doing.

The early part of switch_branches() that computes old is probably be
better moved to the caller cmd_checkout() and used in the new code that
detects the "unborn" case, and passed as to switch_branches() as the third
parameter.  Such improvements and tests are left as an exercise for the
interested and motivated, as usual ;-)

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/checkout.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 4c20dae..5894f40 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -916,6 +916,19 @@ static int parse_branchname_arg(int argc, const char **argv,
 	return argcount;
 }
 
+static int switch_unborn_to_new_branch(struct checkout_opts *opts, const char *old_ref)
+{
+	int status;
+	struct strbuf branch_ref = STRBUF_INIT;
+
+	strbuf_addf(&branch_ref, "refs/heads/%s", opts->new_branch);
+	warning(_("Leaving the unborn branch '%s' behind..."),
+		skip_prefix(old_ref, "refs/heads/"));
+	status = create_symref("HEAD", branch_ref.buf, "checkout -b");
+	strbuf_release(&branch_ref);
+	return status;
+}
+
 int cmd_checkout(int argc, const char **argv, const char *prefix)
 {
 	struct checkout_opts opts;
@@ -1089,5 +1102,16 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	if (opts.writeout_stage)
 		die(_("--ours/--theirs is incompatible with switching branches."));
 
+	if (!new.commit) {
+		unsigned char rev[20];
+		int flag, status;
+		const char *old_ref = resolve_ref("HEAD", rev, 0, &flag);
+
+		if ((flag & REF_ISSYMREF) && is_null_sha1(rev)) {
+			status = switch_unborn_to_new_branch(&opts, old_ref);
+			free((char *)old_ref);
+			return status;
+		}
+	}
 	return switch_branches(&opts, &new);
 }
-- 
1.7.9.172.ge26ae

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-02-06  2:06           ` Junio C Hamano
@ 2012-02-06  2:08             ` Junio C Hamano
  2012-02-06  4:30             ` Jeff King
  1 sibling, 0 replies; 34+ messages in thread
From: Junio C Hamano @ 2012-02-06  2:08 UTC (permalink / raw
  To: git; +Cc: Michael Haggerty, Jeff King

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

> The patch is made on maint-1.7.6 track for no good reason, so it may have
> some merge conflicts around "resolve_ref()" vs "resolve_refdup()" if we
> were to apply it on a more modern codebase, but the resolution should be
> trivial.

The resolution should look like this, just in case.

diff --cc builtin/checkout.c
index 5bf96ba,5894f40..41b9b34
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@@ -1079,5 -1102,16 +1092,16 @@@ int cmd_checkout(int argc, const char *
  	if (opts.writeout_stage)
  		die(_("--ours/--theirs is incompatible with switching branches."));
  
+ 	if (!new.commit) {
+ 		unsigned char rev[20];
+ 		int flag, status;
 -		const char *old_ref = resolve_ref("HEAD", rev, 0, &flag);
++		char *old_ref = resolve_refdup("HEAD", rev, 0, &flag);
+ 
+ 		if ((flag & REF_ISSYMREF) && is_null_sha1(rev)) {
+ 			status = switch_unborn_to_new_branch(&opts, old_ref);
 -			free((char *)old_ref);
++			free(old_ref);
+ 			return status;
+ 		}
+ 	}
  	return switch_branches(&opts, &new);
  }

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

* Re: [PATCH] branch --edit-description: protect against mistyped branch name
  2012-02-06  1:26         ` [PATCH] branch --edit-description: protect against mistyped branch name Junio C Hamano
  2012-02-06  1:27           ` Junio C Hamano
@ 2012-02-06  4:20           ` Jeff King
  1 sibling, 0 replies; 34+ messages in thread
From: Jeff King @ 2012-02-06  4:20 UTC (permalink / raw
  To: Junio C Hamano; +Cc: git, Michael Haggerty

On Sun, Feb 05, 2012 at 05:26:31PM -0800, Junio C Hamano wrote:

> This incidentally also errors out --edit-description when the HEAD points
> at an unborn branch (immediately after "init", or "checkout --orphan"),
> because at that point, you do not even have any commit that is part of
> your history and there is no point in describing how this particular
> branch is different from the branch it forked off of, which is the useful
> bit of information the branch description is designed to capture.
> 
> We may want to special case the unborn case later, but that is outside the
> scope of this patch to prevent more common mistakes before 1.7.9 series
> gains too much widespread use.

That sounds OK to me. I'm not even sure people will want to use
"--edit-description" on an unborn pointed-to branch or not (I mentioned
it only as "this is a plausible use case to me that we might be
breaking"). I think people will still be figuring out workflows around
it. So it's not a big deal to wait and see.

-Peff

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-02-06  2:06           ` Junio C Hamano
  2012-02-06  2:08             ` Junio C Hamano
@ 2012-02-06  4:30             ` Jeff King
  2012-02-06  4:42               ` Andrew Ardill
  2012-02-06  5:15               ` Junio C Hamano
  1 sibling, 2 replies; 34+ messages in thread
From: Jeff King @ 2012-02-06  4:30 UTC (permalink / raw
  To: Junio C Hamano; +Cc: git, Michael Haggerty

On Sun, Feb 05, 2012 at 06:06:53PM -0800, Junio C Hamano wrote:

> Subject: [PATCH] git checkout -b: allow switching out of an unborn branch
> 
> Running "git checkout -b another" immediately after "git init" when you do
> not even have a commit on 'master' is forbidden, with a readable message:
> 
>     $ git checkout -b another
>     fatal: You are on a branch yet to be born
> 
> It is readable but not easily understandable unless the user knows what
> "yet to be born" really means.
> 
> So let's try allowing it and see what happens. I strongly suspect that
> this may just shift the confusion one step further without adding much
> value to the resulting system, because the next question that would come
> to somebody who does not understand what "yet to be born" is is "why don't
> I see 'master' in the output from 'git branch' command?", and the new
> warning may not be descriptive enough to explain what the user is doing.

I thought the concern wasn't confusion at the error message, but rather
"how do I start a new repository with a branch named something besides
'master'?"

You would expect:

  git init
  git checkout -b foo

to work, but it doesn't. And there's no easy way to do what you want
(you have to resort to plumbing to put the value in HEAD). So the issue
is not a bad error message or a confusing situation, but that the user
wants to accomplish X, and we don't provide a reasonable way to do it.

I suspect it hasn't come up so far because the "X" in this case is not
something people generally want to do. I.e., they are almost always
cloning and making a new branch from old history. If they have a
brand-new repo, they almost certainly don't actually care what the
branch is called.

And perhaps in that case we should be discouraging them from calling it
something besides master (because while master is mostly convention,
there are a few magic spots in the code where it is treated specially,
and departing from the convention for no good reason should be
discouraged).

So I don't see "this is just shifting confusion" as a real argument. But
you could argue that it is enabling the user to do something stupid and
pointless, and for that reason it should be disallowed (and in that
case, it might be better for the error to say "what you are doing is
stupid and pointless").

-Peff

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-02-06  4:30             ` Jeff King
@ 2012-02-06  4:42               ` Andrew Ardill
  2012-02-06  5:06                 ` Jeff King
  2012-02-06 18:39                 ` demerphq
  2012-02-06  5:15               ` Junio C Hamano
  1 sibling, 2 replies; 34+ messages in thread
From: Andrew Ardill @ 2012-02-06  4:42 UTC (permalink / raw
  To: Jeff King; +Cc: Junio C Hamano, git, Michael Haggerty

On 6 February 2012 15:30, Jeff King <peff@peff.net> wrote:

> And perhaps in that case we should be discouraging them from calling it
> something besides master (because while master is mostly convention,
> there are a few magic spots in the code where it is treated specially,
> and departing from the convention for no good reason should be
> discouraged).

What exactly are the areas where 'master' is treated specially? I
agree that people should be discouraged from needlessly abandoning
convention, however I think users should have the ability to name
their branches as they see fit.

If I am forced to abandon code targeted at the 'master' naming
convention in order to use my desired naming convention, we should fix
that. Additionally, if I have to either manually set a branch name
with plumbing commands, or delete existing branches that are generated
automatically with no option not to generate them, we should improve
the porcelain to cover these cracks.

In general, I think it plausible that in some use cases the term
'master' might be misleading or inappropriate and users should not be
punished for that.

Regards,

Andrew Ardill

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-02-06  4:42               ` Andrew Ardill
@ 2012-02-06  5:06                 ` Jeff King
  2012-02-06  8:51                   ` Michael Haggerty
  2012-02-06 18:39                 ` demerphq
  1 sibling, 1 reply; 34+ messages in thread
From: Jeff King @ 2012-02-06  5:06 UTC (permalink / raw
  To: Andrew Ardill; +Cc: Junio C Hamano, git, Michael Haggerty

On Mon, Feb 06, 2012 at 03:42:24PM +1100, Andrew Ardill wrote:

> On 6 February 2012 15:30, Jeff King <peff@peff.net> wrote:
> 
> > And perhaps in that case we should be discouraging them from calling it
> > something besides master (because while master is mostly convention,
> > there are a few magic spots in the code where it is treated specially,
> > and departing from the convention for no good reason should be
> > discouraged).
> 
> What exactly are the areas where 'master' is treated specially? I
> agree that people should be discouraged from needlessly abandoning
> convention, however I think users should have the ability to name
> their branches as they see fit.

Fairly minor stuff. Between the top of my head and a quick grep:

  1. Some transports (like git://) are incapable of communicating the
     destination of a remote's symbolic ref (they see only that HEAD
     points to some specific sha1). But things like "clone" want to know
     where the HEAD is pointing to set up the remotes/$origin/HEAD
     link. We can guess that if HEAD and some branch have the same sha1,
     that HEAD is pointing to that branch. But you might have two or
     more such branches pointing to the same spot. In this case, we
     prefer "master" over other branches.

     This code is in guess_remote_head.

  2. When merging, if the current branch is named "master", the default
     merge message says "Merge branch foo". Otherwise, it says "Merge
     branch foo into bar".

  3. It looks like the antique "branches" file format defaults to
     fetching "master" when no branch specifier is given. I doubt anyone
     is still using this file format these days.

I was actually surprised how infrequently the term "master" comes up in
a grep of the code. So while I wouldn't call my search exhaustive, I did
inspect every match in the C code.

> If I am forced to abandon code targeted at the 'master' naming
> convention in order to use my desired naming convention, we should fix
> that. Additionally, if I have to either manually set a branch name
> with plumbing commands, or delete existing branches that are generated
> automatically with no option not to generate them, we should improve
> the porcelain to cover these cracks.
> 
> In general, I think it plausible that in some use cases the term
> 'master' might be misleading or inappropriate and users should not be
> punished for that.

I kind of agree that we shouldn't be unnecessarily restrictive. On the
other hand, I am stretching to find the plausible reason that one would
want to throw away the normal convention. Code aside, it simply
introduces a slight communication barrier when talking with other git
users, and for that reason should be something you don't do lightly. I
don't recall seeing anybody complain seriously about it in the past six
years of git's existence.

-Peff

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-02-06  4:30             ` Jeff King
  2012-02-06  4:42               ` Andrew Ardill
@ 2012-02-06  5:15               ` Junio C Hamano
  2012-02-06  5:18                 ` Jeff King
  1 sibling, 1 reply; 34+ messages in thread
From: Junio C Hamano @ 2012-02-06  5:15 UTC (permalink / raw
  To: Jeff King; +Cc: git, Michael Haggerty

Jeff King <peff@peff.net> writes:

> I thought the concern wasn't confusion at the error message, but rather
> "how do I start a new repository with a branch named something besides
> 'master'?"
>
> You would expect:
>
>   git init
>   git checkout -b foo
>
> to work, but it doesn't. And there's no easy way to do what you want
> (you have to resort to plumbing to put the value in HEAD). So the issue
> is not a bad error message or a confusing situation, but that the user
> wants to accomplish X, and we don't provide a reasonable way to do it.

I think the right interface for "I want to use 'foo' instead of 'master'
like everybody else" would be:

	$ git init --some-option foo

I wouldn't have any issue with that.

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-02-06  5:15               ` Junio C Hamano
@ 2012-02-06  5:18                 ` Jeff King
  2012-02-06  5:30                   ` Junio C Hamano
  0 siblings, 1 reply; 34+ messages in thread
From: Jeff King @ 2012-02-06  5:18 UTC (permalink / raw
  To: Junio C Hamano; +Cc: git, Michael Haggerty

On Sun, Feb 05, 2012 at 09:15:34PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I thought the concern wasn't confusion at the error message, but rather
> > "how do I start a new repository with a branch named something besides
> > 'master'?"
> >
> > You would expect:
> >
> >   git init
> >   git checkout -b foo
> >
> > to work, but it doesn't. And there's no easy way to do what you want
> > (you have to resort to plumbing to put the value in HEAD). So the issue
> > is not a bad error message or a confusing situation, but that the user
> > wants to accomplish X, and we don't provide a reasonable way to do it.
> 
> I think the right interface for "I want to use 'foo' instead of 'master'
> like everybody else" would be:
> 
> 	$ git init --some-option foo
> 
> I wouldn't have any issue with that.

Sure, that's one way to do it. But I don't see any point in not allowing
"git checkout -b" to be another way of doing it. Is there some other use
case for "git checkout -b" from an unborn branch? Or is there some
harmful outcome that can come from doing so that we need to be
protecting against? Am I missing something?

-Peff

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-02-06  5:18                 ` Jeff King
@ 2012-02-06  5:30                   ` Junio C Hamano
  2012-02-06  5:34                     ` Junio C Hamano
                                       ` (2 more replies)
  0 siblings, 3 replies; 34+ messages in thread
From: Junio C Hamano @ 2012-02-06  5:30 UTC (permalink / raw
  To: Jeff King; +Cc: git, Michael Haggerty

Jeff King <peff@peff.net> writes:

> Sure, that's one way to do it. But I don't see any point in not allowing
> "git checkout -b" to be another way of doing it. Is there some other use
> case for "git checkout -b" from an unborn branch? Or is there some
> harmful outcome that can come from doing so that we need to be
> protecting against? Am I missing something?

Mostly because it is wrong at the conceptual level to do so.

	git checkout -b foo

is a short-hand for

	git checkout -b foo HEAD

which is a short-hand for

	git branch foo HEAD &&
        git checkout foo

But the last one has no chance of working if you think about it, because
"git branch foo $start" is a way to start a branch at $start and you need
to have something to point at with refs/heads/foo.

So we are breaking the equivalence between these three only when HEAD
points at an unborn branch.

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-02-06  5:30                   ` Junio C Hamano
@ 2012-02-06  5:34                     ` Junio C Hamano
  2012-02-06  5:45                     ` Jeff King
  2012-02-06  8:59                     ` Michael Haggerty
  2 siblings, 0 replies; 34+ messages in thread
From: Junio C Hamano @ 2012-02-06  5:34 UTC (permalink / raw
  To: Junio C Hamano; +Cc: Jeff King, git, Michael Haggerty

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

> But the last one has no chance of working if you think about it, because
> "git branch foo $start" is a way to start a branch at $start and you need
> to have something to point at with refs/heads/foo.

... which brings us back to your earlier point ...

>> I like your patch better than trying to pass around "0{40}", but:

which is why my conclusion was that "checkout -b" is shifting the
confusion around to different parts.

> So we are breaking the equivalence between these three only when HEAD
> points at an unborn branch.

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-02-06  5:30                   ` Junio C Hamano
  2012-02-06  5:34                     ` Junio C Hamano
@ 2012-02-06  5:45                     ` Jeff King
  2012-02-06  8:59                     ` Michael Haggerty
  2 siblings, 0 replies; 34+ messages in thread
From: Jeff King @ 2012-02-06  5:45 UTC (permalink / raw
  To: Junio C Hamano; +Cc: git, Michael Haggerty

On Sun, Feb 05, 2012 at 09:30:15PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Sure, that's one way to do it. But I don't see any point in not allowing
> > "git checkout -b" to be another way of doing it. Is there some other use
> > case for "git checkout -b" from an unborn branch? Or is there some
> > harmful outcome that can come from doing so that we need to be
> > protecting against? Am I missing something?
> 
> Mostly because it is wrong at the conceptual level to do so.
> 
> 	git checkout -b foo
> 
> is a short-hand for
> 
> 	git checkout -b foo HEAD
> 
> which is a short-hand for
> 
> 	git branch foo HEAD &&
>         git checkout foo
> 
> But the last one has no chance of working if you think about it, because
> "git branch foo $start" is a way to start a branch at $start and you need
> to have something to point at with refs/heads/foo.
> 
> So we are breaking the equivalence between these three only when HEAD
> points at an unborn branch.

I think it is only wrong at the conceptual level because you have
specified the concepts in such a way that it is so. That is how git does
it _now_, but the whole point of this is to change git's behavior to
handle a potentially useful special case. I could also say this: "git
checkout -b foo HEAD" does two things:

  1. create a new branch "foo" pointing to the current sha1 of HEAD

  2. point the HEAD symref at refs/heads/foo

And then the proposed behavior might amend the first point to say:

  1. if HEAD points to an existing ref, then create a new branch...

which is perfectly consistent and simple. It does violate your "X is a
short-hand for Y" above, but why is that a bad thing? It seems you are
arguing against a special case _because_ it is a special case, not
because it is not a reasonable thing to do or expect.

Anyway. I am still not convinced that this is even a useful thing to
want to do, so I am certainly not volunteering to write such a patch. So
perhaps there is no point arguing about it.

-Peff

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-02-06  5:06                 ` Jeff King
@ 2012-02-06  8:51                   ` Michael Haggerty
  2012-02-06  8:57                     ` Jeff King
  0 siblings, 1 reply; 34+ messages in thread
From: Michael Haggerty @ 2012-02-06  8:51 UTC (permalink / raw
  To: Jeff King; +Cc: Andrew Ardill, Junio C Hamano, git

On 02/06/2012 06:06 AM, Jeff King wrote:
> I kind of agree that we shouldn't be unnecessarily restrictive. On the
> other hand, I am stretching to find the plausible reason that one would
> want to throw away the normal convention. Code aside, it simply
> introduces a slight communication barrier when talking with other git
> users, and for that reason should be something you don't do lightly. I
> don't recall seeing anybody complain seriously about it in the past six
> years of git's existence.

In the real-world situation when I noticed this bug, I wasn't trying to
use a nonstandard name for "master".  What I was doing is importing a
snapshot of some code from another non-git project onto a "vendor
branch", which I knew I would later want to merge into my own work
(which I planned to do on master).

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-02-06  8:51                   ` Michael Haggerty
@ 2012-02-06  8:57                     ` Jeff King
  2012-02-06 18:17                       ` Junio C Hamano
  0 siblings, 1 reply; 34+ messages in thread
From: Jeff King @ 2012-02-06  8:57 UTC (permalink / raw
  To: Michael Haggerty; +Cc: Andrew Ardill, Junio C Hamano, git

On Mon, Feb 06, 2012 at 09:51:56AM +0100, Michael Haggerty wrote:

> On 02/06/2012 06:06 AM, Jeff King wrote:
> >I don't recall seeing anybody complain seriously about it in the past
> >six years of git's existence.
> 
> In the real-world situation when I noticed this bug, I wasn't trying to
> use a nonstandard name for "master".  What I was doing is importing a
> snapshot of some code from another non-git project onto a "vendor
> branch", which I knew I would later want to merge into my own work
> (which I planned to do on master).

Thanks, that sounds like a very reasonable use case. I stand corrected.

(For some reason I thought you ran across it accidentally while mucking
with "--edit-description", since were talking about that an unborn
branches in a nearby thread).

-Peff

PS I probably would have done it as:

     git init vendor
     cd vendor
     import import import
     cd ..

     git init project
     cd project
     git fetch ../vendor master:vendor

   but I don't think there's anything wrong with your approach (in fact,
   it's slightly more efficient).

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-02-06  5:30                   ` Junio C Hamano
  2012-02-06  5:34                     ` Junio C Hamano
  2012-02-06  5:45                     ` Jeff King
@ 2012-02-06  8:59                     ` Michael Haggerty
  2012-02-06 18:31                       ` Junio C Hamano
  2 siblings, 1 reply; 34+ messages in thread
From: Michael Haggerty @ 2012-02-06  8:59 UTC (permalink / raw
  To: Junio C Hamano; +Cc: Jeff King, git

On 02/06/2012 06:30 AM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
>> Sure, that's one way to do it. But I don't see any point in not allowing
>> "git checkout -b" to be another way of doing it. Is there some other use
>> case for "git checkout -b" from an unborn branch? Or is there some
>> harmful outcome that can come from doing so that we need to be
>> protecting against? Am I missing something?
> 
> Mostly because it is wrong at the conceptual level to do so.
> 
> 	git checkout -b foo
> 
> is a short-hand for
> 
> 	git checkout -b foo HEAD
> 
> which is a short-hand for
> 
> 	git branch foo HEAD &&
>         git checkout foo
> 
> But the last one has no chance of working if you think about it, because
> "git branch foo $start" is a way to start a branch at $start and you need
> to have something to point at with refs/heads/foo.
> 
> So we are breaking the equivalence between these three only when HEAD
> points at an unborn branch.

You are thinking too much like a developer and not like a user.  For a user,

    git checkout -b foo

is a short-hand for

    "create and check out a branch at my current state"

and the interpretation of what that means when I am on an unborn branch
seems unambiguous.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-02-06  8:57                     ` Jeff King
@ 2012-02-06 18:17                       ` Junio C Hamano
  2012-02-06 20:14                         ` Jeff King
  2012-02-07  8:04                         ` Michael Haggerty
  0 siblings, 2 replies; 34+ messages in thread
From: Junio C Hamano @ 2012-02-06 18:17 UTC (permalink / raw
  To: Jeff King; +Cc: Michael Haggerty, Andrew Ardill, git

Jeff King <peff@peff.net> writes:

> PS I probably would have done it as:
>
>      git init vendor
>      cd vendor
>      import import import
>      cd ..
>
>      git init project
>      cd project
>      git fetch ../vendor master:vendor
>
>    but I don't think there's anything wrong with your approach (in fact,
>    it's slightly more efficient).

Probably I am slower than my usual slow self this morning. Does Michael's
approach go like this:

	git init project
        cd project
        import import import
        git branch -m vendor
        git checkout -b master

to fork from third-party codebase?

Or Michael had his own 'master' already and wanted an independent orphaned
history from vendor, perhaps like this?

	git init project
        cd project
        work work work
        git checkout --orphan vendor
        : perhaps "git clean -f" here???
        import import import
        git checkout master
        : rootless merge???
        git merge vendor

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-02-06  8:59                     ` Michael Haggerty
@ 2012-02-06 18:31                       ` Junio C Hamano
  0 siblings, 0 replies; 34+ messages in thread
From: Junio C Hamano @ 2012-02-06 18:31 UTC (permalink / raw
  To: Michael Haggerty; +Cc: Jeff King, git

Michael Haggerty <mhagger@alum.mit.edu> writes:

>> So we are breaking the equivalence between these three only when HEAD
>> points at an unborn branch.
>
> You are thinking too much like a developer and not like a user.  For a user,
>
>     git checkout -b foo
>
> is a short-hand for
>
>     "create and check out a branch at my current state"
>
> and the interpretation of what that means when I am on an unborn branch
> seems unambiguous.

Ok, that is a very good explanation.

Care to come up with a patch to Documentation/git-checkout.txt?  The
description there strongly implies that <start point> is an existing
commit.  Not much is said about what the lack of <start point> mean when
it describes "checkout -b", and a standalone description of <start point>
says "The name of a comit at which to start... Defaults to HEAD".  These
need to be loosened and described in terms of the closer-to-the-user "at
my current state".

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-02-06  4:42               ` Andrew Ardill
  2012-02-06  5:06                 ` Jeff King
@ 2012-02-06 18:39                 ` demerphq
  1 sibling, 0 replies; 34+ messages in thread
From: demerphq @ 2012-02-06 18:39 UTC (permalink / raw
  To: Andrew Ardill; +Cc: Jeff King, Junio C Hamano, git, Michael Haggerty

On 6 February 2012 05:42, Andrew Ardill <andrew.ardill@gmail.com> wrote:
> On 6 February 2012 15:30, Jeff King <peff@peff.net> wrote:
>
>> And perhaps in that case we should be discouraging them from calling it
>> something besides master (because while master is mostly convention,
>> there are a few magic spots in the code where it is treated specially,
>> and departing from the convention for no good reason should be
>> discouraged).
>
> What exactly are the areas where 'master' is treated specially? I
> agree that people should be discouraged from needlessly abandoning
> convention, however I think users should have the ability to name
> their branches as they see fit.

FWIW, we at $work have used a repo without a master branch at all
since the very beginning and never noticed a problem with it.

Although we *did* rename the original master to "trunk".

We did this because we felt that in a scenario where there is a
designated central "master repo" that the use of "master branch" would
get really confusing, so we have a master repo, whos main branch is
"trunk".

Yves

-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-02-06 18:17                       ` Junio C Hamano
@ 2012-02-06 20:14                         ` Jeff King
  2012-02-07  8:04                         ` Michael Haggerty
  1 sibling, 0 replies; 34+ messages in thread
From: Jeff King @ 2012-02-06 20:14 UTC (permalink / raw
  To: Junio C Hamano; +Cc: Michael Haggerty, Andrew Ardill, git

On Mon, Feb 06, 2012 at 10:17:55AM -0800, Junio C Hamano wrote:

> > PS I probably would have done it as:
> >
> >      git init vendor
> >      cd vendor
> >      import import import
> >      cd ..
> >
> >      git init project
> >      cd project
> >      git fetch ../vendor master:vendor
> >
> >    but I don't think there's anything wrong with your approach (in fact,
> >    it's slightly more efficient).
> 
> Probably I am slower than my usual slow self this morning. Does Michael's
> approach go like this:
> 
> 	git init project
>         cd project
>         import import import
>         git branch -m vendor
>         git checkout -b master
> 
> to fork from third-party codebase?

I thought it would have been:

  git init project
  cd project
  git checkout -b vendor
  import import import
  git checkout -b master
  modify modify modify

but he can probably say more.

-Peff

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

* Re: Bug: "git checkout -b" should be allowed in empty repo
  2012-02-06 18:17                       ` Junio C Hamano
  2012-02-06 20:14                         ` Jeff King
@ 2012-02-07  8:04                         ` Michael Haggerty
  1 sibling, 0 replies; 34+ messages in thread
From: Michael Haggerty @ 2012-02-07  8:04 UTC (permalink / raw
  To: Junio C Hamano; +Cc: Jeff King, Andrew Ardill, git

On 02/06/2012 07:17 PM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> Probably I am slower than my usual slow self this morning. Does Michael's
> approach go like this:
> 
> 	git init project
>         cd project
>         import import import
>         git branch -m vendor
>         git checkout -b master
> 
> to fork from third-party codebase?

I'm not really forking the third-party code; I'm just importing a
snapshot to a particular subdirectory of my own project.  I wanted to do
something like:

    git init project
    cd project
    git checkout -b vendor
    import import import commit (into subdirectory "foo")
    git checkout --orphan master
    git clean -fxd
    hack commit hack commit
    # Then when the vendor stuff is logically needed in master:
    git merge vendor

With the option to import later snapshots of the third-party code to the
"vendor" branch then re-merge it to master.

> Care to come up with a patch to Documentation/git-checkout.txt?  The
> description there strongly implies that <start point> is an existing
> commit.  Not much is said about what the lack of <start point> mean when
> it describes "checkout -b", and a standalone description of <start point>
> says "The name of a comit at which to start... Defaults to HEAD".  These
> need to be loosened and described in terms of the closer-to-the-user "at
> my current state".

I'll work on it as soon as I have time.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

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

end of thread, other threads:[~2012-02-07  8:04 UTC | newest]

Thread overview: 34+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-01-29  6:09 Bug: "git checkout -b" should be allowed in empty repo Michael Haggerty
2012-01-29  6:56 ` Junio C Hamano
2012-01-29  7:50   ` Junio C Hamano
2012-01-30  6:38   ` Michael Haggerty
2012-01-30 18:48     ` Junio C Hamano
2012-01-30 20:10       ` Junio C Hamano
2012-01-30 21:50         ` Jeff King
2012-02-06  2:06           ` Junio C Hamano
2012-02-06  2:08             ` Junio C Hamano
2012-02-06  4:30             ` Jeff King
2012-02-06  4:42               ` Andrew Ardill
2012-02-06  5:06                 ` Jeff King
2012-02-06  8:51                   ` Michael Haggerty
2012-02-06  8:57                     ` Jeff King
2012-02-06 18:17                       ` Junio C Hamano
2012-02-06 20:14                         ` Jeff King
2012-02-07  8:04                         ` Michael Haggerty
2012-02-06 18:39                 ` demerphq
2012-02-06  5:15               ` Junio C Hamano
2012-02-06  5:18                 ` Jeff King
2012-02-06  5:30                   ` Junio C Hamano
2012-02-06  5:34                     ` Junio C Hamano
2012-02-06  5:45                     ` Jeff King
2012-02-06  8:59                     ` Michael Haggerty
2012-02-06 18:31                       ` Junio C Hamano
2012-01-30 21:48       ` Jeff King
2012-02-06  1:26         ` [PATCH] branch --edit-description: protect against mistyped branch name Junio C Hamano
2012-02-06  1:27           ` Junio C Hamano
2012-02-06  4:20           ` Jeff King
2012-01-31  8:57       ` Bug: "git checkout -b" should be allowed in empty repo Michael Haggerty
2012-01-31 10:01         ` Johannes Sixt
2012-01-31 10:11           ` demerphq
2012-01-31 10:09         ` Jakub Narebski
2012-01-31 16:32           ` Michael Haggerty

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).