Commit 140b378d07229e breaks gitk highlighting

Commit 140b378d07229e breaks gitk highlighting

[Mark Levedahl]

gitk since the above commit ("Teach git diff-tree --stdin to diff
trees") fails to reliably highlight commits matching a given path
specification. For an example of the problem,

   $gitk ec3a4ba519c001

   and select commit "touching paths" with "t" in the entry bar

The 4'th commit down (t9124: clean up chdir usage) is NOT highlighted,
though it clearly touches t. Reverting commit 140b378 restores the
correct behavior. This gets into parts of the code I don't understand,
so I decline to try to offer a patch.

Mark

Re: Commit 140b378d07229e breaks gitk highlighting

[Karl Hasselström]

On 2008-09-09 08:31:19 -0400, Mark Levedahl wrote:

> gitk since the above commit ("Teach git diff-tree --stdin to diff
> trees") fails to reliably highlight commits matching a given path
> specification.

Hmmm, not good. Any idea of what git commands gitk actually runs in
your example? Would be nice to be able to diff the output before and
after the change.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: Commit 140b378d07229e breaks gitk highlighting

[Junio C Hamano]

"Mark Levedahl" <mlevedahl@gmail.com> writes:

> gitk since the above commit ("Teach git diff-tree --stdin to diff
> trees") fails to reliably highlight commits matching a given path
> specification. For an example of the problem,
>
>    $gitk ec3a4ba519c001
>
>    and select commit "touching paths" with "t" in the entry bar
>
> The 4'th commit down (t9124: clean up chdir usage) is NOT highlighted,
> though it clearly touches t. Reverting commit 140b378 restores the
> correct behavior. This gets into parts of the code I don't understand,
> so I decline to try to offer a patch.

Feeding the following to "git diff-tree -r -s --stdin -- t" misbehaves
with that change.  The second line is skipped.

7c4d0219cf9ab6a7738a09ad7fec72d5e9f2ac67
a786091b4a487bc08bbff4864717cf5d8383e983
4a09bc966449ca0a7e9a5bb70f91b47debdd7c4e

This should fix it.

I think we should have a stronger warning against misusing lookup_object()
without knowing what it does.  It is marked "Internal only" in the header,
but it does not explain why.  The interface is there to help the lazy
object lookup (e.g. you parse a commit and learn about a parent it has.
You do not want to read the parent commit at that point, so you create a
placeholder commit object and point at it from the child.  You would want
to make sure that you populate the same placeholder object with the data
you read from the parent commit when it gets the turn of the parent commit
to be parsed).

diff --git a/builtin-diff-tree.c b/builtin-diff-tree.c
index 1138c2d..a9e32c9 100644
--- a/builtin-diff-tree.c
+++ b/builtin-diff-tree.c
@@ -71,8 +71,7 @@ static int diff_tree_stdin(char *line)
 	line[len-1] = 0;
 	if (get_sha1_hex(line, sha1))
 		return -1;
-	obj = lookup_object(sha1);
-	obj = obj ? obj : parse_object(sha1);
+	obj = parse_object(sha1);
 	if (!obj)
 		return -1;
 	if (obj->type == OBJ_COMMIT)

Re: Commit 140b378d07229e breaks gitk highlighting

[Mark Levedahl]

On Wed, Sep 10, 2008 at 3:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Feeding the following to "git diff-tree -r -s --stdin -- t" misbehaves
> with that change.  The second line is skipped.
>
> 7c4d0219cf9ab6a7738a09ad7fec72d5e9f2ac67
> a786091b4a487bc08bbff4864717cf5d8383e983
> 4a09bc966449ca0a7e9a5bb70f91b47debdd7c4e
>
> This should fix it.
>

> diff --git a/builtin-diff-tree.c b/builtin-diff-tree.c
> index 1138c2d..a9e32c9 100644
> --- a/builtin-diff-tree.c
> +++ b/builtin-diff-tree.c
> @@ -71,8 +71,7 @@ static int diff_tree_stdin(char *line)
>        line[len-1] = 0;
>        if (get_sha1_hex(line, sha1))
>                return -1;
> -       obj = lookup_object(sha1);
> -       obj = obj ? obj : parse_object(sha1);
> +       obj = parse_object(sha1);
>        if (!obj)
>                return -1;
>        if (obj->type == OBJ_COMMIT)
>

Yes, that fixes the problems I see.

Thanks,
Mark