[PATCH] blame: Allow to blame paths freshly added to the index

When blaming files, changes in the work tree are taken into account
and displayed as being "Not Committed Yet".

However, when blaming a file that is not known to the current HEAD,
git blame fails with `no such path 'foo' in HEAD`, even when the file
was git add'ed.

This would seem uninteresting with the plain `git blame` case, which
it is, but it becomes useful when using copy detection, and the new file
was created from pieces already in HEAD, moved or copied from other
files.
---
 builtin/blame.c               |  4 +++-
 t/t8003-blame-corner-cases.sh | 23 +++++++++++++++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 1e214bd..0858b18 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c

@@ -2240,7 +2240,9 @@ static void verify_working_tree_path(struct commit *work_tree, const char *path)
 		    sha1_object_info(blob_sha1, NULL) == OBJ_BLOB)
 			return;
 	}
-	die("no such path '%s' in HEAD", path);
+
+	if (cache_name_pos(path, strlen(path)) < 0)
+		die("no such path '%s' in HEAD", path);
 }
 
 static struct commit_list **append_parent(struct commit_list **tail, const unsigned char *sha1)

diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
index a9b266f..a0a09e2 100755
--- a/t/t8003-blame-corner-cases.sh
+++ b/t/t8003-blame-corner-cases.sh

@@ -137,6 +137,29 @@ test_expect_success 'blame wholesale copy and more' '
 
 '
 
+test_expect_success 'blame wholesale copy and more in the index' '
+
+	{
+		echo ABC
+		echo DEF
+		echo XXXX
+		echo YYYY
+		echo GHIJK
+	} >horse &&
+	git add horse &&
+	git blame -f -C -C1 -- horse | sed -e "$pick_fc" >current &&
+	{
+		echo mouse-Initial
+		echo mouse-Second
+		echo cow-Fifth
+		echo horse-Not
+		echo mouse-Third
+	} >expected &&
+	test_cmp expected current &&
+	git rm -f horse
+
+'
+
 test_expect_success 'blame path that used to be a directory' '
 	mkdir path &&
 	echo A A A A A >path/file &&

-- 
2.9.1.276.geea30e8

Hi Mike,

On Fri, 15 Jul 2016, Mike Hommey wrote:

When blaming files, changes in the work tree are taken into account
and displayed as being "Not Committed Yet".

However, when blaming a file that is not known to the current HEAD,
git blame fails with `no such path 'foo' in HEAD`, even when the file
was git add'ed.

This would seem uninteresting with the plain `git blame` case, which
it is, but it becomes useful when using copy detection, and the new file
was created from pieces already in HEAD, moved or copied from other
files.
---

Well explained.

Please add your sign-off.

A more common way to do this in our test scripts is by using here
documents. However, in this case I would suggest

	test_write_lines ABC DEF XXXX YYYY GHIJK >horse

instead. The equivalent applies to the 'expected' file below:

+	git add horse &&
+	git blame -f -C -C1 -- horse | sed -e "$pick_fc" >current &&
+	{
+		echo mouse-Initial
+		echo mouse-Second
+		echo cow-Fifth
+		echo horse-Not
+		echo mouse-Third
+	} >expected &&
+	test_cmp expected current &&
+	git rm -f horse

Should this not be a

	test_when_finished "git rm -f horse"

at the beginning?

Otherwise it looks really good to me.

Ciao,
Dscho

On Fri, Jul 15, 2016 at 12:45:15PM +0200, Johannes Schindelin wrote:

Hi Mike,

On Fri, 15 Jul 2016, Mike Hommey wrote:

quoted

When blaming files, changes in the work tree are taken into account
and displayed as being "Not Committed Yet".

However, when blaming a file that is not known to the current HEAD,
git blame fails with `no such path 'foo' in HEAD`, even when the file
was git add'ed.

This would seem uninteresting with the plain `git blame` case, which
it is, but it becomes useful when using copy detection, and the new file
was created from pieces already in HEAD, moved or copied from other
files.
---

Well explained.

Please add your sign-off.

Facepalm, forgot to sign-off again.

