[PATCH v2] blame: avoid checking if a file exists on the working tree if a revision is provided

If a file has been deleted/renamed, blame refused to display
blame content even if the path provided does match an existing
blob on said revision.

$ git status
On branch hide
Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

        deleted:    testfile1.txt

no changes added to commit (use "git add" and/or "git commit -a")

Before:
$ git blame testfile1.txt
fatal: cannot stat path 'testfile1.txt': No such file or directory
$ git blame testfile1.txt HEAD
fatal: cannot stat path 'testfile1.txt': No such file or directory

After:
$ git blame testfile1.txt
fatal: Cannot lstat 'testfile1.txt': No such file or directory
$ git blame testfile1.txt HEAD
^da1a96f testfile2.txt (Edmundo Carmona Antoranz 2015-11-10 17:52:00 -0600 1) testfile 2
^da1a96f testfile2.txt (Edmundo Carmona Antoranz 2015-11-10 17:52:00 -0600 2)
^da1a96f testfile2.txt (Edmundo Carmona Antoranz 2015-11-10 17:52:00 -0600 3) Some content

Signed-off-by: Edmundo Carmona Antoranz <redacted>
---
 builtin/blame.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 83612f5..856971a 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c

@@ -2683,12 +2683,13 @@ parse_done:
 		argv[argc - 1] = "--";
 
 		setup_work_tree();
-		if (!file_exists(path))
-			die_errno("cannot stat path '%s'", path);
 	}
 
 	revs.disable_stdin = 1;
 	setup_revisions(argc, argv, &revs, NULL);
+	if (!revs.pending.nr && !file_exists(path))
+		die_errno("cannot stat path '%s'", path);
+
 	memset(&sb, 0, sizeof(sb));
 
 	sb.revs = &revs;

-- 
2.6.2

On Mon, Nov 16, 2015 at 8:29 PM, Edmundo Carmona Antoranz
[off-list ref] wrote:

blame: avoid checking if a file exists on the working tree
if a revision is provided

This subject is a bit long; try to keep it to about 72 characters or less.

More importantly, though, it doesn't give us a high level overview of
the purpose of the patch, which is that it is fixing blame to work on
a file at a given revision even if the file no longer exists in the
worktree.

If a file has been deleted/renamed, blame refused to display

Imperative: s/refused/refuses/

blame content even if the path provided does match an existing
blob on said revision.

git-blame documentation does not advertise "blame <file> <rev>" as a
valid invocation. It does advertise "blame <rev> -- <file>", and this
case already works correctly even when <file> does not exist in the
worktree.

git-annotate documentation, on the other hand, does advertise
"annotate <file> <rev>", and it fails to work when <file> is absent
from the worktree, so perhaps you want to sell this patch as fixing
git-annotate instead?

$ git status
On branch hide
Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

        deleted:    testfile1.txt

no changes added to commit (use "git add" and/or "git commit -a")

Before:
$ git blame testfile1.txt
fatal: cannot stat path 'testfile1.txt': No such file or directory
$ git blame testfile1.txt HEAD
fatal: cannot stat path 'testfile1.txt': No such file or directory

After:
$ git blame testfile1.txt
fatal: Cannot lstat 'testfile1.txt': No such file or directory
$ git blame testfile1.txt HEAD
^da1a96f testfile2.txt (Edmundo Carmona Antoranz 2015-11-10 17:52:00 -0600 1) testfile 2
^da1a96f testfile2.txt (Edmundo Carmona Antoranz 2015-11-10 17:52:00 -0600 2)
^da1a96f testfile2.txt (Edmundo Carmona Antoranz 2015-11-10 17:52:00 -0600 3) Some content

This example is certainly illustrative, but an even more common case
may be trying to blame/annotate a file which existed in an older
revision but doesn't exist anymore at HEAD, thus is absent from the
worktree. Such a case could likely be described in a sentence or two
without resorting to verbose examples (though, not a big deal if you
keep the example).

A new test or two would be welcome, possibly in t/annotate-tests.sh if
you consider this also fixing git-blame, or in t8001-annotate.sh if
you're selling it only as a fix for git-annotate.

On Tue, Nov 17, 2015 at 12:11:25AM -0500, Eric Sunshine wrote:

quoted

blame content even if the path provided does match an existing
blob on said revision.

git-blame documentation does not advertise "blame <file> <rev>" as a
valid invocation. It does advertise "blame <rev> -- <file>", and this
case already works correctly even when <file> does not exist in the
worktree.

Hmm. Out of curiosity I tried:

  git blame v2.4.0 -- t/t6031-merge-recursive.sh

and it segfaults. This bisects to Max's recent 1b0d400 (blame: extract
find_single_final, 2015-10-30), but I do not see anything obviously
wrong with it from a quick glance.

-Peff

On Tue, Nov 17, 2015 at 2:48 PM, Jeff King [off-list ref] wrote:

On Tue, Nov 17, 2015 at 12:11:25AM -0500, Eric Sunshine wrote:

quoted

quoted

blame content even if the path provided does match an existing
blob on said revision.

git-blame documentation does not advertise "blame <file> <rev>" as a
valid invocation. It does advertise "blame <rev> -- <file>", and this
case already works correctly even when <file> does not exist in the
worktree.

Hmm. Out of curiosity I tried:

  git blame v2.4.0 -- t/t6031-merge-recursive.sh

and it segfaults. This bisects to Max's recent 1b0d400 (blame: extract
find_single_final, 2015-10-30), but I do not see anything obviously
wrong with it from a quick glance.

it did not fail when running through gdb, so I conclude it is a memory issue
(like using an uninitialized pointer, or memory allocation too small).

valgrind produces:

==18444== Process terminating with default action of signal 11 (SIGSEGV)
==18444==  General Protection Fault
==18444==    at 0x4032121: strcmp (valgrind/memcheck/mc_replace_strmem.c:725)
==18444==    by 0x41A86F: get_origin
(/usr/local/google/home/sbeller/OSS/git/builtin/blame.c:483)
==18444==    by 0x4201EF: cmd_blame
(/usr/local/google/home/sbeller/OSS/git/builtin/blame.c:2763)
==18444==    by 0x405896: run_builtin
(/usr/local/google/home/sbeller/OSS/git/git.c:350)
==18444==    by 0x405AA4: handle_builtin
(/usr/local/google/home/sbeller/OSS/git/git.c:536)
==18444==    by 0x405BC0: run_argv
(/usr/local/google/home/sbeller/OSS/git/git.c:582)
==18444==    by 0x405DB8: main
(/usr/local/google/home/sbeller/OSS/git/git.c:690)
==18444==

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Nov 17, 2015 at 5:48 PM, Jeff King [off-list ref] wrote:

On Tue, Nov 17, 2015 at 12:11:25AM -0500, Eric Sunshine wrote:

quoted

quoted

blame content even if the path provided does match an existing
blob on said revision.

git-blame documentation does not advertise "blame <file> <rev>" as a
valid invocation. It does advertise "blame <rev> -- <file>", and this
case already works correctly even when <file> does not exist in the
worktree.

Hmm. Out of curiosity I tried:

  git blame v2.4.0 -- t/t6031-merge-recursive.sh

and it segfaults. This bisects to Max's recent 1b0d400 (blame: extract
find_single_final, 2015-10-30), but I do not see anything obviously
wrong with it from a quick glance.

In the original code, sb->final received was assigned value of obj,
which may have gone through deref_tag(), however, after 1b0d400,
sb->final is unconditionally assigned the original value of obj, not
the (potentially) deref'd value.

On Tue, Nov 17, 2015 at 06:01:25PM -0500, Eric Sunshine wrote:

On Tue, Nov 17, 2015 at 5:48 PM, Jeff King [off-list ref] wrote:

quoted

On Tue, Nov 17, 2015 at 12:11:25AM -0500, Eric Sunshine wrote:

quoted

quoted

blame content even if the path provided does match an existing
blob on said revision.

git-blame documentation does not advertise "blame <file> <rev>" as a
valid invocation. It does advertise "blame <rev> -- <file>", and this
case already works correctly even when <file> does not exist in the
worktree.

Hmm. Out of curiosity I tried:

  git blame v2.4.0 -- t/t6031-merge-recursive.sh

and it segfaults. This bisects to Max's recent 1b0d400 (blame: extract
find_single_final, 2015-10-30), but I do not see anything obviously
wrong with it from a quick glance.

