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

>  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

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

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

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

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

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

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

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

>> 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? :)

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

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

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