Diff-tree does not work for initial commit

Diff-tree does not work for initial commit

[Anatol Pomozov]

Hi, It looks like I found a bug in git.

The problem: In my script I need to know what files were modified by
given commit. I use diff-tree for it. Although it works for most
cases, for initial commit it does not. Here is a sequence of actions.

anatol:~ $ mkdir mkdir initialcommitissue
anatol:~ $ cd initialcommitissue/
anatol:initialcommitissue $ git init
Initialized empty Git repository in /home/anatol/initialcommitissue/.git/
anatol:initialcommitissue $ echo "First commit" > 1.txt
anatol:initialcommitissue $ git add 1.txt
anatol:initialcommitissue $ git commit -m "First commit"
Created initial commit 31ccc6a: First commit
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 1.txt
anatol:initialcommitissue $ git diff-tree HEAD     <<<<< PROBLEM IS HERE
anatol:initialcommitissue $ echo "Second commit" > 2.txt
anatol:initialcommitissue $ git add 2.txt
anatol:initialcommitissue $ git commit -m "Second commit"
Created commit 51e8bcb: Second commit
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 2.txt
anatol:initialcommitissue $ git diff-tree HEAD
51e8bcbb739fc8329fc092db7a84b02bbc64feb2
:000000 100644 0000000000000000000000000000000000000000
c133ee6afb86d836ae607cc12e7b7b42242aa5fa A	2.txt


so git diff-tree HEAD works fine but git diff-tree HEAD~1 does not. I
guess in sake of consistency it should show all changed files in
initial commit.

-- 
anatol

Re: Diff-tree does not work for initial commit

[Michael J Gruber]

Anatol Pomozov venit, vidit, dixit 15.09.2008 22:01:
> Hi, It looks like I found a bug in git.
> 
> The problem: In my script I need to know what files were modified by 
> given commit. I use diff-tree for it. Although it works for most 
> cases, for initial commit it does not. Here is a sequence of actions.
> 
> 
> anatol:~ $ mkdir mkdir initialcommitissue anatol:~ $ cd
> initialcommitissue/ anatol:initialcommitissue $ git init Initialized
> empty Git repository in /home/anatol/initialcommitissue/.git/ 
> anatol:initialcommitissue $ echo "First commit" > 1.txt 
> anatol:initialcommitissue $ git add 1.txt anatol:initialcommitissue $
> git commit -m "First commit" Created initial commit 31ccc6a: First
> commit 1 files changed, 1 insertions(+), 0 deletions(-) create mode
> 100644 1.txt anatol:initialcommitissue $ git diff-tree HEAD     <<<<<
> PROBLEM IS HERE

>From the man page:

       Compares the content and mode of the blobs found via two tree
objects.

       If there is only one <tree-ish> given, the commit is compared
with its parents (see --stdin below).

       Note that git-diff-tree can use the tree encapsulated in a commit
object.


The initial commit has no parent, so diff-tree does not know which tree
to compare to.

You can do

git diff-tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904 HEAD

but I guess you suggest that diff-tree should do that automatically for
a single parentless treeish: bug -> RFE

diff-tree is plumbing. Would this change break anything?

Michael

Re: Diff-tree does not work for initial commit

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Anatol Pomozov venit, vidit, dixit 15.09.2008 22:01:
>> Hi, It looks like I found a bug in git.
>> 
>> The problem: In my script I need to know what files were modified by 
>> given commit. I use diff-tree for it. Although it works for most 
>> cases, for initial commit it does not. Here is a sequence of actions.
>> 
>> 
>> anatol:~ $ mkdir mkdir initialcommitissue anatol:~ $ cd
>> initialcommitissue/ anatol:initialcommitissue $ git init Initialized
>> empty Git repository in /home/anatol/initialcommitissue/.git/ 
>> anatol:initialcommitissue $ echo "First commit" > 1.txt 
>> anatol:initialcommitissue $ git add 1.txt anatol:initialcommitissue $
>> git commit -m "First commit" Created initial commit 31ccc6a: First
>> commit 1 files changed, 1 insertions(+), 0 deletions(-) create mode
>> 100644 1.txt anatol:initialcommitissue $ git diff-tree HEAD     <<<<<
>> PROBLEM IS HERE
>
> From the man page:
>
>        Compares the content and mode of the blobs found via two tree
> objects.
>
>        If there is only one <tree-ish> given, the commit is compared
> with its parents (see --stdin below).
>
>        Note that git-diff-tree can use the tree encapsulated in a commit
> object.
>
> The initial commit has no parent, so diff-tree does not know which tree
> to compare to.

--root?

Re: Diff-tree does not work for initial commit

[Anatol Pomozov]

On Mon, Sep 15, 2008 at 1:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> The initial commit has no parent, so diff-tree does not know which tree
>> to compare to.
>
> --root?

Oops my bad.

I overlooked this part of the manual. Thanks Junio.

Taking back my words about bug.

-- 
anatol

Re: Diff-tree does not work for initial commit

[Michael J Gruber]