In the original code, sb->final received was assigned value of obj,
which may have gone through deref_tag(), however, after 1b0d400,
sb->final is unconditionally assigned the original value of obj, not
the (potentially) deref'd value.

Good eye. I fed it a tag, find_single_final knows that points to a
commit, then prepare_final casts the tag object to a commit. Whoops.

The patch below fixes it for me. It probably needs a test, but I have to
run for the moment.

-- >8 --
Subject: [PATCH] blame: fix object casting regression

Commit 1b0d400 refactored the prepare_final() function so
that it could be reused in multiple places. Originally, the
loop had two outputs: a commit to stuff into sb->final, and
the name of the commit from the rev->pending array.

After the refactor, that loop is put in its own function
with a single return value: the object_array_entry from the
rev->pending array. This contains both the name and the object,
but with one important difference: the object is the
_original_ object found by the revision parser, not the
dereferenced commit. If one feeds a tag to "git blame", we
end up casting the tag object to a "struct commit", which
causes a segfault.

Instead, let's return the commit (properly casted) directly
from the function, and take the "name" as an optional
out-parameter. This does the right thing, and actually
simplifies the callers, who no longer need to cast or
dereference the object_array_entry themselves.

Signed-off-by: Jeff King <redacted>
---
 builtin/blame.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index ac36738..2184e39 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c

@@ -2403,10 +2403,12 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
 	return commit;
 }
 
-static struct object_array_entry *find_single_final(struct rev_info *revs)
+static struct commit *find_single_final(struct rev_info *revs,
+					const char **name_p)
 {
 	int i;
-	struct object_array_entry *found = NULL;
+	struct commit *found = NULL;
+	const char *name = NULL;
 
 	for (i = 0; i < revs->pending.nr; i++) {
 		struct object *obj = revs->pending.objects[i].item;

@@ -2418,22 +2420,20 @@ static struct object_array_entry *find_single_final(struct rev_info *revs)
 			die("Non commit %s?", revs->pending.objects[i].name);
 		if (found)
 			die("More than one commit to dig from %s and %s?",
-			    revs->pending.objects[i].name,
-			    found->name);
-		found = &(revs->pending.objects[i]);
+			    revs->pending.objects[i].name, name);
+		found = (struct commit *)obj;
+		name = revs->pending.objects[i].name;
 	}
+	if (name_p)
+		*name_p = name;
 	return found;
 }
 
 static char *prepare_final(struct scoreboard *sb)
 {
-	struct object_array_entry *found = find_single_final(sb->revs);
-	if (found) {
-		sb->final = (struct commit *) found->item;
-		return xstrdup(found->name);
-	} else {
-		return NULL;
-	}
+	const char *name;
+	sb->final = find_single_final(sb->revs, &name);
+	return xstrdup_or_null(name);
 }
 
 static char *prepare_initial(struct scoreboard *sb)

