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

[Eric Sunshine <sunshine@sunshineco.com>]

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
>