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

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

[Edmundo Carmona Antoranz]

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 <eantoranz@gmail.com>
---
 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

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

[Eric Sunshine]

On Mon, Nov 16, 2015 at 8:29 PM, Edmundo Carmona Antoranz
<eantoranz@gmail.com> 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.

> Signed-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>
> ---
> diff --git 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

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

[Jeff King]

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

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

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

[Stefan Beller]

On Tue, Nov 17, 2015 at 2:48 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Nov 17, 2015 at 12:11:25AM -0500, Eric Sunshine wrote:
>
>> > 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

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

[Eric Sunshine]

On Tue, Nov 17, 2015 at 5:48 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Nov 17, 2015 at 12:11:25AM -0500, Eric Sunshine wrote:
>> > 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.

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

[Jeff King]

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

> On Tue, Nov 17, 2015 at 5:48 PM, Jeff King <peff@peff.net> wrote:
> > On Tue, Nov 17, 2015 at 12:11:25AM -0500, Eric Sunshine wrote:
> >> > 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 <peff@peff.net>
---
 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

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

[Eric Sunshine]

On Tue, Nov 17, 2015 at 6:22 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Nov 17, 2015 at 06:01:25PM -0500, Eric Sunshine wrote:
>> On Tue, Nov 17, 2015 at 5:48 PM, Jeff King <peff@peff.net> wrote:
>> > 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 <sunshine@sunshineco.com>

> -- >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 <peff@peff.net>
> ---
>  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
>

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

[Edmundo Carmona Antoranz]

On Mon, Nov 16, 2015 at 11:11 PM, Eric Sunshine <sunshine@sunshineco.com> 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.

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

[Edmundo Carmona Antoranz]

On Tue, Nov 17, 2015 at 5:37 PM, Edmundo Carmona Antoranz
<eantoranz@gmail.com> 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!