@@ -2721,11 +2721,9 @@ parse_done:
 		die("Cannot use --contents with final commit object name");
 
 	if (reverse && revs.first_parent_only) {
-		struct object_array_entry *entry = find_single_final(sb.revs);
-		if (!entry)
+		final_commit = find_single_final(sb.revs, NULL);
+		if (!final_commit)
 			die("--reverse and --first-parent together require specified latest commit");
-		else
-			final_commit = (struct commit*) entry->item;
 	}
 
 	/*

-- 
2.6.3.636.g1460207

On Mon, Nov 16, 2015 at 11:11 PM, Eric Sunshine [off-list ref] wrote:

This subject is a bit long; try to keep it to about 72 characters or less.

More importantly, though, it doesn't give us a high level overview of
the purpose of the patch, which is that it is fixing blame to work on
a file at a given revision even if the file no longer exists in the
worktree.

Sure!

git-blame documentation does not advertise "blame <file> <rev>" as a
valid invocation. It does advertise "blame <rev> -- <file>", and this
case already works correctly even when <file> does not exist in the
worktree.

git-annotate documentation, on the other hand, does advertise
"annotate <file> <rev>", and it fails to work when <file> is absent
from the worktree, so perhaps you want to sell this patch as fixing
git-annotate instead?

So, do I forget about the blame patch (given that I'm not fixing an
advertised syntax, even if it's supported) and fix annotate instead or
do I fix both? And if I should swing for both, do I create a single
patch or a chain of two patches, one for each builtin?

This example is certainly illustrative, but an even more common case
may be trying to blame/annotate a file which existed in an older
revision but doesn't exist anymore at HEAD, thus is absent from the
worktree. Such a case could likely be described in a sentence or two
without resorting to verbose examples (though, not a big deal if you
keep the example).

K.

A new test or two would be welcome, possibly in t/annotate-tests.sh if
you consider this also fixing git-blame, or in t8001-annotate.sh if
you're selling it only as a fix for git-annotate.

I guess I'll wait for feedback about my first question before I decide
what I will do about the tests.

Thank you very much for your comments, Eric.

On Tue, Nov 17, 2015 at 5:37 PM, Edmundo Carmona Antoranz
[off-list ref] wrote:

So, do I forget about the blame patch (given that I'm not fixing an
advertised syntax, even if it's supported) and fix annotate instead or
do I fix both? And if I should swing for both, do I create a single
patch or a chain of two patches, one for each builtin?

Actually, cmd_annotate will call cmd_blame so this patch actually
fixes annotate as well (nice unintended consequence).

So I guess it will be a single patch. Let me work on the tests and
then I'll send a patch that will hopefully cover all raised points.

Cheers!

It was discovered [1] that 1b0d40000a broke blaming starting from tag object.
Add test which verifies such blaming works.

[1] http://mid.gmane.org/20151117224809.GE27862@sigill.intra.peff.net

Signed-off-by: Max Kirillov <redacted>
---
Thank you. Sorry, I should have noticed that obj ended up unused.

Adding test case for it. Looks ok after the fix.
 t/annotate-tests.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
index b1673b3..093832f 100644
--- a/t/annotate-tests.sh
+++ b/t/annotate-tests.sh

@@ -68,6 +68,13 @@ test_expect_success 'blame 1 author' '
 	check_count A 2
 '
 
+test_expect_success 'blame by tag objects' '
+	git tag -m "test tag" testTag &&
+	git tag -m "test tag #2" testTag2 testTag &&
+	check_count -h testTag A 2 &&
+	check_count -h testTag2 A 2
+'
+
 test_expect_success 'setup B lines' '
 	echo "2A quick brown fox jumps over the" >>file &&
 	echo "lazy dog" >>file &&

-- 
2.3.4.2801.g3d0809b

On Wed, Nov 18, 2015 at 07:36:32AM +0200, Max Kirillov wrote:

It was discovered [1] that 1b0d40000a broke blaming starting from tag object.
Add test which verifies such blaming works.

[1] http://mid.gmane.org/20151117224809.GE27862@sigill.intra.peff.net

Signed-off-by: Max Kirillov <redacted>
---
Thank you. Sorry, I should have noticed that obj ended up unused.

Thanks. Do you mind if I just squash this into the patch with the fix?

-Peff

On Tue, Nov 17, 2015 at 6:22 PM, Jeff King [off-list ref] wrote:

On Tue, Nov 17, 2015 at 06:01:25PM -0500, Eric Sunshine wrote:

quoted

On Tue, Nov 17, 2015 at 5:48 PM, Jeff King [off-list ref] wrote:

quoted

Hmm. Out of curiosity I tried:

  git blame v2.4.0 -- t/t6031-merge-recursive.sh

and it segfaults. This bisects to Max's recent 1b0d400 (blame: extract
find_single_final, 2015-10-30), but I do not see anything obviously
wrong with it from a quick glance.

In the original code, sb->final received was assigned value of obj,
which may have gone through deref_tag(), however, after 1b0d400,
sb->final is unconditionally assigned the original value of obj, not
the (potentially) deref'd value.

Good eye. I fed it a tag, find_single_final knows that points to a
commit, then prepare_final casts the tag object to a commit. Whoops.

The patch below fixes it for me. It probably needs a test, but I have to
run for the moment.

Sorry for the late response. This patch mirrors my thoughts on fixing
the bug, and appears correct. For what it's worth:

Reviewed-by: Eric Sunshine <redacted>