[Bug] git add -i fails in multiple ways prior to first commit.

[Bug] git add -i fails in multiple ways prior to first commit.

[Rhodes, Kate]

$ git init
$ touch foo.txt
$ git add foo.txt
$ git add -i
fatal: bad revision 'HEAD'
            staged     unstaged path


*** Commands ***
   1: [s]tatus	  2: [u]pdate	  3: [r]evert	  4: [a]dd untracked
   5: [p]atch	  6: [d]iff	  7: [q]uit	  8: [h]elp
What now>


* status fails to report the current status
* update fails to work at all
* revert fails to work (not that there should be anything to revert  
but it shouldn't return a "fatal: bad revision 'HEAD'" message.
* add untracked works
* patch works
* diff fails
* help returns instructions that are incorrect owing to the fact that  
half of the things don't work in this state.
* quit, thankfully, works. :)


Why this is important:
git add's interactive mode is intended to make things easier, thus it  
stands to reason that people new to git are going to want to use it.  
Presenting them with so many broken commands so early on is,  
obviously, a really bad idea.

-masukomi

Re: [Bug] git add -i fails in multiple ways prior to first commit.

[Jeff King]

On Mon, Feb 11, 2008 at 07:59:46PM -0500, Rhodes, Kate wrote:

> * status fails to report the current status
> * update fails to work at all

Below is a patch to address these two. It works by simulating the
diff as if HEAD contained nothing. The 'diff' command is still broken
(it would need to generate fake diff output against an empty tree).

However, I wonder if this is the best approach. It would be nice if
there were a shorthand for "the empty tree" for diffing, so you could
just diff against that rather than HEAD, and have the regular plumbing
generate.

I suppose we could just create that tree object, though it adds a slight
amount of cruft to the object database.