Michael J Gruber venit, vidit, dixit 15.09.2008 22:49:
> Anatol Pomozov venit, vidit, dixit 15.09.2008 22:01:
>> Hi, It looks like I found a bug in git.
>>
>> The problem: In my script I need to know what files were modified by 
>> given commit. I use diff-tree for it. Although it works for most 
>> cases, for initial commit it does not. Here is a sequence of actions.
>>
>>
>> anatol:~ $ mkdir mkdir initialcommitissue anatol:~ $ cd
>> initialcommitissue/ anatol:initialcommitissue $ git init Initialized
>> empty Git repository in /home/anatol/initialcommitissue/.git/ 
>> anatol:initialcommitissue $ echo "First commit" > 1.txt 
>> anatol:initialcommitissue $ git add 1.txt anatol:initialcommitissue $
>> git commit -m "First commit" Created initial commit 31ccc6a: First
>> commit 1 files changed, 1 insertions(+), 0 deletions(-) create mode
>> 100644 1.txt anatol:initialcommitissue $ git diff-tree HEAD     <<<<<
>> PROBLEM IS HERE
> 
> From the man page:
> 
>        Compares the content and mode of the blobs found via two tree
> objects.
> 
>        If there is only one <tree-ish> given, the commit is compared
> with its parents (see --stdin below).
> 
>        Note that git-diff-tree can use the tree encapsulated in a commit
> object.
> 
> 
> The initial commit has no parent, so diff-tree does not know which tree
> to compare to.
> 
> You can do
> 
> git diff-tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904 HEAD
> 
> but I guess you suggest that diff-tree should do that automatically for
> a single parentless treeish: bug -> RFE
> 
> diff-tree is plumbing. Would this change break anything?
> 
> Michael

Ooops, that man page is just too long. Scrolling way down:

"git commit-tree --root" treats the root as a commit with an empty tree.
So this does what you want.

But you may want to look into porcelain like
git show --pretty=format: --name-only

Michael

Re: Diff-tree does not work for initial commit

[Sverre Rabbelier]

On Mon, Sep 15, 2008 at 22:49, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> diff-tree is plumbing. Would this change break anything?

Some of my code uses "git rev-parse" on "HEAD^" to see if a commit has
a parent; I wouldn't be surprised if someone else has a script that
uses "git diff-tree" or such for that purpose, or at least assumes
that for a root commit it will complain. Anyway, as Junio said, for
"diff-tree" you can use the "--root" option. A better RFE would
perhaps be that "--root" be supported in more places.

-- 
Cheers,

Sverre Rabbelier

Re: Diff-tree does not work for initial commit

[Jeff King]

On Mon, Sep 15, 2008 at 11:11:30PM +0200, Sverre Rabbelier wrote:

> Some of my code uses "git rev-parse" on "HEAD^" to see if a commit has
> a parent; I wouldn't be surprised if someone else has a script that
> uses "git diff-tree" or such for that purpose, or at least assumes
> that for a root commit it will complain. Anyway, as Junio said, for
> "diff-tree" you can use the "--root" option. A better RFE would
> perhaps be that "--root" be supported in more places.

I posted this a week or so ago, but I am sure it is incomplete. If there
is interest I can clean it up and do a proper submission.

---
diff --git a/builtin-diff.c b/builtin-diff.c
index 76651bd..4151900 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -332,8 +332,11 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 				break;
 			else if (!strcmp(arg, "--cached")) {
 				add_head_to_pending(&rev);
-				if (!rev.pending.nr)
-					die("No HEAD commit to compare with (yet)");
+				if (!rev.pending.nr) {
+					if (!rev.show_root_diff)
+						die("No HEAD commit to compare with (yet)");
+					add_empty_to_pending(&rev);
+				}
 				break;
 			}
 		}
diff --git a/revision.c b/revision.c
index 2f646de..7ec3990 100644
--- a/revision.c
+++ b/revision.c
@@ -145,16 +145,27 @@ void add_pending_object(struct rev_info *revs, struct object *obj, const char *n
 	add_pending_object_with_mode(revs, obj, name, S_IFINVALID);
 }
 
-void add_head_to_pending(struct rev_info *revs)
+static void add_to_pending_by_name(struct rev_info *revs, const char *name)
 {
 	unsigned char sha1[20];
 	struct object *obj;
-	if (get_sha1("HEAD", sha1))
+	if (get_sha1(name, sha1))
 		return;
 	obj = parse_object(sha1);
 	if (!obj)
 		return;
-	add_pending_object(revs, obj, "HEAD");
+	add_pending_object(revs, obj, name);
+}
+
+void add_head_to_pending(struct rev_info *revs)
+{
+	add_to_pending_by_name(revs, "HEAD");
+}
+
+void add_empty_to_pending(struct rev_info *revs)
+{
+	add_to_pending_by_name(revs,
+			"4b825dc642cb6eb9a060e54bf8d69288fbee4904");
 }
 
 static struct object *get_reference(struct rev_info *revs, const char *name, const unsigned char *sha1, unsigned int flags)
