threads / discuss / 15448

Commit 140b378d07229e breaks gitk highlighting

Subject: Commit 140b378d07229e breaks gitk highlighting

## tl;dr

4 messages between Sep 9, 2008 and Sep 10, 2008.

replies: 3people: 3as markdown or json

Mark Levedahl· Sep 9, 2008, 12:31 UTC · lore

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
Karl Hasselström· Sep 10, 2008, 16:01 UTC · re: Mark Levedahl · lore

Re: Commit 140b378d07229e breaks gitk highlighting

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
Junio C Hamano· Sep 10, 2008, 19:22 UTC · re: Mark Levedahl · lore

Re: Commit 140b378d07229e breaks gitk highlighting

"Mark Levedahl" <mlevedahl@gmail.com> writes:
Show 12 quoted lines
> 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)
Mark Levedahl· Sep 10, 2008, 20:03 UTC · re: Junio C Hamano · lore

Re: Commit 140b378d07229e breaks gitk highlighting

On Wed, Sep 10, 2008 at 3:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 10 quoted lines
>
> 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.
>
Show 15 quoted lines
> 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

← back to recent threads