---
 git-add--interactive.perl |   62 ++++++++++++++++++++++++++++++++-------------
 1 files changed, 44 insertions(+), 18 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 17ca5b8..b9f9abe 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -82,6 +82,17 @@ sub list_untracked {
 my $status_fmt = '%12s %12s %s';
 my $status_head = sprintf($status_fmt, 'staged', 'unstaged', 'path');
 
+sub is_initial_commit {
+	return system('git rev-parse HEAD -- >/dev/null 2>&1') != 0;
+}
+
+sub count_blob_lines {
+	my $sha1 = shift;
+	my @lines = run_cmd_pipe(qw(git cat-file blob), $sha1);
+	return scalar(@lines);
+}
+
+
 # Returns list of hashes, contents of each of which are:
 # VALUE:	pathname
 # BINARY:	is a binary path
@@ -103,27 +114,42 @@ sub list_modified {
 		return if (!@tracked);
 	}
 
-	for (run_cmd_pipe(qw(git diff-index --cached
-			     --numstat --summary HEAD --), @tracked)) {
-		if (($add, $del, $file) =
-		    /^([-\d]+)	([-\d]+)	(.*)/) {
-			my ($change, $bin);
-			if ($add eq '-' && $del eq '-') {
-				$change = 'binary';
-				$bin = 1;
-			}
-			else {
-				$change = "+$add/-$del";
-			}
+	if (is_initial_commit()) {
+		for (run_cmd_pipe(qw(git ls-files --stage --exclude-standard
+				     --), @tracked)) {
+			my ($sha1, $file) = /\d+ ([0-9a-f]+) \d\t(.*)/
+				or die "invalid ls-files output: $_";
+			my $n = count_blob_lines($sha1);
 			$data{$file} = {
-				INDEX => $change,
-				BINARY => $bin,
+				INDEX => "+$n/-0",
 				FILE => 'nothing',
-			}
+				INDEXADDDEL => 'create',
+			};
 		}
-		elsif (($adddel, $file) =
-		       /^ (create|delete) mode [0-7]+ (.*)$/) {
-			$data{$file}{INDEX_ADDDEL} = $adddel;
+	}
+	else {
+		for (run_cmd_pipe(qw(git diff-index --cached
+				     --numstat --summary HEAD --), @tracked)) {
+			if (($add, $del, $file) =
+			    /^([-\d]+)	([-\d]+)	(.*)/) {
+				my ($change, $bin);
+				if ($add eq '-' && $del eq '-') {
+					$change = 'binary';
+					$bin = 1;
+				}
+				else {
+					$change = "+$add/-$del";
+				}
+				$data{$file} = {
+					INDEX => $change,
+					BINARY => $bin,
+					FILE => 'nothing',
+				}
+			}
+			elsif (($adddel, $file) =
+			       /^ (create|delete) mode [0-7]+ (.*)$/) {
+				$data{$file}{INDEX_ADDDEL} = $adddel;
+			}
 		}
 	}
 
-- 
1.5.4.1.1296.g34f89-dirty

Re: [Bug] git add -i fails in multiple ways prior to first commit.

[Jakub Narebski]

Jeff King <peff@peff.net> writes:

> However, I wonder if this is the best approach. It would be nice if
> there were a shorthand for "the empty tree" for diffing,

/dev/null? --root?

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [Bug] git add -i fails in multiple ways prior to first commit.

[Jeff King]

On Wed, Feb 13, 2008 at 02:23:48AM -0800, Jakub Narebski wrote:

> > However, I wonder if this is the best approach. It would be nice if
> > there were a shorthand for "the empty tree" for diffing,
> 
> /dev/null? --root?

--root doesn't work since it is so entwined with diffing against an
actual tree. I was thinking something more like turning the magic ref
"EMPTY_TREE" internally into the empty tree, and then everything would
just work.

Or is that what you were suggesting "/dev/null" for? Another possible
name is 4b825dc642cb6eb9a060e54bf8d69288fbee4904, which is the SHA-1 of
the empty tree (currently, but I don't suppose we are going to change the
tree format anytime soon).

-Peff

[PATCH] add--interactive: handle initial commit better

[Jeff King]

On Wed, Feb 13, 2008 at 05:16:49AM -0500, Jeff King wrote:

> However, I wonder if this is the best approach. It would be nice if
> there were a shorthand for "the empty tree" for diffing, so you could
> just diff against that rather than HEAD, and have the regular plumbing
> generate.
> 
> I suppose we could just create that tree object, though it adds a slight
> amount of cruft to the object database.

And here it is. I think this is a more sane approach in general than the
last patch. The only ugly thing is the empty tree hack, but that can be
addressed with a patch to allow referencing the empty tree without it
existing in the object db.

-- >8 --

There were several points where we looked at the HEAD
commit; for initial commits, this is meaningless. So instead
we:

  - show staged status data as a diff against the empty tree
    instead of HEAD
  - show file diffs as creation events
  - use "git rm --cached" to revert instead of going back to
    the HEAD commit

The empty tree diff is a little hack-ish. We actually write
the empty tree object from a fake index using "git
write-tree". This would be a bit cleaner if we could
magically reference the empty tree.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-add--interactive.perl |   64 +++++++++++++++++++++++++++++++++-----------
 1 files changed, 48 insertions(+), 16 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 17ca5b8..bae631e 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -82,6 +82,28 @@ sub list_untracked {
 my $status_fmt = '%12s %12s %s';
 my $status_head = sprintf($status_fmt, 'staged', 'unstaged', 'path');
 
+{
+	my $initial;
+	sub is_initial_commit {
+		$initial = system('git rev-parse HEAD -- >/dev/null 2>&1') != 0
+			unless defined $initial;
+		return $initial;
+	}
+}
+
+{
+	my $sha1;
+	sub get_empty_tree {
+		if (!$sha1) {
+			local $ENV{GIT_INDEX_FILE} = "$GIT_DIR/empty_index";
+			$sha1 = run_cmd_pipe(qw(git write-tree));
+			chomp $sha1;
+			unlink($ENV{GIT_INDEX_FILE});
+		}
+		return $sha1;
+	}
+}
+
 # Returns list of hashes, contents of each of which are:
 # VALUE:	pathname
 # BINARY:	is a binary path
@@ -103,8 +125,10 @@ sub list_modified {
 		return if (!@tracked);
 	}
 
+	my $reference = is_initial_commit() ? get_empty_tree() : 'HEAD';
 	for (run_cmd_pipe(qw(git diff-index --cached
-			     --numstat --summary HEAD --), @tracked)) {
+			     --numstat --summary), $reference,
+			     '--', @tracked)) {
 		if (($add, $del, $file) =
 		    /^([-\d]+)	([-\d]+)	(.*)/) {
 			my ($change, $bin);
@@ -476,21 +500,27 @@ sub revert_cmd {
 				       HEADER => $status_head, },
 				     list_modified());
 	if (@update) {
-		my @lines = run_cmd_pipe(qw(git ls-tree HEAD --),
-					 map { $_->{VALUE} } @update);
-		my $fh;
-		open $fh, '| git update-index --index-info'
-		    or die;
-		for (@lines) {
-			print $fh $_;
+		if (is_initial_commit()) {
+			system(qw(git rm --cached),
+				map { $_->{VALUE} } @update);
 		}
-		close($fh);
-		for (@update) {
-			if ($_->{INDEX_ADDDEL} &&
-			    $_->{INDEX_ADDDEL} eq 'create') {
-				system(qw(git update-index --force-remove --),
-				       $_->{VALUE});
-				print "note: $_->{VALUE} is untracked now.\n";
+		else {
+			my @lines = run_cmd_pipe(qw(git ls-tree HEAD --),
+						 map { $_->{VALUE} } @update);
+			my $fh;
+			open $fh, '| git update-index --index-info'
+			    or die;
+			for (@lines) {
+				print $fh $_;
+			}
+			close($fh);
+			for (@update) {
+				if ($_->{INDEX_ADDDEL} &&
+				    $_->{INDEX_ADDDEL} eq 'create') {
+					system(qw(git update-index --force-remove --),
+					       $_->{VALUE});
+					print "note: $_->{VALUE} is untracked now.\n";
+				}
 			}
 		}
 		refresh();
@@ -956,7 +986,9 @@ sub diff_cmd {
 				     HEADER => $status_head, },
 				   @mods);
 	return if (!@them);
-	system(qw(git diff -p --cached HEAD --), map { $_->{VALUE} } @them);
+	my $reference = is_initial_commit() ? get_empty_tree() : 'HEAD';
+	system(qw(git diff -p --cached), $reference, '--',
+		map { $_->{VALUE} } @them);
 }
 
 sub quit_cmd {
-- 
1.5.4.1.1296.g34f89-dirty

Re: [PATCH] add--interactive: handle initial commit better

[Jeff King]

On Wed, Feb 13, 2008 at 05:50:51AM -0500, Jeff King wrote:

> And here it is. I think this is a more sane approach in general than the
> last patch. The only ugly thing is the empty tree hack, but that can be
> addressed with a patch to allow referencing the empty tree without it
> existing in the object db.

And this patch hard-codes the empty tree. I _think_ this shouldn't cause
us any problems, as it just reuses the existing pretend_sha1_file
infrastructure. However that infrastructure is very rarely used.

This patch also contains the matching fix to git-add--interactive (on
top of my last patch). They should probably just be squashed together,
but I can submit a cleaned up 2-patch series if people agree that this
is a good idea.

-- >8 --
hard-code the empty tree object

Now any commands may reference the empty tree object by its
sha1 (4b825dc642cb6eb9a060e54bf8d69288fbee4904). This is
useful for showing some diffs, especially for initial
commits.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-add--interactive.perl |   13 ++-----------
 sha1_file.c               |   11 +++++++++++
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index bae631e..a0a81f1 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -91,17 +91,8 @@ my $status_head = sprintf($status_fmt, 'staged', 'unstaged', 'path');
 	}
 }
 
-{
-	my $sha1;
-	sub get_empty_tree {
-		if (!$sha1) {
-			local $ENV{GIT_INDEX_FILE} = "$GIT_DIR/empty_index";
-			$sha1 = run_cmd_pipe(qw(git write-tree));
-			chomp $sha1;
-			unlink($ENV{GIT_INDEX_FILE});
-		}
-		return $sha1;
-	}
+sub get_empty_tree {
+	return '4b825dc642cb6eb9a060e54bf8d69288fbee4904';
 }
 
 # Returns list of hashes, contents of each of which are:
diff --git a/sha1_file.c b/sha1_file.c
index 4179949..1a6c7c8 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1845,6 +1845,15 @@ static struct cached_object {
 } *cached_objects;
 static int cached_object_nr, cached_object_alloc;
 
+static struct cached_object empty_tree = {
+	/* empty tree sha1: 4b825dc642cb6eb9a060e54bf8d69288fbee4904 */
+	"\x4b\x82\x5d\xc6\x42\xcb\x6e\xb9\xa0\x60"
+	"\xe5\x4b\xf8\xd6\x92\x88\xfb\xee\x49\x04",
+	OBJ_TREE,
+	"",
+	0
+};
+
 static struct cached_object *find_cached_object(const unsigned char *sha1)
 {
 	int i;
@@ -1854,6 +1863,8 @@ static struct cached_object *find_cached_object(const unsigned char *sha1)
 		if (!hashcmp(co->sha1, sha1))
 			return co;
 	}
+	if (!hashcmp(sha1, empty_tree.sha1))
+		return &empty_tree;
 	return NULL;
 }
 
-- 
1.5.4.1.1297.g3482b-dirty

Re: [PATCH] add--interactive: handle initial commit better

[Johannes Schindelin]

Hi,

On Wed, 13 Feb 2008, Jeff King wrote:

> diff --git a/sha1_file.c b/sha1_file.c
> index 4179949..1a6c7c8 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -1845,6 +1845,15 @@ static struct cached_object {
>  } *cached_objects;
>  static int cached_object_nr, cached_object_alloc;
>  
> +static struct cached_object empty_tree = {
> +	/* empty tree sha1: 4b825dc642cb6eb9a060e54bf8d69288fbee4904 */
> +	"\x4b\x82\x5d\xc6\x42\xcb\x6e\xb9\xa0\x60"
> +	"\xe5\x4b\xf8\xd6\x92\x88\xfb\xee\x49\x04",
> +	OBJ_TREE,
> +	"",
> +	0
> +};
> +
>  static struct cached_object *find_cached_object(const unsigned char *sha1)
>  {
>  	int i;
> @@ -1854,6 +1863,8 @@ static struct cached_object *find_cached_object(const unsigned char *sha1)
>  		if (!hashcmp(co->sha1, sha1))
>  			return co;
>  	}
> +	if (!hashcmp(sha1, empty_tree.sha1))
> +		return &empty_tree;
>  	return NULL;
>  }

Heh.  This is cute.  But it is also a bit hard to reference, no?  I mean, 
you have to remember the SHA-1 of it...

Maybe {} ?

Ciao,
Dscho

Re: [PATCH] add--interactive: handle initial commit better

[Jakub Narebski]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Wed, 13 Feb 2008, Jeff King wrote:
> >  	}
> > +	if (!hashcmp(sha1, empty_tree.sha1))
> > +		return &empty_tree;
> >  	return NULL;
> >  }
> 
> Heh.  This is cute.  But it is also a bit hard to reference, no?  I mean, 
> you have to remember the SHA-1 of it...
> 
> Maybe {} ?

Or NULL? You can do this even without modifying git code, I think, by
adding refs/NULL with appropriate sha-1...

^{tree} I think resolves to HEAD^{tree}

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH] add--interactive: handle initial commit better