diff --git a/revision.h b/revision.h
index 2fdb2dd..8c990d5 100644
--- a/revision.h
+++ b/revision.h
@@ -152,6 +152,7 @@ extern void add_object(struct object *obj,
 extern void add_pending_object(struct rev_info *revs, struct object *obj, const char *name);
 
 extern void add_head_to_pending(struct rev_info *);
+extern void add_empty_to_pending(struct rev_info *);
 
 enum commit_action {
 	commit_ignore,

Re: Diff-tree does not work for initial commit

[Sverre Rabbelier]

On Tue, Sep 16, 2008 at 00:34, Jeff King <peff@peff.net> wrote:
> I posted this a week or so ago, but I am sure it is incomplete. If there
> is interest I can clean it up and do a proper submission.

<patch snipped>

I like it, although I think that if we add broader support for it, we
should probably be consequent and add it everywhere where appropriate?
(That is ofcourse, assuming that does not take too long to implement
etc.)

-- 
Cheers,

Sverre Rabbelier

Re: Diff-tree does not work for initial commit

[Jeff King]

On Tue, Sep 16, 2008 at 08:19:37AM +0200, Sverre Rabbelier wrote:

> I like it, although I think that if we add broader support for it, we
> should probably be consequent and add it everywhere where appropriate?
> (That is ofcourse, assuming that does not take too long to implement
> etc.)

Right, that was what I meant by "incomplete". I think there are several
other cases where giving "--root" would have expected behavior but is
currently ignored. I'll take a closer look, but I probably won't have
time for a few days.

-Peff

[RFC/PATCH] extend meaning of "--root" option to index comparisons

[Jeff King]

The "--root" option generally means "treat any commits
without parents as a big creation event". This extends the
meaning to make an index comparison against a non-existant
HEAD into a big creation event. In other words, "if this
index _were_ to become a commit, this is how we would show
it with --root."

Specifically, we cover the case of

  git diff --cached --root

to show either the diff between the index and HEAD, or if
there is no HEAD, show the diff against the empty tree.
This can simplify calling scripts which must otherwise
special-case the initial commit when showing the index
status.

We intentionally don't cover:

  - git diff --cached --root HEAD

    The user has specifically asked for HEAD, which doesn't
    exist.

  - git diff-index

    The user is required to specify a tree-ish to
    diff-index; if that tree-ish doesn't exist, we should
    report an error.

Signed-off-by: Jeff King <peff@peff.net>
---
On Tue, Sep 16, 2008 at 02:21:05AM -0400, Jeff King wrote:

> Right, that was what I meant by "incomplete". I think there are several
> other cases where giving "--root" would have expected behavior but is
> currently ignored. I'll take a closer look, but I probably won't have
> time for a few days.

Actually, I wasn't able to find any more cases. I don't think it makes
sense to override the behavior when an explicit tree-ish is given, so
that cuts out the two places mentioned above. Though I think
that scripts using this might prefer the plumbing
diff-index, so maybe there is a better way to support this
(e.g., if --root is specified without a tree-ish, assume
HEAD or empty tree).

diff-tree already handles --root itself. And there is no way to my
knowledge to provoke the same kind of "show the diff against its parent"
behavior via git-diff, since a single tree-ish there means "diff against
the working tree".

And of course for diff-files, such an option makes no sense.

Can you think of any other cases?

 builtin-diff.c       |    7 +++++--
 revision.c           |   17 ++++++++++++++---
 revision.h           |    1 +
 t/t4030-diff-root.sh |   21 +++++++++++++++++++++
 4 files changed, 41 insertions(+), 5 deletions(-)
 create mode 100755 t/t4030-diff-root.sh

diff --git a/builtin-diff.c b/builtin-diff.c
index 037c303..0a1efb5 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -315,8 +315,11 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 				break;
 			else if (!strcmp(arg, "--cached")) {
 				add_head_to_pending(&rev);
-				if (!rev.pending.nr)
-					die("No HEAD commit to compare with (yet)");
+				if (!rev.pending.nr) {
+					if (!rev.show_root_diff)
+						die("No HEAD commit to compare with (yet)");
+					add_empty_to_pending(&rev);
+				}
 				break;
 			}
 		}
diff --git a/revision.c b/revision.c
index 499f0e0..de0fd89 100644
--- a/revision.c
+++ b/revision.c
@@ -145,16 +145,27 @@ void add_pending_object(struct rev_info *revs, struct object *obj, const char *n
 	add_pending_object_with_mode(revs, obj, name, S_IFINVALID);
 }
 
-void add_head_to_pending(struct rev_info *revs)
+static void add_to_pending_by_name(struct rev_info *revs, const char *name)
 {
 	unsigned char sha1[20];
 	struct object *obj;
-	if (get_sha1("HEAD", sha1))
+	if (get_sha1(name, sha1))
 		return;
 	obj = parse_object(sha1);
 	if (!obj)
 		return;
-	add_pending_object(revs, obj, "HEAD");
+	add_pending_object(revs, obj, name);
+}
+
+void add_head_to_pending(struct rev_info *revs)
+{
+	add_to_pending_by_name(revs, "HEAD");
+}
+
+void add_empty_to_pending(struct rev_info *revs)
+{
+	add_to_pending_by_name(revs,
+			"4b825dc642cb6eb9a060e54bf8d69288fbee4904");
 }
 
 static struct object *get_reference(struct rev_info *revs, const char *name, const unsigned char *sha1, unsigned int flags)
diff --git a/revision.h b/revision.h
index fc23522..108f43d 100644
--- a/revision.h
+++ b/revision.h
@@ -151,6 +151,7 @@ extern void add_object(struct object *obj,
 extern void add_pending_object(struct rev_info *revs, struct object *obj, const char *name);
 
 extern void add_head_to_pending(struct rev_info *);
+extern void add_empty_to_pending(struct rev_info *);
 
 enum commit_action {
 	commit_ignore,
diff --git a/t/t4030-diff-root.sh b/t/t4030-diff-root.sh
new file mode 100755
index 0000000..e5174b7
--- /dev/null
+++ b/t/t4030-diff-root.sh
@@ -0,0 +1,21 @@
+#!/bin/sh
+
+test_description='diff --root allows comparison between index and root'
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	echo content >file &&
+	git add file
+'
+
+test_expect_success 'diff --cached (without --root)' '
+	test_must_fail git diff --cached --name-only
+'
+
+test_expect_success 'diff --cached (with --root)' '
+	echo file >expect &&
+	git diff --cached --name-only --root >actual &&
+	test_cmp expect actual
+'
+
+test_done
-- 
1.6.0.2.249.g97d7f.dirty

Re: [RFC/PATCH] extend meaning of "--root" option to index comparisons

[Anatol Pomozov]

Hi, Jeff.

Thanks for your patch.

On Thu, Sep 18, 2008 at 2:21 AM, Jeff King <peff@peff.net> wrote:
> The "--root" option generally means "treat any commits
> without parents as a big creation event". This extends the
> meaning to make an index comparison against a non-existant
> HEAD into a big creation event. In other words, "if this
> index _were_ to become a commit, this is how we would show
> it with --root."
>
> Specifically, we cover the case of
>
>  git diff --cached --root
>
> to show either the diff between the index and HEAD, or if
> there is no HEAD, show the diff against the empty tree.
> This can simplify calling scripts which must otherwise
> special-case the initial commit when showing the index
> status.
>
> We intentionally don't cover:
>
>  - git diff --cached --root HEAD
>
>    The user has specifically asked for HEAD, which doesn't
>    exist.
>
>  - git diff-index
>
>    The user is required to specify a tree-ish to
>    diff-index; if that tree-ish doesn't exist, we should
>    report an error.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> On Tue, Sep 16, 2008 at 02:21:05AM -0400, Jeff King wrote:
>
>> Right, that was what I meant by "incomplete". I think there are several
>> other cases where giving "--root" would have expected behavior but is
>> currently ignored. I'll take a closer look, but I probably won't have
>> time for a few days.
>
> Actually, I wasn't able to find any more cases. I don't think it makes
> sense to override the behavior when an explicit tree-ish is given, so
> that cuts out the two places mentioned above. Though I think
> that scripts using this might prefer the plumbing
> diff-index, so maybe there is a better way to support this
> (e.g., if --root is specified without a tree-ish, assume
> HEAD or empty tree).
>
> diff-tree already handles --root itself. And there is no way to my
> knowledge to provoke the same kind of "show the diff against its parent"
> behavior via git-diff, since a single tree-ish there means "diff against
> the working tree".
>
> And of course for diff-files, such an option makes no sense.
>
> Can you think of any other cases?

git log??

git log --root for empty repo should not print anything (instead of
error message that we have now).

>
>  builtin-diff.c       |    7 +++++--
>  revision.c           |   17 ++++++++++++++---
>  revision.h           |    1 +
>  t/t4030-diff-root.sh |   21 +++++++++++++++++++++

Should documentation (man-pages) reflect your changes as well?

>  4 files changed, 41 insertions(+), 5 deletions(-)
>  create mode 100755 t/t4030-diff-root.sh
>
> diff --git a/builtin-diff.c b/builtin-diff.c
> index 037c303..0a1efb5 100644
> --- a/builtin-diff.c
> +++ b/builtin-diff.c
> @@ -315,8 +315,11 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>                                break;
>                        else if (!strcmp(arg, "--cached")) {
>                                add_head_to_pending(&rev);
> -                               if (!rev.pending.nr)
> -                                       die("No HEAD commit to compare with (yet)");
> +                               if (!rev.pending.nr) {
> +                                       if (!rev.show_root_diff)
> +                                               die("No HEAD commit to compare with (yet)");
> +                                       add_empty_to_pending(&rev);
> +                               }
>                                break;
>                        }
>                }
> diff --git a/revision.c b/revision.c
> index 499f0e0..de0fd89 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -145,16 +145,27 @@ void add_pending_object(struct rev_info *revs, struct object *obj, const char *n
>        add_pending_object_with_mode(revs, obj, name, S_IFINVALID);
>  }
>
> -void add_head_to_pending(struct rev_info *revs)
> +static void add_to_pending_by_name(struct rev_info *revs, const char *name)
>  {
>        unsigned char sha1[20];
>        struct object *obj;
> -       if (get_sha1("HEAD", sha1))
> +       if (get_sha1(name, sha1))
>                return;
>        obj = parse_object(sha1);
>        if (!obj)
>                return;
> -       add_pending_object(revs, obj, "HEAD");
> +       add_pending_object(revs, obj, name);
> +}
> +
> +void add_head_to_pending(struct rev_info *revs)
> +{
> +       add_to_pending_by_name(revs, "HEAD");
> +}
> +
> +void add_empty_to_pending(struct rev_info *revs)
> +{
> +       add_to_pending_by_name(revs,
> +                       "4b825dc642cb6eb9a060e54bf8d69288fbee4904");
>  }
>
>  static struct object *get_reference(struct rev_info *revs, const char *name, const unsigned char *sha1, unsigned int flags)
> diff --git a/revision.h b/revision.h
> index fc23522..108f43d 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -151,6 +151,7 @@ extern void add_object(struct object *obj,
>  extern void add_pending_object(struct rev_info *revs, struct object *obj, const char *name);
>
>  extern void add_head_to_pending(struct rev_info *);
> +extern void add_empty_to_pending(struct rev_info *);
>
>  enum commit_action {
>        commit_ignore,
> diff --git a/t/t4030-diff-root.sh b/t/t4030-diff-root.sh
> new file mode 100755
> index 0000000..e5174b7
> --- /dev/null
> +++ b/t/t4030-diff-root.sh
> @@ -0,0 +1,21 @@
> +#!/bin/sh
> +
> +test_description='diff --root allows comparison between index and root'
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> +       echo content >file &&
> +       git add file
> +'
> +
> +test_expect_success 'diff --cached (without --root)' '
> +       test_must_fail git diff --cached --name-only
> +'
> +
> +test_expect_success 'diff --cached (with --root)' '
> +       echo file >expect &&
> +       git diff --cached --name-only --root >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_done
> --
> 1.6.0.2.249.g97d7f.dirty

-- 
anatol

Re: [RFC/PATCH] extend meaning of "--root" option to index comparisons

[Sverre Rabbelier]

[Please cull the parts of the mail you are not responding to, it is
hard to find your reply when you don't.]

On Thu, Sep 18, 2008 at 18:31, Anatol Pomozov <anatol.pomozov@gmail.com> wrote:
> git log??
>
> git log --root for empty repo should not print anything (instead of
> error message that we have now).

I do not agree here, what is the use in having a command that does nothing?

> Should documentation (man-pages) reflect your changes as well?

Yes, they should be updated.

-- 
Cheers,

Sverre Rabbelier

Re: [RFC/PATCH] extend meaning of "--root" option to index comparisons

[Jeff King]

On Thu, Sep 18, 2008 at 09:31:24AM -0700, Anatol Pomozov wrote:

> > Can you think of any other cases?
> 
> git log??
> 
> git log --root for empty repo should not print anything (instead of
> error message that we have now).

I'm not sure that's the same as "--root", though. In existing --root
cases, we are saying "pretend that beyond the initial commit, there is a
commit that contains the empty tree". The logical extension of git-log
here would be to print out that commit.

Not to mention that "git log --root" _already_ has defined semantics
(you just don't really need it since log.showroot defaults to true).

I wonder if my patch is actually confusing things more, and the right
solution is an option that says "pretend that a non-existant HEAD is a
commit with no log and the empty tree." But I think that may just be
confusing things more, because the semantics of such a null commit
wouldn't be clear (e.g., git log would actually produce a little bit of
output).

Maybe it really is better to just force the caller to check the initial
commit condition. It's more work for them, but the semantics are simple
and unambiguous.

> Should documentation (man-pages) reflect your changes as well?

Yes, definitely. However, I'm not sure yet what the changes should _be_
(if any).

-Peff

Re: [RFC/PATCH] extend meaning of "--root" option to index comparisons

[Anatol Pomozov]

Hi, Jeff.

On Fri, Sep 19, 2008 at 7:25 AM, Jeff King <peff@peff.net> wrote:
> I'm not sure that's the same as "--root", though. In existing --root
> cases, we are saying "pretend that beyond the initial commit, there is a
> commit that contains the empty tree". The logical extension of git-log
> here would be to print out that commit.
Well yeah, agree. My proposal differs from --root meaning and what you
are doing in your patch. So let's continue this 'git log' discussion
without relation to your changes.

> Not to mention that "git log --root" _already_ has defined semantics
> (you just don't really need it since log.showroot defaults to true).
Hm..

anatol:opensource $ mkdir ex
anatol:opensource $ cd ex/
anatol:ex $ git init
Initialized empty Git repository in /personal/sources/opensource/ex/.git/
anatol:ex $ git log
fatal: bad default revision 'HEAD'
anatol:ex $ git config log.showroot true
anatol:ex $ git config log.showroot
true
anatol:ex $ git log
fatal: bad default revision 'HEAD'
anatol:ex $ git config core.showroot true
anatol:ex $ git config core.showroot
true
anatol:ex $ git log
fatal: bad default revision 'HEAD'

I dont see how does log.showroot or core.showroot affect 'git log'.
man git-log says nothing, git-config only mentions that initial commit
is "a big creation event".

> I wonder if my patch is actually confusing things more, and the right
> solution is an option that says "pretend that a non-existant HEAD is a
> commit with no log and the empty tree." But I think that may just be
> confusing things more, because the semantics of such a null commit
> wouldn't be clear (e.g., git log would actually produce a little bit of
> output).
Yeap - probably it would confuse even more, that is why I brought it
for discussion :)

But for me as for person that still actively works with Subversion it
was quite surprising that I have error messages right after I created
a fresh empty repo. I always thought that "Empty repo" -> "No history"
-> "No log output"

Subversion in the same situation.

anatol:2 $ svn info | grep Revision
Revision: 0
anatol:2 $ svn log
------------------------------------------------------------------------

So svn has the same notion of Initial commit which is "big creation
event" but not visible in "svn log"

The difference from git is that init git repo has no HEAD. HEAD is
undefined. Would it be better if absence of HEAD would mean the same
as "HEAD points to the initial commit".

> Maybe it really is better to just force the caller to check the initial
> commit condition. It's more work for them, but the semantics are simple
> and unambiguous.

What is the best way to check that repo has valid HEAD? Check that
file .git/HEAD exists?

-- 
anatol

Re: [RFC/PATCH] extend meaning of "--root" option to index comparisons

[Jeff King]

On Fri, Sep 19, 2008 at 09:54:15AM -0700, Anatol Pomozov wrote:

> I dont see how does log.showroot or core.showroot affect 'git log'.
> man git-log says nothing, git-config only mentions that initial commit
> is "a big creation event".

Try this:

  mkdir repo && cd repo && git init
  echo content >file && git add file && git commit -m initial
  git show ;# you see initial as creation event
  git config log.showroot false
  git show ;# you see commit log, but no diff
  git show --root ;# same as log.showroot=true

where of course the same holds for log, as show uses the same display
logic.

> But for me as for person that still actively works with Subversion it
> was quite surprising that I have error messages right after I created
> a fresh empty repo. I always thought that "Empty repo" -> "No history"
> -> "No log output"

I agree it is a bit nicer not to get an error in that situation. It is
really a result of the way git thinks of history. It is not "no history"
but rather "some history may or may not exist, but you have no valid
pointer to any history".

However, I wonder if it might be possible to special-case the "branch
yet to be born" case. That is, if HEAD points to a ref which does not
exist, can we treat that specially. I suspect it may turn out to be a
lot of work tracking down all of the spots in the code that would need
to be special-cased, which may make it not worthwhile.

> > Maybe it really is better to just force the caller to check the initial
> > commit condition. It's more work for them, but the semantics are simple
> > and unambiguous.
> 
> What is the best way to check that repo has valid HEAD? Check that
> file .git/HEAD exists?

No, HEAD will exist even in a just-created repo. But you can check
whether HEAD points to a valid commit:

  git rev-parse --verify HEAD

-Peff

Re* [RFC/PATCH] extend meaning of "--root" option to index comparisons

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Sep 18, 2008 at 09:31:24AM -0700, Anatol Pomozov wrote:
>
>> > Can you think of any other cases?
>> 
>> git log??
>> 
>> git log --root for empty repo should not print anything (instead of
>> error message that we have now).
>
> I'm not sure that's the same as "--root", though. In existing --root
> cases, we are saying "pretend that beyond the initial commit, there is a
> commit that contains the empty tree". The logical extension of git-log
> here would be to print out that commit.

I would say:

 (1) A user getting an error message from "git init && git log" may be
     annoyed, but he very well knows there is no history yet _anyway_.
     This initial annoyance will pass immediately after creating any
     commit, so I do not think it is a big issue.

     "bad default revision 'HEAD'" is a cryptic way to give that indicaion
     that can be improved but that is a separate issue.  Rewording it so
     that it explains the situation better in user's terms would be a
     worthy improvement.

 (2) "--root" is about "do we show a creation event as a huge diff from
     emptyness?".  Yes, we turn it on for "git log" but it does not have
     anything to do with the issue of yet to be born branch, where there
     isn't even a big creation event yet.

I am reluctant to agree with the opinion that "git log" should be _silent_
in a world without any history.

Perhaps something like this would be a good compromise?  I dunno.

 builtin-log.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git c/builtin-log.c w/builtin-log.c
index 081e660..881324c 100644
--- c/builtin-log.c
+++ w/builtin-log.c
@@ -42,7 +42,14 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 	if (default_date_mode)
 		rev->date_mode = parse_date_format(default_date_mode);
 
-	argc = setup_revisions(argc, argv, rev, "HEAD");
+	argc = setup_revisions(argc, argv, rev, NULL);
+	if (!rev->pending.nr) {
+		add_head_to_pending(rev);
+		if (!rev->pending.nr) {
+			printf("No commits (yet).\n");
+			exit(0);
+		}
+	}
 
 	if (rev->diffopt.pickaxe || rev->diffopt.filter)
 		rev->always_show_header = 0;

Re: Re* [RFC/PATCH] extend meaning of "--root" option to index comparisons

[Jeff King]

On Fri, Sep 19, 2008 at 01:27:46PM -0700, Junio C Hamano wrote:

>  (1) A user getting an error message from "git init && git log" may be
>      annoyed, but he very well knows there is no history yet _anyway_.
>      This initial annoyance will pass immediately after creating any
>      commit, so I do not think it is a big issue.

I think there is an additional case of script writers, who want their
scripts to fail gracefully or otherwise do the right thing with an
initial commit. Right now they have to special-case the initial commit.
I don't know if it is possible to have sane enough behavior that the
special case can be eliminated, or if it will simply make things worse.

>      "bad default revision 'HEAD'" is a cryptic way to give that indicaion
>      that can be improved but that is a separate issue.  Rewording it so
>      that it explains the situation better in user's terms would be a
>      worthy improvement.

I agree that would be an improvement.

>  (2) "--root" is about "do we show a creation event as a huge diff from
>      emptyness?".  Yes, we turn it on for "git log" but it does not have
>      anything to do with the issue of yet to be born branch, where there
>      isn't even a big creation event yet.

What about index comparisons? What should an index comparison to a
branch yet-to-be-born look like? Right now it is an error.

> I am reluctant to agree with the opinion that "git log" should be _silent_
> in a world without any history.

It feels a bit more Unix-y to me. That is, if I am asking for some set
of commits, and there are _no_ commits in the set, then I expect no
output. That makes sense for text processing.

> -	argc = setup_revisions(argc, argv, rev, "HEAD");
> +	argc = setup_revisions(argc, argv, rev, NULL);
> +	if (!rev->pending.nr) {
> +		add_head_to_pending(rev);
> +		if (!rev->pending.nr) {
> +			printf("No commits (yet).\n");
> +			exit(0);
> +		}
> +	}

I like the idea of an improved message, but such a message should
definitely not go to stdout; it would feed nonsense to a command like
"git log | my_log_filter".

-Peff

Re: Re* [RFC/PATCH] extend meaning of "--root" option to index comparisons

[Anatol Pomozov]

Hi,

Jeff mostly explained what I expect from 'git log' and I agree with
him. We ('git log' 'git rev-parse' ...) should separate cases when
revision is broken (like we have a junk in the HEAD file) from the
case when "branch is not created yet" (which is part of normal
workflow).

What about following algorithm. HEAD points to ref and ref is not
created yet. Additional check could be
 a) there are no other refs
 or/and b) object database is empty

>> -     argc = setup_revisions(argc, argv, rev, "HEAD");
>> +     argc = setup_revisions(argc, argv, rev, NULL);
>> +     if (!rev->pending.nr) {
>> +             add_head_to_pending(rev);
>> +             if (!rev->pending.nr) {
>> +                     printf("No commits (yet).\n");
>> +                     exit(0);
>> +             }
>> +     }
>
> I like the idea of an improved message, but such a message should
> definitely not go to stdout; it would feed nonsense to a command like
> "git log | my_log_filter".

+1 here. By default 'git log' should not output anything in this case,
even "No commits yet". Although such message would be fine for
something like "git log --verbose"

-- 
anatol

Re: Re* [RFC/PATCH] extend meaning of "--root" option to index comparisons

[Jakub Narebski]

Anatol Pomozov wrote:

> What about following algorithm. HEAD points to ref and ref is not
> created yet. Additional check could be
>  a) there are no other refs
>  or/and b) object database is empty

