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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 15, 2016, 20:58 UTC
Message-ID: <xmqqvb06lita.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160715024254.29186-1-mh@glandium.org>

Mike Hommey <mh@glandium.org> writes:

Show 11 quoted lines

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

Show 17 quoted lines

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