threads / discuss / 2242

gitk shows an empty line between "Comments" and changed files

Subject: gitk shows an empty line between "Comments" and changed files

## tl;dr

11 messages between Oct 27, 2005 and Oct 29, 2005.

replies: 10people: 3as markdown or json

Pavel Roskin· Oct 27, 2005, 17:30 UTC · lore

Hello!

gitk is now showing an empty line between "Comments" and changes files. This change was caused by 8b7e5d76e836396a097bb6f61cf930ea872a7bd3 (Make "gitk" work better with dense revlists).

git-diff-tree outputs some SHA1 as the first line if only one tree-ish argument is provided. I don't see any way to suppress that output (see diff-tree.c, line 114).

One solution would be to add an option to git-diff-tree to suppress all headers (let's call it --no-headers).

Or maybe the SHA1 header should never be printed at all? It looks like it's not documented anywhere. It doesn't break the tests.

While debugging the patch, I have found that the p variable is unused in both functions that stopped passing it to git-diff-tree.

Also, gettreediffs function in gitk could use --names-only for git-diff-tree, because it only needs names.

Maybe --names-only and --name-status should suppress the SHA1 header to match their descriptions? Unfortunately, they don't suppress the patch is -p is specified, so "matching the descriptions" would have to take care of it.

P.S. I consider printing the SHA1 header in git-diff-tree an undocumented feature (in other words, a bug).

Proposed patch:

Don't print the SHA1 when only one tree-ish is given to git-diff-tree.

Signed-off-by: Pavel Roskin <proski@gnu.org>

diff --git a/diff-tree.c b/diff-tree.c
index 382011a..ac53f48 100644
--- a/diff-tree.c
+++ b/diff-tree.c
@@ -111,7 +111,6 @@ static int diff_tree_commit(const unsign
 		unsigned char parent[20];
 		if (get_sha1_hex(buf + offset + 7, parent))
 			return -1;
-		header = generate_header(name, sha1_to_hex(parent), buf, size);
 		diff_tree_sha1_top(parent, commit, "");
 		if (!header && verbose_header) {
 			header_prefix = "\ndiff-tree ";

-- 
Regards,
Pavel Roskin

Junio C Hamano· Oct 27, 2005, 17:51 UTC · re: Pavel Roskin · lore

Re: gitk shows an empty line between "Comments" and changed files

Pavel Roskin <proski@gnu.org> writes:

> Or maybe the SHA1 header should never be printed at all?  It looks like
> it's not documented anywhere.  It doesn't break the tests.

AFAIK, its only user (except humans) is patch-id.

> P.S. I consider printing the SHA1 header in git-diff-tree an
> undocumented feature (in other words, a bug).

Pavel Roskin· Oct 28, 2005, 01:36 UTC · re: Junio C Hamano · lore

Re: gitk shows an empty line between "Comments" and changed files

Hi, Junio!

On Thu, 2005-10-27 at 10:51 -0700, Junio C Hamano wrote:

Show 6 quoted lines

> Pavel Roskin <proski@gnu.org> writes:
> 
> > Or maybe the SHA1 header should never be printed at all?  It looks like
> > it's not documented anywhere.  It doesn't break the tests.
> 
> AFAIK, its only user (except humans) is patch-id.

Thank you for reply! git-patch-id is badly documented, and its output is not documented at all. It outputs two SHA1 hashes, and the second one is taken from the line produced by git-diff-tree. If there is no such line, the second ID is 0.

git-patch-id is only used by git-cherry. git-cherry writes the second SHA1 to some files in a temporary directory, but it never reads those files, it only checks that they exist.

I'm pretty confident now that the patch I posted in the last message was correct.

Of course, more cleanup will be needed to remove the second ID from git-patch-id, to adjust git-cherry accordingly and to remove "p" assignments in gitk.

-- 
Regards,
Pavel Roskin

Junio C Hamano· Oct 28, 2005, 09:13 UTC · re: Pavel Roskin · lore

Re: gitk shows an empty line between "Comments" and changed files

Pavel Roskin <proski@gnu.org> writes:

> git-patch-id is only used by git-cherry.  git-cherry writes the second
> SHA1 to some files in a temporary directory, but it never reads those
> files, it only checks that they exist.

I do not oppose dropping the commit-id line from the default output, but having it optionally available would be useful in one application. Somebody _could_ write a tool that does something like:

    git-rev-list ^$old_head $new_head |
    git-diff-tree -p -m --stdin --with-commit-ids |
    git-patch-id

to cache the patch-id --> commit-id mappings. If this were kept on the upstream repo for public query, it would be useful for you to find out if your favorite patch as already been merged. For example, gitweb could have an query page to let you submit a patch-id and return the commit (or "no such patch merged yet").

Pavel Roskin· Oct 28, 2005, 22:45 UTC · re: Junio C Hamano · lore

Re: gitk shows an empty line between "Comments" and changed files

Hi, Junio!

On Fri, 2005-10-28 at 02:13 -0700, Junio C Hamano wrote:

Show 14 quoted lines

> Pavel Roskin <proski@gnu.org> writes:
> 
> > git-patch-id is only used by git-cherry.  git-cherry writes the second
> > SHA1 to some files in a temporary directory, but it never reads those
> > files, it only checks that they exist.
> 
> I do not oppose dropping the commit-id line from the default
> output, but having it optionally available would be useful in
> one application.  Somebody _could_ write a tool that does
> something like:
> 
>     git-rev-list ^$old_head $new_head |
>     git-diff-tree -p -m --stdin --with-commit-ids |
>     git-patch-id

Sounds good. Perhaps the commit IDs should have a prefix identifying them.

Another approach would be to use something slightly more elaborate than a pipe. If I understand correctly, the commit ID would be already known from the git-rev-list output. Passing commit IDs through patch-id without actually doing anything with them seems non-elegant. Maybe we could teach git-patch-id (or another script) to get patches by commit-id instead of using stdin?

-- 
Regards,
Pavel Roskin

Linus Torvalds· Oct 28, 2005, 23:00 UTC · re: Pavel Roskin · lore

Re: gitk shows an empty line between "Comments" and changed files

On Fri, 28 Oct 2005, Pavel Roskin wrote:

Show 12 quoted lines

> > 
> > I do not oppose dropping the commit-id line from the default
> > output, but having it optionally available would be useful in
> > one application.  Somebody _could_ write a tool that does
> > something like:
> > 
> >     git-rev-list ^$old_head $new_head |
> >     git-diff-tree -p -m --stdin --with-commit-ids |
> >     git-patch-id
> 
> Sounds good.  Perhaps the commit IDs should have a prefix identifying
> them.

Guys, why do you want to drop it? We've always had it, and it doesn't really hurt.

Yes, gitk got a new empty line because I didn't realize that the output of "git-diff-tree $commit" is slightly different from "git-diff-tree $t2 $t2", but hey, that was due to a gitk change, and I think it should be trivial for gitk to just react to it.

So if we add a new flag, please make it go the other way: one that makes the output really quiet, but keeps the standard output the same.

		Linus

Junio C Hamano· Oct 29, 2005, 02:49 UTC · re: Pavel Roskin · lore

Re: gitk shows an empty line between "Comments" and changed files

Pavel Roskin <proski@gnu.org> writes:

Show 6 quoted lines

>>     git-rev-list ^$old_head $new_head |
>>     git-diff-tree -p -m --stdin --with-commit-ids |
>>     git-patch-id
>
> Sounds good.  Perhaps the commit IDs should have a prefix identifying
> them.

I do not think git-diff-tree -p output can have 40-byte hexadecimal at the beginning of the output anywhere other than commit object names; why clutter output?

> Another approach would be to use something slightly more elaborate than
> a pipe.  If I understand correctly, the commit ID would be already known
> from the git-rev-list output.  Passing commit IDs through patch-id
> without actually doing anything with them seems non-elegant.

Sorry you lost me. I am not sure what you mean by "without actually doing anything" part. The input to patch-id command in the above pipe is (commit-object-name patch)*. The command reads such a stream, and transforms it to a (patch-id commit-object-name)* stream. In other words, the input identifies each patch with a commit-object-name, and the command condenses each patch to a patch-id, and spits them out, labelled with commit-object-name.

Linus Torvalds· Oct 29, 2005, 03:07 UTC · re: Junio C Hamano · lore

Re: gitk shows an empty line between "Comments" and changed files

On Fri, 28 Oct 2005, Junio C Hamano wrote:

Show 9 quoted lines

> 
> Sorry you lost me.  I am not sure what you mean by "without
> actually doing anything" part.  The input to patch-id command in
> the above pipe is (commit-object-name patch)*.  The command
> reads such a stream, and transforms it to a (patch-id
> commit-object-name)* stream.  In other words, the input
> identifies each patch with a commit-object-name, and the command
> condenses each patch to a patch-id, and spits them out, labelled
> with commit-object-name.

Note that git-patch-id will happily take a patch without the commit ID at the head, it just won't have a commit ID to match it up with. For such patches it will just spit it out with an all-zero commit-object-name.

And that's very much by design. The point is that you can match up your (perhaps non-git) patches with what has been accepted. Which is why git-patch-id should always take non-git patches too, and then you can match them up by sorting by patch ID and doing "join -1" to match up duplicates.

So git-patch-id will work with or without the commit ID, but the commit ID is then later needed to figure out _which_ commit you matched up.

		Linus

Pavel Roskin· Oct 29, 2005, 04:23 UTC · re: Linus Torvalds · lore

Re: gitk shows an empty line between "Comments" and changed files

Quoting Linus Torvalds <torvalds@osdl.org>:

Show 5 quoted lines

> Note that git-patch-id will happily take a patch without the commit ID at
> the head, it just won't have a commit ID to match it up with. For such
> patches it will just spit it out with an all-zero commit-object-name.
>
> And that's very much by design.

[snip]

OK, if it's by design, I'll fix gitk only.

I understand the default behavior of git-diff-tree won't change, so I'll simply strip the first line.

-- Regards, Pavel Roskin

Linus Torvalds· Oct 29, 2005, 04:33 UTC · re: Pavel Roskin · lore

Re: gitk shows an empty line between "Comments" and changed files

On Sat, 29 Oct 2005, Pavel Roskin wrote:

> 
> I understand the default behavior of git-diff-tree won't change, so I'll simply
> strip the first line.

Be careful, though. The merge case uses git-diff-tree differently, so I think there is no extra line for a merge.

		Linus

Pavel Roskin· Oct 29, 2005, 04:54 UTC · re: Linus Torvalds · lore

Re: gitk shows an empty line between "Comments" and changed files

Quoting Linus Torvalds <torvalds@osdl.org>:

Show 8 quoted lines

> On Sat, 29 Oct 2005, Pavel Roskin wrote:
> >
> > I understand the default behavior of git-diff-tree won't change, so I'll
> simply
> > strip the first line.
>
> Be careful, though. The merge case uses git-diff-tree differently, so I
> think there is no extra line for a merge.

Sure.  Thanks.

-- Regards, Pavel Roskin