What about the case when you create _independent_ branch (additional
root, i.e. parentless commit), like 'html', 'man' and 'todo' branches
in git.git repository?  Neither a) nor b) applies, and I don't consider
such situation a bug.  HEAD should be in proper symref format:
"ref: refs/heads/<branchname>".

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: Re* [RFC/PATCH] extend meaning of "--root" option to index comparisons

[Jeff King]

On Sun, Sep 21, 2008 at 08:58:30AM -0700, Anatol Pomozov wrote:

> revision is broken (like we have a junk in the HEAD file) from the
> case when "branch is not created yet" (which is part of normal
> workflow).
> 
> What about following algorithm. HEAD points to ref and ref is not
> created yet. Additional check could be
>  a) there are no other refs
>  or/and b) object database is empty

I don't think those things have to do with "branch not created yet".
They are about "repository has no branches".

The only thing that indicates you a branch has not been created is the
absence of that ref. So you really have two situations there:

  1. a symref like HEAD points to a branch that does not exist

  2. the user inputs a ref-name that does not exist

In the former, that means we are on a branch that hasn't been created
yet (which generally does mean a new repo, but it's possible to reach
this state in other ways). Or it means somebody made a mistake when they
updated the symref.

In the latter, it _could_ mean that we are interested in a branch yet to
be born, but in general we consider it to mean the user made a typo.