[Jeff King]

On Wed, Feb 13, 2008 at 05:11:10AM -0800, Jakub Narebski wrote:

> > Heh.  This is cute.  But it is also a bit hard to reference, no?  I mean, 
> > you have to remember the SHA-1 of it...
> > 
> > Maybe {} ?
> 
> Or NULL? You can do this even without modifying git code, I think, by
> adding refs/NULL with appropriate sha-1...

You still need my patch, unless you want to create the empty tree object
in the databsae. The existence of the object and its name are separate.
If you want to add a magic ref or syntax that maps to the correct SHA-1,
then that makes sense to me.

-Peff

Re: [PATCH] add--interactive: handle initial commit better

[Johannes Schindelin]

Hi,

On Wed, 13 Feb 2008, Jakub Narebski wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Wed, 13 Feb 2008, Jeff King wrote:
> > >  	}
> > > +	if (!hashcmp(sha1, empty_tree.sha1))
> > > +		return &empty_tree;
> > >  	return NULL;
> > >  }
> > 
> > Heh.  This is cute.  But it is also a bit hard to reference, no?  I mean, 
> > you have to remember the SHA-1 of it...
> > 
> > Maybe {} ?
> 
> Or NULL?

I was talking about a special handling: "{}" is not a valid refname.  But 
"NULL" _is_.  So I wanted to avoid that "NULL" explicitely.

