Re: Bug: "git checkout -b" should be allowed in empty repo

Michael Haggerty [off-list ref] 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');

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:

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

On 01/30/2012 07:48 PM, Junio C Hamano wrote:

Michael Haggerty [off-list ref] writes:

quoted

[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/

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

Michael Haggerty [off-list ref] writes:

On 01/30/2012 07:48 PM, Junio C Hamano wrote:

quoted

Michael Haggerty [off-list ref] writes:

quoted

[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.

quoted

(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.
 
[...]

quoted

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

On 31 January 2012 11:01, Johannes Sixt [off-list ref] wrote:

Am 1/31/2012 9:57, schrieb Michael Haggerty:

quoted

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

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/