-Peff

Re* [RFC/PATCH] extend meaning of "--root" option to index comparisons

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Sep 19, 2008 at 01:27:46PM -0700, Junio C Hamano wrote:
> ...
>>  (2) "--root" is about "do we show a creation event as a huge diff from
>>      emptyness?".  Yes, we turn it on for "git log" but it does not have
>>      anything to do with the issue of yet to be born branch, where there
>>      isn't even a big creation event yet.
>
> What about index comparisons? What should an index comparison to a
> branch yet-to-be-born look like? Right now it is an error.

It should be an error, because that is _not_ even an comparison.  At least
at diff-index level.

The diff wrapper UI could do something different, though.  And an obvious
thing to do is to give a fake creation event.

The current output feels perfectly sensible to me.

	$ mkdir d; cd d; tar xf .../t.tar; git init; git add .
	$ git diff --cached
        fatal: No HEAD commit to compare with (yet)

The alternative is no different from "find . -type f | xargs cat" from the
point of view of reviewability.  To make sure you have what you want in
your initial revision, so that you can get things right from the start,
you would want to check things like:

	$ tar df .../t.tar ;# did I change anything?
        $ git ls-files ;# do I have what I want?
        $ git clean -n ;# have I missed anything?

By allowing an auto-fallback to the comparison with an empty tree object, 
you are giving these possibilities:

	$ git diff --cached --stat
	$ git diff --cached --name-only