Ciao,
Dscho

Re: [PATCH] add--interactive: handle initial commit better

[Johannes Schindelin]

Hi,

On Wed, 13 Feb 2008, Johannes Schindelin wrote:

> On Wed, 13 Feb 2008, Jakub Narebski wrote:
> 
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > 
> > > On Wed, 13 Feb 2008, Jeff King wrote:
> > > >  	}
> > > > +	if (!hashcmp(sha1, empty_tree.sha1))
> > > > +		return &empty_tree;
> > > >  	return NULL;
> > > >  }
> > > 
> > > Heh.  This is cute.  But it is also a bit hard to reference, no?  I 
> > > mean, you have to remember the SHA-1 of it...
> > > 
> > > Maybe {} ?

IOW something like this (on top of your two patches; feel free to merge):

-- snipsnap --
[PATCH] Make {} synonymous to the empty tree

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
 cache.h     |    5 +++++
 sha1_file.c |    4 +---
 sha1_name.c |    5 +++++
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index f4c56fa..b71f772 100644
--- a/cache.h
+++ b/cache.h
@@ -266,6 +266,11 @@ static inline enum object_type object_type(unsigned int mode)
 #define INFOATTRIBUTES_FILE "info/attributes"
 #define ATTRIBUTE_MACRO_PREFIX "[attr]"
 
