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

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

[Mike Hommey]

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

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

[Johannes Schindelin]

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.

>  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

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

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

[Mike Hommey]

On Fri, Jul 15, 2016 at 12:45:15PM +0200, Johannes Schindelin wrote:
> 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.

Facepalm, forgot to sign-off again.

> >  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:
> 
> > +	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

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

[Jeff King]

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

> > > +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

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

[Mike Hommey]

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:
> 
> > > > +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

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

[Mike Hommey]

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 <mh@glandium.org>
---
 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

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

[Johannes Schindelin]

Hi Mike,

On Fri, 15 Jul 2016, Mike Hommey wrote:

> 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 &&

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

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

[Junio C Hamano]

Mike Hommey <mh@glandium.org> writes:

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

I suspect that this would be useful without copy detection.  If you
"git mv fileA fileB" (optionally followed by "edit fileB"), fileB
would not be in HEAD but you should be able to trace the lineage of
the lines in it back through the renaming event, and this change
also allows that use case, no?

> ---

Missing sign-off?

>  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);
>  }

This is a surprisingly small change to bring a big difference at the
usage level.  I am sort-of surprised that the "fake working tree
commit" mechanism was already prepared to handle this, even though I
am responsible for the introduction of it at 1cfe7733 (git-blame: no
rev means start from the working tree file., 2007-01-30).

Having said that, do we act differently when the index is unmerged
at path in any way?  When path exists but is unmerged, you get
negative index from cache_name_pos(), and IIUC, "blaming of working
tree file" does not care if the index is unmerged.  It creates the
fake working tree file commit, pretends HEAD is its parent, and digs
the lineage from there.

I suspect that the above change needs to be updated further if the
user wants to run "blame path" during a conflicted renaming merge,
i.e.

 0. Before two histories diverged, there was old_path.
 1. Our side updated contents of that file and kept it at old_path.
 2. Their side updated contents of that file and moved it to path.
 3. "git merge" notcied the rename, created three stages at "path",
    with the result of conflicted content-level merge in the working
    tree at "path".
 4. The user edits "path" and resolves the conflict, but wants to
    double check before running "git add path".
 5. "git blame path"

Perhaps something like this should suffice:

    pos = cache_name_pos(path, strlen(path));
    if (0 <= pos)
    	; /* ok */
    else if (!strcmp(active_cache[-1 - pos]->name), path)
        ; /* ok -- just unmerged */
    else
    	die("no such path in HEAD");

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

[Junio C Hamano]

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

> I suspect that the above change needs to be updated further if the
> user wants to run "blame path" during a conflicted renaming merge,
> i.e.
>
>  0. Before two histories diverged, there was old_path.
>  1. Our side updated contents of that file and kept it at old_path.
>  2. Their side updated contents of that file and moved it to path.
>  3. "git merge" notcied the rename, created three stages at "path",
>     with the result of conflicted content-level merge in the working
>     tree at "path".
>  4. The user edits "path" and resolves the conflict, but wants to
>     double check before running "git add path".
>  5. "git blame path"
>
> Perhaps something like this should suffice:
>
>     pos = cache_name_pos(path, strlen(path));
>     if (0 <= pos)
>     	; /* ok */
>     else if (!strcmp(active_cache[-1 - pos]->name), path)
>         ; /* ok -- just unmerged */
>     else
>     	die("no such path in HEAD");

I do not think the "conflicted renaming merge" example would not be
a problem in practice iff "git merge" was used, because the fake
working tree commit would look at both our tree and their tree, and
find "path" in theirs inside the loop above this "die".

But the user can be in the same conflicted rename situation with
"git am -3" or cherry-pick, and in these cases there won't be extra
parent commits for the fake work tree commit, hence the conclusion
does not change.

Thanks.

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

[Mike Hommey]

> I suspect that this would be useful without copy detection.  If you
> "git mv fileA fileB" (optionally followed by "edit fileB"), fileB
> would not be in HEAD but you should be able to trace the lineage of
> the lines in it back through the renaming event, and this change
> also allows that use case, no?

It should, but that'd be copy/move detection, wouldn't it? :)

> > I suspect that the above change needs to be updated further if the
> > user wants to run "blame path" during a conflicted renaming merge,
> > i.e.
> >
> >  0. Before two histories diverged, there was old_path.
> >  1. Our side updated contents of that file and kept it at old_path.
> >  2. Their side updated contents of that file and moved it to path.
> >  3. "git merge" notcied the rename, created three stages at "path",
> >     with the result of conflicted content-level merge in the working
> >     tree at "path".
> >  4. The user edits "path" and resolves the conflict, but wants to
> >     double check before running "git add path".
> >  5. "git blame path"
> >
> > Perhaps something like this should suffice:
> >
> >     pos = cache_name_pos(path, strlen(path));
> >     if (0 <= pos)
> >     	; /* ok */
> >     else if (!strcmp(active_cache[-1 - pos]->name), path)
> >         ; /* ok -- just unmerged */
> >     else
> >     	die("no such path in HEAD");
> 
> I do not think the "conflicted renaming merge" example would not be
> a problem in practice iff "git merge" was used, because the fake
> working tree commit would look at both our tree and their tree, and
> find "path" in theirs inside the loop above this "die".