but the latter is already available from ls-files anyway, and the former
does not feel so interesting.  

In exchange, we lose the reminder to the user that this is a creation
event.  An interactive user (remember, I am not talking about diff-index
here, but diff front-end) may want to treat it specially perhaps by being
extra careful.  If there were no downsides like this in "fall back to
comparing with an empty tree" approach, I wouldn't hesitate to agree it is
a good idea, though.

>> I am reluctant to agree with the opinion that "git log" should be _silent_
>> in a world without any history.
>
> It feels a bit more Unix-y to me. That is, if I am asking for some set
> of commits, and there are _no_ commits in the set, then I expect no
> output.

To this, I am inclined to agree.  We could do something like the attached
patch, but there is a caveat.

There may be commands that

 (1) cannot sanely operate without any positive ref;

 (2) give default "HEAD";

 (3) do not have its own input verification to make sure there is at least
     one positive ref, because they have been relying on revision
     machinery to die() with the existing check.

and this patch is actively breaking them.  If people like this approach
(and I probably will join them), commands that match the above criteria
need to be identified and fixed by setting revs->require_valid_def.

 revision.c |   22 ++++++++++++++++++----
 revision.h |    5 ++++-
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git i/revision.c w/revision.c
index 2f646de..5ce7795 100644
--- i/revision.c
+++ w/revision.c
@@ -1295,12 +1295,26 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 		prepare_show_merge(revs);
 	if (revs->def && !revs->pending.nr) {
 		unsigned char sha1[20];
-		struct object *object;
 		unsigned mode;
-		if (get_sha1_with_mode(revs->def, sha1, &mode))
+		int flag;
+		if (!get_sha1_with_mode(revs->def, sha1, &mode)) {
+			struct object *object;
+			object = get_reference(revs, revs->def, sha1, 0);
+			add_pending_object_with_mode(revs, object, revs->def, mode);
+		} else if (!revs->require_valid_def &&
+			   !strcmp(revs->def, "HEAD") &&
+			   resolve_ref(revs->def, sha1, 0, &flag) &&
+			   (flag & REF_ISSYMREF)) {
+			/*
+			 * Most commands can operate on an unborn branch
+			 * without an explicit ref parameter as if no
+			 * positive ref was specified (as opposed to the
+			 * traditional behaviour of barfing on invalid HEAD).
+			 */
+			;
+		} else {
 			die("bad default revision '%s'", revs->def);
-		object = get_reference(revs, revs->def, sha1, 0);
-		add_pending_object_with_mode(revs, object, revs->def, mode);
+		}
 	}
 
 	/* Did the user ask for any diff output? Run the diff! */
