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

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

[Pavel Roskin]

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

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

[Junio C Hamano]

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

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

[Pavel Roskin]

Hi, Junio!

On Thu, 2005-10-27 at 10:51 -0700, Junio C Hamano wrote:
> 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

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

[Junio C Hamano]

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").

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

[Pavel Roskin]

Hi, Junio!

On Fri, 2005-10-28 at 02:13 -0700, Junio C Hamano wrote:
> 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

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

[Linus Torvalds]

On Fri, 28 Oct 2005, Pavel Roskin wrote:
> > 
> > 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

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

[Junio C Hamano]

Pavel Roskin <proski@gnu.org> writes:

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

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

[Linus Torvalds]

On Fri, 28 Oct 2005, Junio C Hamano wrote:
> 
> 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

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

[Pavel Roskin]

Quoting Linus Torvalds <torvalds@osdl.org>:

> 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

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

[Linus Torvalds]

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

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

[Pavel Roskin]

Quoting Linus Torvalds <torvalds@osdl.org>:

> 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