More than that, it seems that's a case that already works without the
patch (it doesn't complain that "no such path in HEAD", although it
only identifies the "theirs" part of the merge conflict when blaming
the file straight out of the merge failure, but that'd be a separate
issue.

> But the user can be in the same conflicted rename situation with
> "git am -3" or cherry-pick, and in these cases there won't be extra
> parent commits for the fake work tree commit, hence the conclusion
> does not change.

Indeed, with cherry-pick, the "no such path in HEAD" error is happening
with the patch.

Mike

[PATCH v3 1/2] blame: Allow to blame paths freshly added to the index

[Mike Hommey]

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 <mh@glandium.org>
---
 builtin/blame.c               | 10 +++++++++-
 t/t8003-blame-corner-cases.sh | 45 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 1e214bd..f297a7f 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2230,6 +2230,7 @@ static int git_blame_config(const char *var, const char *value, void *cb)
 static void verify_working_tree_path(struct commit *work_tree, const char *path)
 {
 	struct commit_list *parents;
+	int pos;
 
 	for (parents = work_tree->parents; parents; parents = parents->next) {
 		const unsigned char *commit_sha1 = parents->item->object.oid.hash;
@@ -2240,7 +2241,14 @@ 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);
+
+	pos = cache_name_pos(path, strlen(path));
+	if (pos >= 0)
+		; /* path is in the index */
+	else if (!strcmp(active_cache[-1 - pos]->name, path))
+		; /* path is in the index, unmerged */
+	else
+		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..eab2e28 100755
--- a/t/t8003-blame-corner-cases.sh
+++ b/t/t8003-blame-corner-cases.sh
@@ -137,6 +137,51 @@ test_expect_success 'blame wholesale copy and more' '
 
 '
 
+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
+
+'
+
+test_expect_success 'blame during cherry-pick with file rename conflict' '
+
+	test_when_finished "git reset --hard && git checkout master" &&
+	git checkout HEAD~3 &&
+	echo MOUSE >> mouse &&
+	git mv mouse rodent &&
+	git add rodent &&
+	GIT_AUTHOR_NAME=Rodent git commit -m "rodent" &&
+	git checkout --detach master &&
+	(git cherry-pick HEAD@{1} || test $? -eq 1) &&
+	git show HEAD@{1}:rodent > rodent &&
+	git add rodent &&
+	git blame -f -C -C1 rodent | sed -e "$pick_fc" >current &&
+	cat current &&
+	cat >expected <<-\EOF &&
+	mouse-Initial
+	mouse-Second
+	rodent-Not
+	EOF
+	test_cmp expected current
+'
+
 test_expect_success 'blame path that used to be a directory' '
 	mkdir path &&
 	echo A A A A A >path/file &&
-- 
2.9.1.277.gdbc86c9.dirty

[PATCH v3 2/2] t/t8003-blame-corner-cases.sh: Use here documents

[Mike Hommey]

Somehow, this test was using:

{
	echo A
	echo B
} > file

block to feed file contents. This changes those to the form most common
in git test scripts:

cat >file <<-\EOF
A
B
EOF

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 t/t8003-blame-corner-cases.sh | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
index eab2e28..e48370d 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,12 @@ 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
 
 '
-- 
2.9.1.277.gdbc86c9.dirty

Re: [PATCH v3 2/2] t/t8003-blame-corner-cases.sh: Use here documents

[Johannes Schindelin]

Hi Mike,

On Sat, 16 Jul 2016, Mike Hommey wrote:

> Somehow, this test was using:
> 
> {
> 	echo A
> 	echo B
> } > file
> 
> block to feed file contents. This changes those to the form most common
> in git test scripts:
> 
> cat >file <<-\EOF
> A
> B
> EOF
> 
> Signed-off-by: Mike Hommey <mh@glandium.org>

I reviewed both patches and like them.

Thanks,
Dscho

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

[Junio C Hamano]

Mike Hommey <mh@glandium.org> writes:

>> I suspect that this would be useful without copy detection.  If you
>> "git mv fileA fileB" (optionally followed by "edit fileB"), fileB
>> would not be in HEAD but you should be able to trace the lineage of
>> the lines in it back through the renaming event, and this change
>> also allows that use case, no?
>
> It should, but that'd be copy/move detection, wouldn't it? :)

Actually, in the context of "git blame", there is no extra
"detection" needed for following a whole file rename.

>> But the user can be in the same conflicted rename situation with
>> "git am -3" or cherry-pick, and in these cases there won't be extra
>> parent commits for the fake work tree commit, hence the conclusion
>> does not change.
>
> Indeed, with cherry-pick, the "no such path in HEAD" error is happening
> with the patch.

OK.

Re: [PATCH v3 2/2] t/t8003-blame-corner-cases.sh: Use here documents

[Junio C Hamano]

Mike Hommey <mh@glandium.org> writes:

> Somehow, this test was using:
>
> {
> 	echo A
> 	echo B
> } > file
>
> block to feed file contents. This changes those to the form most common
> in git test scripts:
>
> cat >file <<-\EOF
> A
> B
> EOF
>
> Signed-off-by: Mike Hommey <mh@glandium.org>
> ---

It is not so strong a preference to ask you to re-roll, but for
future reference, I'd prefer to see this preparatory clean-up early
in the series, followed by the primary thing, IOW, I would have
liked more if the two patches were swapped.

Thanks.  Will queue.

>  t/t8003-blame-corner-cases.sh | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
> index eab2e28..e48370d 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,12 @@ 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
>  
>  '