diff --git i/revision.h w/revision.h
index 2fdb2dd..7890fb7 100644
--- i/revision.h
+++ w/revision.h
@@ -30,7 +30,10 @@ struct rev_info {
 	const char *prefix;
 	const char *def;
 	void *prune_data;
-	unsigned int early_output;
+
+	/* Miscellaneous */
+	unsigned int	early_output:1,
+			require_valid_def:1;
 
 	/* Traversal flags */
 	unsigned int	dense:1,

Re: Re* [RFC/PATCH] extend meaning of "--root" option to index comparisons

[Jeff King]

On Sun, Sep 21, 2008 at 11:48:10AM -0700, Junio C Hamano wrote:

> > What about index comparisons? What should an index comparison to a
> > branch yet-to-be-born look like? Right now it is an error.
> 
> It should be an error, because that is _not_ even an comparison.  At least
> at diff-index level.
> 
> The diff wrapper UI could do something different, though.  And an obvious
> thing to do is to give a fake creation event.

So is that an implicit endorsement of my "diff --cached --root" patch,
which does exactly that?

> The current output feels perfectly sensible to me.
> 
> 	$ mkdir d; cd d; tar xf .../t.tar; git init; git add .
> 	$ git diff --cached
>         fatal: No HEAD commit to compare with (yet)

Sure. I don't think any behavior should be changed without a "please
treat branch-to-be-born as an empty tree" flag.

> The alternative is no different from "find . -type f | xargs cat" from the
> point of view of reviewability.  To make sure you have what you want in

Interesting comparison. The find example you give has a problem if there
are no files. But my patch is more akin to adding a --no-run-if-empty
flag to xargs here.

> By allowing an auto-fallback to the comparison with an empty tree object, 
> you are giving these possibilities:
> 
> 	$ git diff --cached --stat
> 	$ git diff --cached --name-only
> 
> but the latter is already available from ls-files anyway, and the former
> does not feel so interesting.  

I didn't think we are introducing any new possibilities anyway, since
one can always just compare against the empty tree manually (though I
think "git diff --cached --stat" might be useful for a "status"-like
script).

The advantage is saving callers from having to do two _different_ things
for the initial and regular commit cases.

And for interactive users, seeing the error and saying "Oh, I really
would like to see the diff against the empty tree, but I can't remember
the SHA-1 of the empty tree" (though for that, I have also been running
with a fake ref "EMPTY" which is just simpler to remember). So instead
they can just repeat the command with "--root".