quoted

 static struct commit_list **append_parent(struct commit_list **tail, const unsigned char *sha1)

diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
index a9b266f..a0a09e2 100755
--- a/t/t8003-blame-corner-cases.sh
+++ b/t/t8003-blame-corner-cases.sh

@@ -137,6 +137,29 @@ test_expect_success 'blame wholesale copy and more' '
 
 '
 
+test_expect_success 'blame wholesale copy and more in the index' '
+
+	{
+		echo ABC
+		echo DEF
+		echo XXXX
+		echo YYYY
+		echo GHIJK
+	} >horse &&

A more common way to do this in our test scripts is by using here
documents. However, in this case I would suggest

	test_write_lines ABC DEF XXXX YYYY GHIJK >horse

I merely copied the pattern used in other places in the same test file.
Using test_write_lines or something else (what are "here documents"?)
would break consistency. I can also change the other similar blocks at
the same time, though, whichever you prefer.

instead. The equivalent applies to the 'expected' file below:

quoted

+	git add horse &&
+	git blame -f -C -C1 -- horse | sed -e "$pick_fc" >current &&
+	{
+		echo mouse-Initial
+		echo mouse-Second
+		echo cow-Fifth
+		echo horse-Not
+		echo mouse-Third
+	} >expected &&
+	test_cmp expected current &&
+	git rm -f horse

Should this not be a

	test_when_finished "git rm -f horse"

at the beginning?

Indeed.

Thanks

Mike

On Fri, Jul 15, 2016 at 09:32:45PM +0900, Mike Hommey wrote:

quoted

quoted

+test_expect_success 'blame wholesale copy and more in the index' '
+
+	{
+		echo ABC
+		echo DEF
+		echo XXXX
+		echo YYYY
+		echo GHIJK
+	} >horse &&

A more common way to do this in our test scripts is by using here
documents. However, in this case I would suggest

	test_write_lines ABC DEF XXXX YYYY GHIJK >horse

I merely copied the pattern used in other places in the same test file.
Using test_write_lines or something else (what are "here documents"?)
would break consistency. I can also change the other similar blocks at
the same time, though, whichever you prefer.

A here document is this:

   cat <<-\EOF
   ABC
   DEF
   XXXX
   YYYY
   GHIJK
   EOF

The "<<" starts the here-doc. The "-" tells the shell to strip leading
tabs (so you can keep it indented with the rest of the code. The "\"
tells the shell not to interpolate (not a big deal here, but great for
more complicated input). The "EOF" tells it where to stop.

Matching surrounding style is always reasonable, though I do think this
particular file is a bit of an oddball. Most of our scripts use here
documents. Either is OK in this case, IMHO.

Personally I do not find test_write_lines particularly readable, but I
guess some people do, which is why it exists.

-Peff

On Fri, Jul 15, 2016 at 08:37:59AM -0400, Jeff King wrote:

On Fri, Jul 15, 2016 at 09:32:45PM +0900, Mike Hommey wrote:

quoted

quoted

quoted

+test_expect_success 'blame wholesale copy and more in the index' '
+
+	{
+		echo ABC
+		echo DEF
+		echo XXXX
+		echo YYYY
+		echo GHIJK
+	} >horse &&

A more common way to do this in our test scripts is by using here
documents. However, in this case I would suggest

	test_write_lines ABC DEF XXXX YYYY GHIJK >horse

I merely copied the pattern used in other places in the same test file.
Using test_write_lines or something else (what are "here documents"?)
would break consistency. I can also change the other similar blocks at
the same time, though, whichever you prefer.

A here document is this:

   cat <<-\EOF
   ABC
   DEF
   XXXX
   YYYY
   GHIJK
   EOF

The "<<" starts the here-doc. The "-" tells the shell to strip leading
tabs (so you can keep it indented with the rest of the code. The "\"
tells the shell not to interpolate (not a big deal here, but great for
more complicated input). The "EOF" tells it where to stop.

Oh, so that's what they are called! I've used them for 20 years
without knowing :) TIL.

Mike

When blaming files, changes in the work tree are taken into account
and displayed as being "Not Committed Yet".

However, when blaming a file that is not known to the current HEAD,
git blame fails with `no such path 'foo' in HEAD`, even when the file
was git add'ed.

This would seem uninteresting with the plain `git blame` case, which
it is, but it becomes useful when using copy detection, and the new file
was created from pieces already in HEAD, moved or copied from other
files.

Signed-off-by: Mike Hommey <redacted>
---
 builtin/blame.c               |  4 ++-
 t/t8003-blame-corner-cases.sh | 57 ++++++++++++++++++++++++++++++-------------
 2 files changed, 43 insertions(+), 18 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 1e214bd..0858b18 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c

@@ -2240,7 +2240,9 @@ static void verify_working_tree_path(struct commit *work_tree, const char *path)
 		    sha1_object_info(blob_sha1, NULL) == OBJ_BLOB)
 			return;
 	}