+/* empty tree sha1: 4b825dc642cb6eb9a060e54bf8d69288fbee4904 */
+#define EMPTY_TREE_SHA1 (unsigned char *)\
+	"\x4b\x82\x5d\xc6\x42\xcb\x6e\xb9\xa0\x60" \
+	"\xe5\x4b\xf8\xd6\x92\x88\xfb\xee\x49\x04"
+
 extern int is_bare_repository_cfg;
 extern int is_bare_repository(void);
 extern int is_inside_git_dir(void);
diff --git a/sha1_file.c b/sha1_file.c
index 1a6c7c8..cba629a 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1846,9 +1846,7 @@ static struct cached_object {
 static int cached_object_nr, cached_object_alloc;
 
 static struct cached_object empty_tree = {
-	/* empty tree sha1: 4b825dc642cb6eb9a060e54bf8d69288fbee4904 */
-	"\x4b\x82\x5d\xc6\x42\xcb\x6e\xb9\xa0\x60"
-	"\xe5\x4b\xf8\xd6\x92\x88\xfb\xee\x49\x04",
+	EMPTY_TREE_SHA1,
 	OBJ_TREE,
 	"",
 	0
diff --git a/sha1_name.c b/sha1_name.c
index 6cfd1f9..3a39a55 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -760,5 +760,10 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
 			return get_tree_entry(tree_sha1, cp+1, sha1, mode);
 		}
 	}
+	if (ret && !strcmp(name, "{}")) {
+		*mode = 0755;
+		hashcpy(sha1, EMPTY_TREE_SHA1);
+		ret = 0;
+	}
 	return ret;
 }

Re: [Bug] git add -i fails in multiple ways prior to first commit.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> However, I wonder if this is the best approach. It would be nice if
> there were a shorthand for "the empty tree" for diffing, so you could
> just diff against that rather than HEAD, and have the regular plumbing
> generate.

I guess you can set your worktree to an empty directory and run
diff-files backwards, like perhaps:

	mkdir /var/tmp/empty
        (cd .git && GIT_WORK_TREE=/var/tmp/empty git diff -R)

Have I tried it?  No --- I am not sick enough to be motivated.

Re: [Bug] git add -i fails in multiple ways prior to first commit.

[Jeff King]

On Wed, Feb 13, 2008 at 03:30:44AM -0800, Junio C Hamano wrote:

> > However, I wonder if this is the best approach. It would be nice if
> > there were a shorthand for "the empty tree" for diffing, so you could
> > just diff against that rather than HEAD, and have the regular plumbing
> > generate.
> 
> I guess you can set your worktree to an empty directory and run
> diff-files backwards, like perhaps:
> 
> 	mkdir /var/tmp/empty
>         (cd .git && GIT_WORK_TREE=/var/tmp/empty git diff -R)
> 
> Have I tried it?  No --- I am not sick enough to be motivated.

Actually, I thought of that already. It does work, though when I tried
it, I failed to 'cd .git' which broke it. But yes, I think it is a
nastier solution than just using the "virtual" empty tree.

-Peff