> In exchange, we lose the reminder to the user that this is a creation
> event.  An interactive user (remember, I am not talking about diff-index
> here, but diff front-end) may want to treat it specially perhaps by being
> extra careful.  If there were no downsides like this in "fall back to
> comparing with an empty tree" approach, I wouldn't hesitate to agree it is
> a good idea, though.

You seem to be arguing against doing this by default, which I am not
really advocating (to be honest, I am not 100% sure I am advocating
"diff --cached --root", but this discussion is helping me sort out the
positives and negatives). It only makes sense to me with an extra flag
that explicitly says "and if there is no HEAD, do this fallback."

> To this, I am inclined to agree.  We could do something like the attached
> patch, but there is a caveat.

I do think this approach makes sense (and it mirrors what I just
explained in my last mail to Anatol, which is that we are really talking
about identifying non-existent branches in symrefs).

But I agree there will be a big fallout as we break many of the callers
who expect the barfing. I can try to look at some of the implications,
but expect a delay there from me due to real life concerns.

> +		} else if (!revs->require_valid_def &&
> +			   !strcmp(revs->def, "HEAD") &&
> +			   resolve_ref(revs->def, sha1, 0, &flag) &&
> +			   (flag & REF_ISSYMREF)) {

I wonder if this should be restricted to HEAD and not simply all
symrefs. The thing we are really pointing out is that something points
to a non-existing ref, and because that something is on disk and not
typed in by the user, we are assuming it is not just a mistake.

The only other example I can think of is a refs/remotes/$foo/HEAD, which
we might access via "$foo", can point to an unborn branch.

But maybe it is best to be conservative at first and stick with "HEAD".

-Peff