-	die("no such path '%s' in HEAD", path);
+
+	if (cache_name_pos(path, strlen(path)) < 0)
+		die("no such path '%s' in HEAD", path);
 }
 
 static struct commit_list **append_parent(struct commit_list **tail, const unsigned char *sha1)

diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
index a9b266f..2812d7c 100755
--- a/t/t8003-blame-corner-cases.sh
+++ b/t/t8003-blame-corner-cases.sh

@@ -41,12 +41,12 @@ test_expect_success setup '
 	test_tick &&
 	GIT_AUTHOR_NAME=Fourth git commit -m Fourth &&
 
-	{
-		echo ABC
-		echo DEF
-		echo XXXX
-		echo GHIJK
-	} >cow &&
+	cat >cow <<-\EOF &&
+	ABC
+	DEF
+	XXXX
+	GHIJK
+	EOF
 	git add cow &&
 	test_tick &&
 	GIT_AUTHOR_NAME=Fifth git commit -m Fifth

@@ -115,11 +115,11 @@ test_expect_success 'append with -C -C -C' '
 test_expect_success 'blame wholesale copy' '
 
 	git blame -f -C -C1 HEAD^ -- cow | sed -e "$pick_fc" >current &&
-	{
-		echo mouse-Initial
-		echo mouse-Second
-		echo mouse-Third
-	} >expected &&
+	cat >expected <<-\EOF &&
+	mouse-Initial
+	mouse-Second
+	mouse-Third
+	EOF
 	test_cmp expected current
 
 '

@@ -127,12 +127,35 @@ test_expect_success 'blame wholesale copy' '
 test_expect_success 'blame wholesale copy and more' '
 
 	git blame -f -C -C1 HEAD -- cow | sed -e "$pick_fc" >current &&
-	{
-		echo mouse-Initial
-		echo mouse-Second
-		echo cow-Fifth
-		echo mouse-Third
-	} >expected &&
+	cat >expected <<-\EOF &&
+	mouse-Initial
+	mouse-Second
+	cow-Fifth
+	mouse-Third
+	EOF
+	test_cmp expected current
+
+'
+
+test_expect_success 'blame wholesale copy and more in the index' '
+
+	cat >horse <<-\EOF &&
+	ABC
+	DEF
+	XXXX
+	YYYY
+	GHIJK
+	EOF
+	git add horse &&
+	test_when_finished "git rm -f horse" &&
+	git blame -f -C -C1 -- horse | sed -e "$pick_fc" >current &&
+	cat >expected <<-\EOF &&
+	mouse-Initial
+	mouse-Second
+	cow-Fifth
+	horse-Not
+	mouse-Third
+	EOF
 	test_cmp expected current
 
 '

-- 
2.9.1.276.geea30e8

Hi Mike,

On Fri, 15 Jul 2016, Mike Hommey wrote:

Sorry, I did not realize that there was precedent for this awkward
paradigm in the same test script.

I like that you fix them, but I would prefer that to be done in a separate
patch (does not even need to be the same patch series).

Apart from that (i.e. apart from touching unrelated parts of the test
script), the patch looks good to me.

Thanks,
Dscho